sof-hda-dsp/sof-soundwire: Create "hdmi:" mapping PCMs to allow passthrough if supported

[ujfalusi]

Hi,

SOF with IPC4 can use 'ChainDMA' on selected PCMs (HDMI/DP currently) which allows them to be used for bytestream passthrough since the data is passed through unmodified.
The kernel will list the PCMs with ChainDMA to the card's components list:
thesofproject/linux#4921
For example for sof-dsp-hda cards iec61937-pcm:5,4,3 will be added.

For user space to use HDMI passthrough, the hdmi: PCM device should be present correctly mapping to the machine, for example for sof-dsp-hda:

aplay -L | grep hdmi

hdmi:CARD=sofhdadsp,DEV=0
hdmi:CARD=sofhdadsp,DEV=1
hdmi:CARD=sofhdadsp,DEV=2

This PR does this by

1. Checking the iec61937-pcm indexes against the expected HDMI devices (sof-dsp-hda: 3-5, sof-soundwire: 5-7)
2. If there is a match (passthrough can be supported) then we will create three conf files:

The generated files are:
[1] /var/lib/alsa/conf.d/42-sof-hdmi.conf
[2] /var/lib/alsa/card[card_number].conf.d/30-sof-hdmi-common.conf
[3] /var/lib/alsa/card[card_number].conf.d/31-sof-hdmi.conf

[1] includes the pcm/iec958.conf and pcm/hdmi.conf to global space of
alsaconf to be used by the card macros
[2] Card specific macros for hdmi PCM definition, mapping
[3] Card specific definitions of the three HDMI port

Note for [1]: I needed to use shell "echo '... since the cfg-save would expand the includes into the saved config file and that just does not result a working alsa configuration.

@perexg, I'm sure all this can be done in a cleaner way... We cannot do this unconditionally as if the HDMI is not using ChainDMA (and SOF is not IPC4) then the passthrough is not possible since the firmware might touch the data.

[ujfalusi]

Changes since v1:

• Fix indentation (spaces -> tabs) in ucm2/sof-soundwire/sof-soundwire.conf
• ucm.py reports the same errors now as it reports for main on my machine.
  • hrm, but locally it fails differently than what CI reports 🤔

[perexg]

Thanks. Some notes:

• Creating of /var/lib/alsa/conf.d/42-sof-hdmi.conf should be avoided, UCM should create only card specific files. We can include unconditionally hdmi.conf in aliases.conf for now, until we have a better way. The iec958 device has a bit different characteristics, so this does not need to be included. Would you like to submit an one-line patch for alsa-lib?

