UCM2: Intel: sof-hda-dsp: Control SOF processing from UCM #49

singalsu · 2024-04-30T09:41:13Z

No description provided.

singalsu · 2024-04-30T09:47:43Z

ucm2/sof/ipc3/eq_fir/pass.blob

There should be a readme text file that describes how these were generated (currently with scripts in sof/tools/tune/).

we need a mechanism by which people can install new settings without overriding the existing ones - because at each distro update the new settings would be overridden back...

Something like a search path with a hierarchy (/etc/ or ~/.config) or something.

This could be interesting as well: are settings user-specific or card-specific?

That's a good idea, I'll try to add support for that. I'd assume settings are card specific, so another directory that is safe from distribution updates will be useful for enthusiasts. I'll see if UCM can access e.g. $HOME.

ujfalusi

@singalsu, impressive to say the least, I have just few comments to consider.

ucm2/Intel/sof-hda-dsp/HiFi-analog-sof.conf

ucm2/sof/product_configs/AAEON/UPX-TGL01.conf

ujfalusi · 2024-04-30T12:09:15Z

ucm2/Intel/sof-hda-dsp/HiFi.conf

@@ -8,7 +8,7 @@ SectionVerb {
 	Value.TQ "HiFi"
 }

-Include.hda-analog.File "/HDA/HiFi-analog.conf"
+Include.hda-analog.File "/Intel/sof-hda-dsp/HiFi-analog-sof.conf"


you could drop the -sof postfix, the conf file is under sof-hda-dsp anyways?

How would the support for sof-soundwire going to be added?

Yep, I will do it. I hope I can soon start also with SDW, it will be simpler than this thanks to endpoint dedicated DAI pipelines.

so for SoundWire i'd really like to see no configurations in Enable, it should all be boot sequences.

plbossart

It's an excellent start @singalsu, but there are multiple concerns on editing files, overriding defaults and more importantly whether the settings actually need to be applied on an endpoint enable/disable. If we can provide the settings at boot time and let the frameworks update the settings when an endpoint is enabled we would simplify the maintenance of all these files.

plbossart · 2024-04-30T13:46:02Z

ucm2/sof/ipc3/eq_fir/pass.blob

we need a mechanism by which people can install new settings without overriding the existing ones - because at each distro update the new settings would be overridden back...

Something like a search path with a hierarchy (/etc/ or ~/.config) or something.

This could be interesting as well: are settings user-specific or card-specific?

plbossart · 2024-04-30T13:47:51Z

ucm2/Intel/sof-hda-dsp/HiFi.conf

@@ -8,7 +8,7 @@ SectionVerb {
 	Value.TQ "HiFi"
 }

-Include.hda-analog.File "/HDA/HiFi-analog.conf"
+Include.hda-analog.File "/Intel/sof-hda-dsp/HiFi-analog-sof.conf"


I can't recall why the initial part was 'HDA' but it was intentional.

I don't fully understand the impact of renaming the path, but this is potentially problematic.

It would be much better to add DSP configuration on top of the /HDA/HiFi-analog.conf config. Just add only appropriate configs to the proper configuration tree like:

Include.hda-analog.File "/HDA/HiFi-analog.conf" SectionDevice."Headphones" { If.headphone_with_drc { Condition { Type ControlExists Control "name='${var:PostMixerAnalogPlaybackDRCSwitch}'" } True { EnableSequence [ cset "name='${var:PostMixerAnalogPlaybackDRCSwitch}' off" cset-tlv "name='${var:PostMixerAnalogPlaybackDRCBytes}' ${var:HeadphoneDrcBlob}" .... finish ... ] } } }

Also, if something repeats, you may consider to use macros...

Thanks @perexg , that would be better way indeed, and there's lot of repeats. I'll do that for next update.

Also, if something repeats, you may consider to use macros...

@perexg I don't seem to be able to use macros inside EnableSequence []. I get e.g. error ALSA lib parser.c:1237:(parse_sequence) error: sequence command 'Macro.id2.HeadphoneSofDrcEqEnable' is ignored if I try such.

plbossart · 2024-04-30T13:49:53Z

ucm2/Intel/sof-hda-dsp/HiFi-analog-sof.conf

+	      PostMixerAnalogPlaybackFIRBytes "Post Mixer Analog Playback FIR Eq bytes"
+	      PostMixerAnalogPlaybackDRCBytes "Post Mixer Analog Playback DRC bytes"
+	      PostMixerAnalogPlaybackDRCSwitch "Post Mixer Analog Playback DRC switch"
+	      SpeakerIirBlob "${ConfTopDir}/sof/${var:SOFIPCVer}/eq_iir/highpass_100hz_0db_48khz.blob"


