Unable to open hostless PCM device after introduction of commit - ASoC: Stop dummy from overriding hwparams

Unable to open hostless PCM device after introduction of commit - ASoC: Stop dummy from overriding hwparams

[Patrick Lai]

Hi Amadeusz,

On the product I am working on, a hostless PCM device is defined for 
purpose of activating CODEC driver to setup the path inside CODEC. So, 
CPU DAI and PCM Platform are defined to use dummy dai & DMA supplied by 
sound/soc/soc-utils.c.

After upgrading to newer kernel, hostless PCM device failed to open. 
After doing a bit of digging, the root cause is that dummy_dma_hardware 
is not set in dummy_dma_open() due to new conditional check logic 
introduced in this commit - 6c504663ba2ee2abeaf5622e27082819326c1bd4.

In order to fix problem I am encountering properly without regressing 
your scenario, I would like to get a better understanding of problem you 
were addressing. My understanding, from looking through other drivers 
under sound/soc, is that pcm hardware info is usually set by PCM 
platform/DMA drivers. For your scenario, do you have other component e.g 
CPU/CODEC DAI, set PCM hardware definition? I am not sure conditional 
check logic from 6c504663ba2ee2abeaf5622e27082819326c1bd4 guarantees 
that other component would be setting pcm hardware info. Appreciate if 
you can provide more insight to your scenario?

Thanks
Patrick

Re: Unable to open hostless PCM device after introduction of commit - ASoC: Stop dummy from overriding hwparams

[Amadeusz Sławiński]

On 11/18/2022 6:17 PM, Patrick Lai wrote:
> Hi Amadeusz,
> 
> On the product I am working on, a hostless PCM device is defined for 
> purpose of activating CODEC driver to setup the path inside CODEC. So, 
> CPU DAI and PCM Platform are defined to use dummy dai & DMA supplied by 
> sound/soc/soc-utils.c.
> 
> After upgrading to newer kernel, hostless PCM device failed to open. 
> After doing a bit of digging, the root cause is that dummy_dma_hardware 
> is not set in dummy_dma_open() due to new conditional check logic 
> introduced in this commit - 6c504663ba2ee2abeaf5622e27082819326c1bd4.
> 
> In order to fix problem I am encountering properly without regressing 
> your scenario, I would like to get a better understanding of problem you 
> were addressing. My understanding, from looking through other drivers 
> under sound/soc, is that pcm hardware info is usually set by PCM 
> platform/DMA drivers. For your scenario, do you have other component e.g 
> CPU/CODEC DAI, set PCM hardware definition? I am not sure conditional 
> check logic from 6c504663ba2ee2abeaf5622e27082819326c1bd4 guarantees 
> that other component would be setting pcm hardware info. Appreciate if 
> you can provide more insight to your scenario?
> 
> Thanks
> Patrick

Hi Patrick,

Call path is: ... -> __soc_pcm_open() -> soc_pcm_components_open -> 
snd_soc_component_open -> open callback, where for dummy device open 
callback is dummy_dma_open.

Expanding on the issue in question which was cause of the patch.

With following debug log:
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index e12f8244242b..b086ec05da25 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -290,6 +290,8 @@ int snd_soc_component_open(struct snd_soc_component 
*component,
  {
         int ret = 0;

+       pr_err("%s\n", component->name);
+
         if (component->driver->open)
                 ret = component->driver->open(component, substream);

that's what I get in dmesg on one of our test platforms:
[   95.522577] avs_rt274.1-platform
[   95.526019] i2c-INT34C2:00
[   95.528837] snd_soc_core:dpcm_fe_dai_startup:  audio: ASoC: open FE audio
[   95.528849] avs_rt274.1-platform
[   95.532249] snd-soc-dummy
[   95.534989] snd-soc-dummy
[   95.537800] snd_soc_avs:avs_dai_fe_startup: snd_soc_avs 0000:00:1f.3: 
avs_dai_fe_startup fe STARTUP tag 1 str 0000000064defd29

"avs_rt274.1-platform" component is handled in sound/soc/intel/avs/pcm.c 
it calls avs_component_open() which sets hwparams to generic set 
supported by i2s devices in AVS driver.

"i2c-INT34C2:00" is codec driver sound/soc/codecs/rt274.c it does not 
have open callback.

And finally "snd-soc-dummy" which as mentioned above calls 
dummy_dma_open which originally overridden hwparams set in 
avs_component_open() with its own limited ones.

(When topology is loaded it also creates FEs, which further limit 
allowed hwparams, they are a subset of the ones set above).

As mentioned in the patch:
"Alternative approach would be to copy whole dummy handling and rename 
it to "snd-soc-null" or something similar. And remove hwparams 
assignment to make it really do nothing."

However, looking at it again, I would consider the existence of 
dummy_dma_open() to be scope creep. If component is really a dummy one 
it should not affect any other components in any way. And if any drivers 
depends on dummy setting parameters for it, I would consider it 
partially broken. And would say that issue should rather be fixed on 
driver side by making a dedicated component for it instead of using a 
dummy one.

I hope that I cleared up situation a bit.

Thanks,
Amadeusz

Re: Unable to open hostless PCM device after introduction of commit - ASoC: Stop dummy from overriding hwparams

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On Mon, Nov 21, 2022 at 04:29:19PM +0100, Amadeusz Sławiński wrote:

> And finally "snd-soc-dummy" which as mentioned above calls dummy_dma_open
> which originally overridden hwparams set in avs_component_open() with its
> own limited ones.

...

> However, looking at it again, I would consider the existence of
> dummy_dma_open() to be scope creep. If component is really a dummy one it
> should not affect any other components in any way. And if any drivers
> depends on dummy setting parameters for it, I would consider it partially
> broken. And would say that issue should rather be fixed on driver side by
> making a dedicated component for it instead of using a dummy one.

That's not such a clear statement in this case.  The thing with the
hostless links Qualcomm are trying to use here is that we don't actually
want to do any DMA ops at all, these paths never go to memory.  Really
they're CODEC to CODEC links but done in an older way.  There's only DMA
ops there because we need something to keep userspace happy, they don't
actually mean anything and we'd never want them to be used if anything
else is providing actual parameters.

It would be cleaner if these systems were to use a CODEC to CODEC link
but I'm not sure how well that plays with DPCM - the whole thing is a
bit of a house of cards there.

In any case, any news on a fix for this?  The suggestion of a
custom/variant version of the dummy component that explicitly flags that
it's supposed to be used in a hostless configuration does seem like the
least fragile thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]