• This is a serious candidate to use UCM macros here (for the one PCM definition). The common PCM can be removed (it's not required to have two stage PCM - it was a try to clean the previous configuration scheme). Also single PCMs should be created from sof-soundwire.conf and sof-hda-dsp.conf. The bonus is, that in this UCM config files, we know the PCM characteristics (PCM device, CTL index), so it will result in cleaner config. Only two macros are required - one for PCM configuration, second to save the final config. See split.conf.

• We should probably also pass a flag to user space app if the UCM device supports pass-through (UCM API extension is required for Device.Hdmi).

The config in sof-hda-dsp.conf may be like:

Macro [
    { 
        HdmiPCM { Index 0 Device 3 }
        HdmiPCM { Index 1 Device 4 }
        HdmiPCM { Index 2 Device 5 }
        HdmiPCMSave { }
    }
]

[ujfalusi]

Thanks. Some notes:

* Creating of `/var/lib/alsa/conf.d/42-sof-hdmi.conf` should be avoided, UCM should create only card specific files. We can include unconditionally `hdmi.conf` in `aliases.conf` for now, until we have a better way. The iec958 device has a bit different characteristics, so this does not need to be included. Would you like to submit an one-line patch for alsa-lib?

OK, I have this working without the iec958 inclusion, I must have added it back by mistake.

alsa-project/alsa-lib#393

* This is a serious candidate to use UCM macros here (for the one PCM definition). The common PCM can be removed (it's not required to have two stage PCM - it was a try to clean the previous configuration scheme). Also single PCMs should be created from `sof-soundwire.conf` and `sof-hda-dsp.conf`. The bonus is, that in this UCM config files, we know the PCM characteristics (PCM device, CTL index), so it will result in cleaner config. Only two macros are required - one for PCM configuration, second to save the final config. See [split.conf](https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/common/pcm/split.conf).

Right, I'm not sure how that is done in practice. I could not find a way to save a generated section, not to mention how to generate that section which can then be saved.
So, we should have a single generated config (/var/lib/alsa/card0.conf.d/42-sof-hdmi.conf for example) with all the hdmi device mapping 'code' or separate conf files for each HDMI?

I guess one of the generated alsaconf section in the split.conf is LibraryConfig.pcm.SubstiConfig.pcm."${var:__Name}" and the SplitPCMDevice is referring to the generated config?

* We should probably also pass a flag to user space app if the UCM device supports pass-through (UCM API extension is required for `Device.Hdmi`).

I see, I would guess that this needs alignment with Pulseaudio and Pipewire at least?

The config in sof-hda-dsp.conf may be like:

Macro [
    { 
        HdmiPCM { Index 0 Device 3 }
        HdmiPCM { Index 1 Device 4 }
        HdmiPCM { Index 2 Device 5 }
        HdmiPCMSave { }
    }
]

OK, let me see how far I will get with this ;)

Thank you for the suggestions!

[perexg]

So, we should have a single generated config (/var/lib/alsa/card0.conf.d/42-sof-hdmi.conf for example) with all the hdmi device mapping 'code' or separate conf files for each HDMI?

One file should be enough. Just save the appropriate generated subtree:

FixedBootSequence [
	cfg-save "${var:LibDir}/sof-hdmi.conf:sof-hdmi"
]

(assuming PCM configs like sof-hdmi.pcm.hdmi.0)

The command alsactl dump-cfg is very useful to check the current configuration tree (including changes in /var). Your hdmi config should be in cards.0.pcm.hdmi subtree.

I guess one of the generated alsaconf section in the split.conf is LibraryConfig.pcm.SubstiConfig.pcm."${var:__Name}" and the SplitPCMDevice is referring to the generated config?

Not really. Use only SplitPCM macro for an example. SplitPCMDevice is a helper which uses different UCM config for applications implementing channel splitting.

[ujfalusi]

@perexg, it almost works, but not the way I expected..
in ucm2/Intel/sof-hda-dsp/sof-hda-dsp.conf

Include.sof-hdmi.File "/lib/sof-hdmi2.conf"

Macro [
  {	
	HdmiPCM { Device 3 Index 0 }
	HdmiPCM { Device 4 Index 1 }
	HdmiPCM { Device 5 Index 2 }
	HdmiPCMSave { }
  }	
]

The /lib/sof-hdmi2.conf files is:

DefineMacro.HdmiPCM.If.iec61937 {
	Condition {
		Type RegexMatch
		Regex "([${var:__Device}](,|$))"
		String "${var:Iec61937Pcm1}"
	}
	True {
		LibraryConfig.generic.Config {

			sof-hdmi.pcm.hdmi."${var:__Index}" {
				@args [ CARD AES0 AES1 AES2 AES3 ]
				@args.CARD {
					type string
				}
				@args.AES0 {
					type integer
				}
				@args.AES1 {
					type integer
				}
				@args.AES2 {
					type integer
				}
				@args.AES3 {
					type integer
				}
				type hooks
				slave.pcm {
					type hw
					card $CARD
					device "${evali:$__Device}"
				}
				hooks.0 {
					type ctl_elems
					hook_args [
					{
						name "IEC958 Playback Default"
						index "${evali:$__Index}"
						lock true
						preserve true
						value [ $AES0 $AES1 $AES2 $AES3 ]
					}
					{
						name "IEC958 Playback Switch"
						index "${evali:$__Index}"
						value true
					}
					]
				}
				hint.device "${evali:$__Device}"
			}
		}
	}
	True.BootSequence [
			shell "echo 'True: ${var:__Device}:${var:__Index}' >> /tmp/alsa-ucm.txt"
	]

	False.BootSequence [
			shell "echo 'False: ${var:__Device}:${var:__Index}' >> /tmp/alsa-ucm.txt"
	]
}

DefineMacro.HdmiPCMSave.If.0 {
	Condition { Type AlwaysTrue }
	True.BootSequence [
			shell "echo 'Save' >> /tmp/alsa-ucm.txt"
			cfg-save "${var:LibDir}/42-sof-hdmi.conf:sof-hdmi"
	]
}

It creates one file with a single pcm.hdmi.X where X is the value from the last macro call. If I drop the Device 4 and 5, I have the mapping for Device 3, if I only drop the Device 5 then it is the Device 4 only.

But still, only the section generated by the last call to the macro is present.
Tried also to wrap the macro calls into { }, did not helped either.

In /tmp/alsa-ucm.txt I only have prints from the last call of the macro + Save

[perexg]

Replace:

  LibraryConfig.generic.Config {
 			sof-hdmi.pcm.hdmi."${var:__Index}" {

with shorter notation (less tabs) and correct use SubstiConfig (substituted configuration tree - not just unevaluated copy):

  True.LibraryConfig.generic.SubstiConfig.sof-hdmi.pcm.hdmi."${var:__Index}" {
  }

[perexg]

Also, the RegEx is too much specific for your use. I would like to move regex to the caller (call macros from this condition) - outside /lib file.

[ujfalusi]

@perexg, I think I got it working! 🎉
But before updating the PR, I think you want this to be more generic, not tied to SOF, right?
If that is the case that the /lib/sof-hdmi.conf and the use of sof-hdmi in it is not correct, neither the generated file's name of 42-sof-hdmi.conf.
What paths, names do you see fit for the purpose? Or should we add parameter to the HdmiPCMSave macro as filename (if so, with or without the leading number, do we even want a leading number at all?)?

This is what I have now:
[1] in sof-hda-dsp.conf :

Include.sof-hdmi.File "/lib/sof-hdmi2.conf"

If.Hdmi3-iec61937 {
	Condition {
		Type RegexMatch
		Regex "([3](,|$))"
		String "${var:Iec61937Pcm1}"
	}
	True {
		Macro.hdmi3.HdmiPCM { Device 3 Index 0 }
		Define.hdmi_iec61937 "yes"
	}
}

If.Hdmi4-iec61937 {
	Condition {
		Type RegexMatch
		Regex "([4](,|$))"
		String "${var:Iec61937Pcm1}"
	}
	True {
		Macro.hdmi4.HdmiPCM { Device 4 Index 1 }
		Define.hdmi_iec61937 "yes"
	}
}

If.Hdmi5-iec61937 {
	Condition {
		Type RegexMatch
		Regex "([5](,|$))"
		String "${var:Iec61937Pcm1}"
	}
	True {
		Macro.hdmi5.HdmiPCM { Device 5 Index 2 }
		Define.hdmi_iec61937 "yes"
	}
}

If.HdmiIec61937 {
	Condition {
		Type String
		Empty "${var:hdmi_iec61937}"
	}
	False {
		Macro.save_hdmi_cfg.HdmiPCMSave { }
	}
}

[2] the /lib/sof-hdmi2.conf :

DefineMacro.HdmiPCM {
	LibraryConfig.generic.Config.sof-hdmi.pcm.hdmi."${var:__Index}" {
		@args [ CARD AES0 AES1 AES2 AES3 ]
		@args.CARD {
			type string
		}
		@args.AES0 {
			type integer
		}
		@args.AES1 {
			type integer
		}
		@args.AES2 {
			type integer
		}
		@args.AES3 {
			type integer
		}
		type hooks
		slave.pcm {
			type hw
			card $CARD
			device "${evali:$__Device}"
		}
		hooks.0 {
			type ctl_elems
			hook_args [
			{
				name "IEC958 Playback Default"
				index "${evali:$__Index}"
				lock true
				preserve true
				value [ $AES0 $AES1 $AES2 $AES3 ]
			}
			{
				name "IEC958 Playback Switch"
				index "${evali:$__Index}"
				value true
			}
			]
		}
		hint.device "${evali:$__Device}"
	}
	BootSequence [
			shell "echo 'True: ${var:__Device}:${var:__Index}' >> /tmp/alsa-ucm.txt"
	]

}

DefineMacro.HdmiPCMSave {
	BootSequence [
			shell "echo 'Save' >> /tmp/alsa-ucm.txt"
			cfg-save "${var:LibDir}/42-sof-hdmi.conf:sof-hdmi"
	]
}

The /var/lib/alsa/card0.conf.d/42-sof-hdmi.conf contains the definitions for the hdmi PCM devices correctly and

# cat /tmp/alsa-ucm.txt 
True: 3:0
True: 4:1
True: 5:2
Save

[ujfalusi]

Changes since v2:

• Converted to use Macros as suggested by @perexg:
• ucm2/lib/common/pcm/hdmi.conf HdmiPCM and HdmiPCMSave
• sof-hda-dsp and sof-soundwire to use this new way

[ujfalusi]

debugprints :(

[ujfalusi]

Changes since v3:

• Debugprints removed
• hdmi_iec61937 definition fixed

[perexg]

Thanks. Good work. Please, check my latest comments.

[ujfalusi]

Changes since v4:

• Use FixedBootSequence in HdmiPCMSave macro

[perexg]

Probably last thing I forgot to note in the first review - can we use RegexMatch here using the ${var:Iec61937Pcms1} string instead a temporary variable?

Like Regex "((^|,)[345](,|$))" ?

It saves some lines.

[ujfalusi]

Changes since v5:

• Use Iec61937Pcms1 directly for condition to save the hdmi config
• collapse True cases into one line now.
• and fix copy-paste for the condition in sof-soundwire :(

[ujfalusi]

Right, sof-soundwire stopped working correctly with the last update

[ujfalusi]

Right, sof-soundwire stopped working correctly with the last update

No, this PR is right, the reason why it is broken is: c56d0a4 ("sof-soundwire: Add basic support for cs42l43's speaker")
I have rebased the PR to make sure there are no conflicts.

# alsaucm -c hw:0 dump text
ALSA lib ucm_subs.c:807:(uc_mgr_get_substituted_value) variable '${var:Codec1}' is not defined in this context!
ALSA lib main.c:1554:(snd_use_case_mgr_open) error: failed to import hw:0 use case configuration -22
alsaucm: error failed to open sound card hw:0: Invalid argument

[perexg]

Right, sof-soundwire stopped working correctly with the last update

No, this PR is right, the reason why it is broken is: c56d0a4 ("sof-soundwire: Add basic support for cs42l43's speaker") I have rebased the PR to make sure there are no conflicts.

# alsaucm -c hw:0 dump text
ALSA lib ucm_subs.c:807:(uc_mgr_get_substituted_value) variable '${var:Codec1}' is not defined in this context!
ALSA lib main.c:1554:(snd_use_case_mgr_open) error: failed to import hw:0 use case configuration -22
alsaucm: error failed to open sound card hw:0: Invalid argument

Sorry. The various changes were merged together by mistake. Use new 035d920 HEAD.

[ujfalusi]

Changes since v6:

• Rebased and verified to be working on sof-soundwire and sof-hda-dsp alike.

[ujfalusi]

@perexg, can I do the update to 'Syntax 7' when the support is released with alsa-lib?
We often recommend users to update their UCM to git version but Syntax 7 would need git version of alsa-lib, which is more problematic to recommend for users.

[perexg]

@perexg, can I do the update to 'Syntax 7' when the support is released with alsa-lib? We often recommend users to update their UCM to git version but Syntax 7 would need git version of alsa-lib, which is more problematic to recommend for users.

I applied the current work. It looks fine for me.

For Syntax 7 - could you create a separate PR, please? I'll apply it after new alsa-lib release so this will cause less issues. Thank you.

[ujfalusi]

@perexg, does this makes sense for the Syntax 7 update:

diff --git a/ucm2/Intel/sof-hda-dsp/sof-hda-dsp.conf b/ucm2/Intel/sof-hda-dsp/sof-hda-dsp.conf
index 00e40d690aee..0604b09c049e 100644
--- a/ucm2/Intel/sof-hda-dsp/sof-hda-dsp.conf
+++ b/ucm2/Intel/sof-hda-dsp/sof-hda-dsp.conf
@@ -1,4 +1,4 @@
-Syntax 6
+Syntax 7
 
 Include.card-init.File "/lib/card-init.conf"
 
@@ -128,32 +128,9 @@ If.Capture {
 
 Include.hdmi-pcm.File "/common/pcm/hdmi.conf"
 
-If.Hdmi3-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[3](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi3.HdmiPCM { Device 3 Index 0 }
-}
-
-If.Hdmi4-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[4](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi4.HdmiPCM { Device 4 Index 1 }
-}
-
-If.Hdmi5-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[5](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi5.HdmiPCM { Device 5 Index 2 }
-}
+Macro.0.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 3 Idx 0 }
+Macro.1.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 4 Idx 1 }
+Macro.2.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 5 Idx 2 }
 
 If.HdmiIec61937 {
 	Condition {
diff --git a/ucm2/common/pcm/hdmi.conf b/ucm2/common/pcm/hdmi.conf
index 0a870edba54f..1a52986b7a0d 100644
--- a/ucm2/common/pcm/hdmi.conf
+++ b/ucm2/common/pcm/hdmi.conf
@@ -1,3 +1,5 @@
+Syntax 7
+
 # Macro HdmiPCM - Generate ALSA control section for hdmi: PCM device
 #
 # Arguments:
@@ -72,3 +74,22 @@ DefineMacro.HdmiPCMSave {
 			cfg-save "${var:LibDir}/${var:__Name}.conf:hdmi-pcm"
 	]
 }
+
+# Macro SofHdmiDevice - Create hdmi PCM device for a SOF sound card if
+#			passthrough is supported
+#
+# Arguments:
+#   SofIec61937Pcms - Comma separated list of PCM device indexes (from
+#		      iec61937-pcm: components tag)
+#   Dev - hardware PCM device to match in SofIec61937Pcms
+#   Idx - hdmi: device index and control index
+#
+
+DefineMacro.SofHdmiDevice.If.iec61937 {
+	Condition {
+		Type RegexMatch
+		Regex "((^|,)[${var:__Dev}](,|$))"
+		String "${var:__SofIec61937Pcms}"
+	}
+	True.Macro.hdmi.HdmiPCM { Device "${var:__Dev}" Index "${var:__Idx}" }
+}
diff --git a/ucm2/sof-soundwire/sof-soundwire.conf b/ucm2/sof-soundwire/sof-soundwire.conf
index 34ceb64c98cf..d110a869ac69 100644
--- a/ucm2/sof-soundwire/sof-soundwire.conf
+++ b/ucm2/sof-soundwire/sof-soundwire.conf
@@ -1,4 +1,4 @@
-Syntax 6
+Syntax 7
 
 SectionUseCase."HiFi" {
 	File "/sof-soundwire/HiFi.conf"
@@ -111,32 +111,9 @@ If.mics-array {
 
 Include.hdmi-pcm.File "/common/pcm/hdmi.conf"
 
-If.Hdmi5-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[5](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi5.HdmiPCM { Device 5 Index 0 }
-}
-
-If.Hdmi6-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[6](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi6.HdmiPCM { Device 6 Index 1 }
-}
-
-If.Hdmi7-iec61937 {
-	Condition {
-		Type RegexMatch
-		Regex "((^|,)[7](,|$))"
-		String "${var:Iec61937Pcms1}"
-	}
-	True.Macro.hdmi7.HdmiPCM { Device 7 Index 2 }
-}
+Macro.0.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 5 Idx 0 }
+Macro.1.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 6 Idx 1 }
+Macro.2.SofHdmiDevice { SofIec61937Pcms "${var:Iec61937Pcms1}" Dev 7 Idx 2 }
 
 If.HdmiIec61937 {
 	Condition {

I think this can even be simplified a bit further by extending the SofHdmiDevice macro, but then the macro will be quite huge:
sof-hda-dsp: Macro.0.SofHdmiDevices { SofIec61937Pcms "${var:Iec61937Pcms1}" HdmiPcms "3 4 5" }
sof-soundwire: Macro.0.SofHdmiDevices { SofIec61937Pcms "${var:Iec61937Pcms1}" HdmiPcms "5 6 7" }

Then we can have DefineRegex in the pcm/hdmi.conf to extract the IDs to SofHdmi (\d{1,3}) then in the macro we could use SofHdmi1/2/3 (SofHdmi1 is Index 0, SofHdmi2 is Index 1, SofHdmi3 is Index 2).
It might work, but the macro will be longer and not sure how readable things will be

[perexg]

It looks nice. I would just rename SofIec61937Pcms macro argument to something like DetectedPcms or so. It's a general macro and we can eventually add more delimiters (like spaces etc.) in future on demand. Also SofHdmiDevice should be HdmiDevice (proposal - generic name).

[perexg]

Then we can have DefineRegex in the pcm/hdmi.conf to extract the IDs to SofHdmi (\d{1,3}) then in the macro we could use SofHdmi1/2/3 (SofHdmi1 is Index 0, SofHdmi2 is Index 1, SofHdmi3 is Index 2). It might work, but the macro will be longer and not sure how readable things will be

Yes, the current proposal seems more readable (one line to create one device).

[ujfalusi]

It looks nice. I would just rename SofIec61937Pcms macro argument to something like DetectedPcms or so. It's a general macro and we can eventually add more delimiters (like spaces etc.) in future on demand. Also SofHdmiDevice should be HdmiDevice (proposal - generic name).

Right, I have kept the 'Sof' namespacing as the SOF provides the iec61937-pcm: in the component list (amixer -c0 info), this list of device ids can (probably it will) include non HDMI PCMs capable of passthrough (real S/PDIF), so that list is kind of SOF specific.

Let me think how to document it if it is converted to a generic macro.