hard-coding the path makes it difficult to override with a different blob.

I don't have a solution but maybe there should be a separate variable for the path prefix that can use an environment variable or something.

A variable that holds ${ConfTopDir}/sof/${var:SOFIPCVer} would be simple and clean up the code a bit. These are the defaults so I don't expect them to change much. For customize that I'd need help from experts.

Yep, it's possible to access environment variables, with ${env:}. @plbossart Do you have suggestions for it and should it override ${ConfTopDir}/sof or even the ${var:SOFIPCVer} too?

I was only referring to a path change (similar to what we do for firmware/topology).

the IPC version cannot be changed IMHO, it's a given on a platform. On recent kernels we expose it with debugfs, but that requires 'sudo' access.

plbossart · 2024-04-30T13:51:05Z

ucm2/Intel/sof-hda-dsp/HiFi-analog-sof.conf

+	}
+}
+
+# Include blob customizations: sys_vendor / product_name .conf


I don't understand the "Include blob customizations', how does this override the defaults?

The file ProductConfigSOF, it it exists, can re-define the previously defined default blobs, e.g. SpeakerFirBlob to enable the FIR filter that is by default passthrough. Or the SpeakerIirBlob that is set for default 100 Hz high-pass.

Isn't it simpler conceptually to do

if (product config)
include product_config_file
else
include default_config_file

It's very hard to debug if parts of the config are overridden and others are not. Only experts would understand what they are doing, the rest of the world needs things to be error-proof.

Yes it could be like that. The benefit of current way is that the product config does not need to define every variable, only the ones that should change. E.g. headphone settings should be changed from pass-through default to something else only by user.

With the if - else you propose few code lines would be saved but there would be risk for UCM silently not working if product config misses to set some variables.

ucm2/Intel/sof-hda-dsp/HiFi-analog-sof.conf

plbossart · 2024-04-30T13:54:07Z

ucm2/Intel/sof-hda-dsp/HiFi-analog-sof.conf

+					cset-tlv "name='${var:PostMixerAnalogPlaybackDRCBytes}' ${var:HeadphoneDrcBlob}"
+					cset-tlv "name='${var:PostMixerAnalogPlaybackIIRBytes}' ${var:HeadphoneIirBlob}"
+					cset-tlv "name='${var:PostMixerAnalogPlaybackFIRBytes}' ${var:HeadphoneFirBlob}"
+					#shell "/bin/echo 'Headphone DRC EQ ${var:HeadphoneIirBlob} ${var:HeadphoneFirBlob} ${var:SOFConfFullPath}' >> /tmp/alsa-ucm.txt"


is this really a sequence that needs to be done when enabling an endpoint, or could it be done at boot time?

Boot time sequences are stored separately in a different file.

and now that I think of it, I wonder if the only difference between the processing-enabled cases or not enabled should be the boot settings.

In theory when enabling a path the controls are restored by DAPM, are they not? If I am correct we could simplify all this and just provide a bunch of settings. Maybe some of the values will not be used, e.g. if a user never enables a given path, but that's considerably less complicated to maintain.

This is needed with HDA with shared DAI for endpoints. I'm planning to add for EQ components similar on/off switch as DRC has. With such it would be possible to have boot time passed blob and passthrough for anything else but speaker and just control the switch control. But it would also limit customize of other endpoints with this framework. So, I chose to transmit the blob in every EnableSequence. But e.g. with SDW with separate PCMs and DAIs for endpoints there is no need to do runtime blobs passing. I should work with such topology next since SDW PC notebooks are appearing.

The EQ firmware also has capability for enum type EQ presets (but no control exposed) so such could work for convertible notebooks, if UCM would be aware of orientation. I'll implement first the switch. The enum control add needs some thinking it it's feasible.

Note: DRC would work in git main builds with just switch control, and I don't currently see need for playback DRC for other than notebook kind of speaker. But the signed FW release that I'm using has non-working switch control. It requires a stream start for switch to get and hear the boost impact. While with blob send it always works. So, in this work I'm also always sending the DRC blob. Hope the new FW releases include this DRC fix.

Ah yes for HDaudio the settings depend on how the endpoint is configured (speaker or headphone). Same for input if there's no PCH-DMIC.

Indeed I don't see how we can avoid configurations during the Enable step. Thanks for the clarification.

plbossart · 2024-04-30T13:58:28Z

ucm2/sof/product_configs/AAEON/UPX-TGL01.conf

