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

ujfalusi commented 6 months ago

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: https://github.com/thesofproject/linux/pull/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

Checking the iec61937-pcm indexes against the expected HDMI devices (sof-dsp-hda: 3-5, sof-soundwire: 5-7)
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 commented 6 months ago

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 :thinking:

perexg commented 6 months ago

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 commented 6 months ago

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.

https://github.com/alsa-project/alsa-lib/pull/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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@perexg, I think I got it working! :tada: 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 commented 6 months ago

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 commented 6 months ago

Changes since v3:

Debugprints removed
hdmi_iec61937 definition fixed

ujfalusi commented 6 months ago

Changes since v4:

Use FixedBootSequence in HdmiPCMSave macro

ujfalusi commented 6 months ago

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 commented 6 months ago

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

ujfalusi commented 6 months ago

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

No, this PR is right, the reason why it is broken is: c56d0a46c621 ("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 commented 6 months ago

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 035d9206cffdf7942352d2daf3c34dde491c01dc HEAD.

ujfalusi commented 6 months ago

Changes since v6:

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

ujfalusi commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

alsa-project / alsa-ucm-conf

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