+#Define.SpeakerDrcBlob "/usr/share/alsa/ucm2/sof/ipc4/drc/speaker_aaeon_upx-tgl01.blob"
+#Define.HeadphoneIirBlob "/usr/share/alsa/ucm2/sof/ipc4/eq_iir/headphone_aaeon_upx-tgl01.blob"
+#Define.HeadphoneFirBlob "/usr/share/alsa/ucm2/sof/ipc4/eq_fir/headphone_aaeon_upx-tgl01.blob"
+#Define.HeadphoneDrcBlob "/usr/share/alsa/ucm2/sof/ipc4/drc/headphone_aaeon_upx-tgl01.blob"


Sorry, we should not rely on people editing files. That's a big no-no for files updated by a distro.

I added those as comments for what a vendor can customize, it might not be clear from the HiFi-analog-sof.conf. Your idea for user's customization is good so I will try to add such possibility. It will be these same UCM variables.

Also I'm not sure UPX is a good example, not sure it can ever have UCM in speaker mode. But since Intel is not selling consumer devices, not even NuC any more, so I though UPX that most of us developers use, might be the best harmless example.

singalsu · 2024-05-06T15:22:51Z

@perexg I just updated this PR. Is the modification to HDA/HiFi-analog.conf like you suggested?

I tried to move entire speaker and headphone control to a macro and pass to it the endpoint=Headphone or endpoint=Speaker. But the macro apparently can't use e.g. cset-tlv "name='${var:PostMixerAnalogPlaybackDRCBytes}' ${var:${var:__endpoint}DrcBlob}". So I could not find good usage for the macro feature.

perexg · 2024-05-06T17:09:01Z

@perexg I just updated this PR. Is the modification to HDA/HiFi-analog.conf like you suggested?

Unfortnunately no. HDA/HiFi-analog.conf should not be touched at all. Just include it to your file and do the modification to the Device sections on top. It is not required to define the whole device - just add new things. Eventually, you can keep those changes in a separarate file and include HDA/HiFi-analog.conf and this new file from the toplevel one. The trees are merged. See my original hint.

Also note that there's execution order for the dynamically evaluated trees - see https://www.alsa-project.org/alsa-doc/alsa-lib/group__ucm__conf.html (Configuration tree evaluation paragraph).

I tried to move entire speaker and headphone control to a macro and pass to it the endpoint=Headphone or endpoint=Speaker. But the macro apparently can't use e.g. cset-tlv "name='${var:PostMixerAnalogPlaybackDRCBytes}' ${var:${var:__endpoint}DrcBlob}". So I could not find good usage for the macro feature.

Indirect variable references should be supported as:

Define.a "${var:__endpoint}DrcBlob}"
Define.b "${var:$a}"

The inner expression evaluations are not supported.

singalsu · 2024-05-07T13:45:58Z

Thanks @perexg ! I hope I got right now. This version seems to work OK in my tests.

plbossart

only minor comments.
The DMI convention will probably fail on a number of devices, but that's a good start.

plbossart · 2024-05-07T14:17:39Z

ucm2/Intel/sof-hda-dsp/HiFi-sof.conf

+                                cset-tlv "name='${var:PostMixerAnalogPlaybackDRCBytes}' ${var:EndpointDrcBlob}"
+                                cset-tlv "name='${var:PostMixerAnalogPlaybackIIRBytes}' ${var:EndpointIirBlob}"
+                                cset-tlv "name='${var:PostMixerAnalogPlaybackFIRBytes}' ${var:EndpointFirBlob}"
+                                #shell "/bin/echo '${var:__endpoint} ${var:EndpointIirBlob} ${var:EndpointFirBlob} ${var:EndpointDrcBlob}' >> /tmp/alsa-ucm.txt"


is this command left intentionally @singalsu?

This looks like a debug statement. To raise the log level, uncomment it.

Is there a better logging solution?

I've commented it out because the log file write breaks UCM when the user logs in. When the shell command is enabled I need to "sudo chmod a+rw /tmp/alsa-ucm.txt" and restart ucm to get it working and logging the blob data file applies to DSP. A debug print to e.g. /var/log/syslog would be great but I haven't seen such in documentation.

plbossart · 2024-05-07T14:17:51Z

ucm2/Intel/sof-hda-dsp/HiFi-sof.conf

+                                EnableSequence [
+                                        cset-tlv "name='${var:PostMixerAnalogPlaybackIIRBytes}' ${var:EndpointIirBlob}"
+                                        cset-tlv "name='${var:PostMixerAnalogPlaybackFIRBytes}' ${var:EndpointFirBlob}"
+                                        #shell "/bin/echo '${var:__endpoint} ${var:EndpointIirBlob} ${var:EndpointFirBlob}' >> /tmp/alsa-ucm.txt"


same, why is this one commented out?

Yep, I was thinking to remove these after the script stabilizes. They have been a good help in debugging the failures after changes to script.

Add support for Digital Microphones DMIC01 on Qualcomm X1E80100 CRD board. Closes: alsa-project#414 Signed-off-by: Krzysztof Kozlowski <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>

The example set contains passthrough configuration blobs with SOF IPC3 and IPC4 headers for DRC, FIR, and IIR. A few high-pass configurations are added for IIR to be used e.g. for speakers. A DRC blob is added that can be used to boost speaker playback loudness. The blobs are all in binary format. Signed-off-by: Seppo Ingalsuo <[email protected]>

This example shows how to define IIR, FIR, and DRC processing for speaker and headphone endpoints. Signed-off-by: Seppo Ingalsuo <[email protected]>

This patch adds to Intel/sof-hda-dsp/HiFi.conf inclusion of HiFi-sof.conf that by redefine of headphone and speaker handling adds to UCM control of DRC and EQ SOF processing components. The modified setting are applied in case of SOF processing components' controls are detected. There is no change to operation if no controls are present e.g. with legacy SOF topology. If DRC control is found, it is assumed that also FIR and IIR also exist. If there is no DRC but FIR is found, then it is assumed that IIR also exists. This matches SOF FW builds for IPC3 (FIR, IIR) and IPC4 (DRC, FIR, IIR). The controls names are different in IPC3 and IPC4 topologies. Also the configuration blobs differ. The speaker mode is by default set up with 100 Hz high-pass IIR. The DRC is set to a default speaker setting that boosts playback loudness. The FIR is bypassed. In the headphone mode all the processing is set to bypass and DRC switch is set off. The processing can be customized for products with UCM scripts placed into blobs/sof/product_configs. The file path should be <sys_vendor>/<product_name>.conf from DMI ID. An user configuration can be similarly placed into blobs/sof/user_configs directory to e.g. avoid it being overwritten by distribution. Signed-off-by: Seppo Ingalsuo <[email protected]>

lgirdwood · 2024-09-13T15:39:44Z

@singalsu I guess this is now resolved upstream now, if so pls close. Thanks !

singalsu · 2024-09-13T16:04:09Z

@singalsu I guess this is now resolved upstream now, if so pls close. Thanks !

Yep!

singalsu requested review from perexg, plbossart, lgirdwood, jsarha, kv2019i and ujfalusi April 30, 2024 09:41

singalsu commented Apr 30, 2024

View reviewed changes

singalsu marked this pull request as ready for review April 30, 2024 09:49

singalsu requested a review from ranj063 April 30, 2024 09:49

ujfalusi reviewed Apr 30, 2024

View reviewed changes

plbossart reviewed Apr 30, 2024

View reviewed changes

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch from 2170562 to d05f574 Compare May 6, 2024 15:14

singalsu requested review from marc-hb, plbossart and ujfalusi May 6, 2024 15:15

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch 2 times, most recently from d0f710f to 2b2e831 Compare May 7, 2024 13:40

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch from 2b2e831 to 871446c Compare May 7, 2024 13:49

plbossart reviewed May 7, 2024

View reviewed changes

ucm2: Qualcomm: x1e80100: add recording via DMIC01

c281725

Add support for Digital Microphones DMIC01 on Qualcomm X1E80100 CRD board. Closes: alsa-project#414 Signed-off-by: Krzysztof Kozlowski <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch 2 times, most recently from 228ef05 to 83907e1 Compare May 22, 2024 07:07

marc-hb removed their request for review May 22, 2024 19:21

singalsu added 2 commits May 30, 2024 18:25

UCM2: SOF: Add example blobs customization for AAEON UPX-TGL01 board

eabdf9f

This example shows how to define IIR, FIR, and DRC processing for speaker and headphone endpoints. Signed-off-by: Seppo Ingalsuo <[email protected]>

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch from 83907e1 to 261ff87 Compare May 31, 2024 10:23

singalsu force-pushed the intel_sof_hda_dsp_add_processing_control branch from 261ff87 to a50f7d9 Compare May 31, 2024 12:20

singalsu closed this Sep 13, 2024

UCM2: Intel: sof-hda-dsp: Control SOF processing from UCM #49

UCM2: Intel: sof-hda-dsp: Control SOF processing from UCM #49

Conversation

singalsu commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented May 6, 2024

perexg commented May 6, 2024 • edited Loading

singalsu commented May 7, 2024

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Sep 13, 2024

singalsu commented Sep 13, 2024

plbossart May 2, 2024 •

edited

Loading

perexg commented May 6, 2024 •

edited

Loading