Ambiguous DAPM widget names and DPCM

Ambiguous DAPM widget names and DPCM

[Daniel Mack]

Hi Mark,
Hi Liam,

I spent some hours reverse-engineering the DPCM code, and I plan to
prepare a patch to add some documentation once I've assembled a fully
working setup. However, I have a general question regarding DAPM widget
names.

DPCM uses DAPM widgets and routes in order to determine the FE <-> BE
connections at runtime. The problem with this is that widget names are
very ambiguous, as the automatically created widgets for DAIs are named
after their stream_name properties, which contain "Playback" or
"Capture" for all components in my case. Hence, the code which walks the
connections (snd_soc_dapm_dai_get_connected_widgets()) will most likely
match the wrong one (which has empty sound/sink lists), which results in
a failure like this:

[   16.606268]  fe: ASoC: fe no valid playback route

I hacked the names of all components so they are unique, but even then,
the problem is that snd_soc_dapm_new_dai_widgets() is called twice for
the CPU DAI, once from soc_probe_platform(), and later from
soc_probe_link_dais(), and both calls would create DAPM widgets with
identical names, resulting in the same problem as described above.

I don't know how to handle this, but I believe that nobody was really
hit by this issue yet, as DPCM doesn't seem to be used widely, at least
not by any machine code in mainline.

One idea would be to change the automatically generated names of DAI
widgets in order to make them unique, or re-factor the lookup routines.

Any opinion on that? Do I miss a general consideration here?

With more hacks in place, the setup somehow succeeds, but when the
stream is opened, we'll hit a BUG() in sound/core/pcm_memory.c, because
the FE device's DMA type is undefined. But that's another topic I'll
address later.


Thanks,
Daniel

Re: Ambiguous DAPM widget names and DPCM

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1408 bytes --]

On Tue, Jul 23, 2013 at 10:30:16AM +0200, Daniel Mack wrote:

> DPCM uses DAPM widgets and routes in order to determine the FE <-> BE
> connections at runtime. The problem with this is that widget names are
> very ambiguous, as the automatically created widgets for DAIs are named
> after their stream_name properties, which contain "Playback" or
> "Capture" for all components in my case. Hence, the code which walks the

Could you go into more detail about the ambiguity you see here?

> I hacked the names of all components so they are unique, but even then,

This is obviously required, yes - just as with everything else you
shouldn't use the same name twice.

> the problem is that snd_soc_dapm_new_dai_widgets() is called twice for
> the CPU DAI, once from soc_probe_platform(), and later from
> soc_probe_link_dais(), and both calls would create DAPM widgets with
> identical names, resulting in the same problem as described above.

The theory here is that the CPUs should be converted into components and
then have the DAIs instantiated at component probe time - the creation
at link time is there as a crutch for older drivers.

> I don't know how to handle this, but I believe that nobody was really
> hit by this issue yet, as DPCM doesn't seem to be used widely, at least
> not by any machine code in mainline.

Yes, there's zero use in mainline.  I don't think this code has ever
been tested.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Ambiguous DAPM widget names and DPCM

[Daniel Mack]

Hi Mark,

On 23.07.2013 14:30, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 10:30:16AM +0200, Daniel Mack wrote:
> 
>> DPCM uses DAPM widgets and routes in order to determine the FE <-> BE
>> connections at runtime. The problem with this is that widget names are
>> very ambiguous, as the automatically created widgets for DAIs are named
>> after their stream_name properties, which contain "Playback" or
>> "Capture" for all components in my case. Hence, the code which walks the
> 
> Could you go into more detail about the ambiguity you see here?

I have a platform which has two codecs (cs4271, ak4101) connected to one
McASP instance. For DPCM, the link structure looks like this:

  mcasp <-> snd-soc-dummy, .dynamic = 1
  mcasp <-> cs4271, .no_pcm = 1
  mcasp <-> ak4101, .no_pcm = 1

With printk() in snd_soc_dapm_new_control(), I see the following widgets
being created during boot (one pair for each Codec, and two pairs for
the platform dai):

[    1.963501]  snd_soc_dapm_new_control() ceb6b680 name Playback
[    1.971739]  snd_soc_dapm_new_control() ceb6b5c0 name Capture
[    1.996001]  snd_soc_dapm_new_control() ceb6b500 name Playback
[    2.004354]  snd_soc_dapm_new_control() ceb6b440 name Capture
[    2.019684]  snd_soc_dapm_new_control() ceb6b380 name Playback
[    2.028053]  snd_soc_dapm_new_control() ceb6b2c0 name Capture
[    2.047526]  snd_soc_dapm_new_control() ceb6b200 name Playback
[    2.055846]  snd_soc_dapm_new_control() ceb6b140 name Capture
[    2.071168]  snd_soc_dapm_new_control() ceb6b080 name Playback
[    2.079461]  snd_soc_dapm_new_control() ceb8ff00 name Capture

And the widgets referenced by the audio map will hence have to be named
"Playback" or "Capture" as well, which has no chance to refer to the
correct one, right?

>> I hacked the names of all components so they are unique, but even then,
> 
> This is obviously required, yes - just as with everything else you
> shouldn't use the same name twice.

Yeah, but snd_soc_dapm_new_dai_widgets() relies on
dai->driver->playback.stream_name and takes that as widget name. So is
this approach faulty, and the automatically generated name would need to
be based on dev_name(dai->dev)? Or do we need to clean up all drivers in
order to make their streams unique? Then again, the question is how to
deal with multiple instances of the same dai driver, and how to tell
them apart in the audio map.

>> the problem is that snd_soc_dapm_new_dai_widgets() is called twice for
>> the CPU DAI, once from soc_probe_platform(), and later from
>> soc_probe_link_dais(), and both calls would create DAPM widgets with
>> identical names, resulting in the same problem as described above.
> 
> The theory here is that the CPUs should be converted into components and
> then have the DAIs instantiated at component probe time - the creation
> at link time is there as a crutch for older drivers.

Which older driver would rely on that, and can we safely remove it?

>> I don't know how to handle this, but I believe that nobody was really
>> hit by this issue yet, as DPCM doesn't seem to be used widely, at least
>> not by any machine code in mainline.
> 
> Yes, there's zero use in mainline.  I don't think this code has ever
> been tested.

No problem. I think it's a really essential feature which just needs
some fixups and explanation ...


Daniel

Re: Ambiguous DAPM widget names and DPCM

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2344 bytes --]

On Tue, Jul 23, 2013 at 05:34:59PM +0200, Daniel Mack wrote:
> On 23.07.2013 14:30, Mark Brown wrote:

> > Could you go into more detail about the ambiguity you see here?

> I have a platform which has two codecs (cs4271, ak4101) connected to one
> McASP instance. For DPCM, the link structure looks like this:

>   mcasp <-> snd-soc-dummy, .dynamic = 1
>   mcasp <-> cs4271, .no_pcm = 1
>   mcasp <-> ak4101, .no_pcm = 1

> With printk() in snd_soc_dapm_new_control(), I see the following widgets
> being created during boot (one pair for each Codec, and two pairs for
> the platform dai):

OK, so the issue here is probably that you're overloading the single
McASP object rather than anything else - that's always been a bit of a
hack and I suspect it's going to be causing issues again now.  In which
case we probably do need to bite the bullet on making multi-drop DAIs
work.  But let's see...

> And the widgets referenced by the audio map will hence have to be named
> "Playback" or "Capture" as well, which has no chance to refer to the
> correct one, right?

Lookups are done first in the local DAPM context so so long as the thing
that's setting up the links is doing so in the same context everything
should be OK.  This is needed so you can have two of the same device in
a system.

> > This is obviously required, yes - just as with everything else you
> > shouldn't use the same name twice.

> Yeah, but snd_soc_dapm_new_dai_widgets() relies on
> dai->driver->playback.stream_name and takes that as widget name. So is
> this approach faulty, and the automatically generated name would need to
> be based on dev_name(dai->dev)? Or do we need to clean up all drivers in
> order to make their streams unique? Then again, the question is how to
> deal with multiple instances of the same dai driver, and how to tell
> them apart in the audio map.

The prefix mechanism is intended to address this; however it's only
currently supported for CODECs.  If we move the prefixes to components
then they should be usable for CPU DAIs.

> > The theory here is that the CPUs should be converted into components and
> > then have the DAIs instantiated at component probe time - the creation
> > at link time is there as a crutch for older drivers.

> Which older driver would rely on that, and can we safely remove it?

Probably none right now.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Ambiguous DAPM widget names and DPCM

[Daniel Mack]

On 23.07.2013 20:30, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 05:34:59PM +0200, Daniel Mack wrote:

>> With printk() in snd_soc_dapm_new_control(), I see the following widgets
>> being created during boot (one pair for each Codec, and two pairs for
>> the platform dai):
> 
> OK, so the issue here is probably that you're overloading the single
> McASP object rather than anything else

Hmm, I'm not following. Isn't that the way things are supposed to work
with DPCM?

> Lookups are done first in the local DAPM context so so long as the thing
> that's setting up the links is doing so in the same context everything
> should be OK.  This is needed so you can have two of the same device in
> a system.

Hmm, but DPCM does not have such a local context, and just tries to look
up the widgets by name globally.

> The prefix mechanism is intended to address this; however it's only
> currently supported for CODECs.  If we move the prefixes to components
> then they should be usable for CPU DAIs.

But that's a static string as well. How can we distiguish between two
Codecs of the same type, or two McASP instances?

All of that might not have been a big issue so far maybe, but with DPMC,
we need to make the names unique, and add a way to uniquely identify
each widget along with its device identifier.

Liam, could you explain what you think makes sense here?


Thanks,
Daniel

Re: Ambiguous DAPM widget names and DPCM

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1350 bytes --]

On Thu, Jul 25, 2013 at 02:24:54PM +0200, Daniel Mack wrote:
> On 23.07.2013 20:30, Mark Brown wrote:
> > On Tue, Jul 23, 2013 at 05:34:59PM +0200, Daniel Mack wrote:

> > OK, so the issue here is probably that you're overloading the single
> > McASP object rather than anything else

> Hmm, I'm not following. Isn't that the way things are supposed to work
> with DPCM?

Not normally - normally there's a single CODEC on each external
interface with multiple front ends connected to it.

> > Lookups are done first in the local DAPM context so so long as the thing
> > that's setting up the links is doing so in the same context everything
> > should be OK.  This is needed so you can have two of the same device in
> > a system.

> Hmm, but DPCM does not have such a local context, and just tries to look
> up the widgets by name globally.

OK, so this is probably something that needs to be looked at.

> > The prefix mechanism is intended to address this; however it's only
> > currently supported for CODECs.  If we move the prefixes to components
> > then they should be usable for CPU DAIs.

> But that's a static string as well. How can we distiguish between two
> Codecs of the same type, or two McASP instances?

The point is to give a different prefix to each device; giving the same
prefix to different devices rather defeats the poinnt.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Ambiguous DAPM widget names and DPCM

[Liam Girdwood]

On Thu, 2013-07-25 at 13:48 +0100, Mark Brown wrote:
> On Thu, Jul 25, 2013 at 02:24:54PM +0200, Daniel Mack wrote:
> > On 23.07.2013 20:30, Mark Brown wrote:

> 
> > > The prefix mechanism is intended to address this; however it's only
> > > currently supported for CODECs.  If we move the prefixes to components
> > > then they should be usable for CPU DAIs.
> 
> > But that's a static string as well. How can we distiguish between two
> > Codecs of the same type, or two McASP instances?
> 
> The point is to give a different prefix to each device; giving the same
> prefix to different devices rather defeats the poinnt.

I don't think we had any issues with DAI global naming on OMAP4 (we were
probably just lucky I guess). We had several physical McBSP and several
logical McPDM interfaces that were used by the ABE and they were
globally connected in the machine driver using the DAI device name e.g.
"omap-mcbsp.1"

Fwiw, I've just looked at Peter's topic/ti-audio-next-bnw-wip branch at
gitorious.org:omap-audio/linux-audio.git and can see we may be missing a
couple of patches that can affect the DAI connections i.e.
35f485c300d514225710af4a436c987b15b9b690. These are not required for
DPCM on Haswell (where I connect to DAIs SSP0 and SSP1) and were written
before Mark upstreamed the CODEC<->CODEC work which also supported
widget DAI connections. It may be that some of these patches are still
required for some configurations. Peter, you may be able to drop some of
these patches since CODEC<->CODEC provides similar DAI widget
functionality. 

We probably have to address the local/global context for widget naming
as Mark suggests. I guess that the global context will only apply to the
machine driver whilst the local context will only be used for component
drivers. This may just mean some changes for machine drivers in the way
we connect to component driver widgets.

Liam

Re: Ambiguous DAPM widget names and DPCM

[Peter Ujfalusi]

On 07/23/2013 05:34 PM, Daniel Mack wrote:
> With printk() in snd_soc_dapm_new_control(), I see the following widgets
> being created during boot (one pair for each Codec, and two pairs for
> the platform dai):
> 
> [    1.963501]  snd_soc_dapm_new_control() ceb6b680 name Playback
> [    1.971739]  snd_soc_dapm_new_control() ceb6b5c0 name Capture
> [    1.996001]  snd_soc_dapm_new_control() ceb6b500 name Playback
> [    2.004354]  snd_soc_dapm_new_control() ceb6b440 name Capture
> [    2.019684]  snd_soc_dapm_new_control() ceb6b380 name Playback
> [    2.028053]  snd_soc_dapm_new_control() ceb6b2c0 name Capture
> [    2.047526]  snd_soc_dapm_new_control() ceb6b200 name Playback
> [    2.055846]  snd_soc_dapm_new_control() ceb6b140 name Capture
> [    2.071168]  snd_soc_dapm_new_control() ceb6b080 name Playback
> [    2.079461]  snd_soc_dapm_new_control() ceb8ff00 name Capture

Can you print these with dev_err(dapm->dev, ...); so we can see the context
where it is going to be added?
It might worth asking the core to prefix the codecs DAPM, so you avoid name
duplication on the same context.
Both cs4271 and ak4101 have 'Playback'/'Capture' names for the streams.

BTW: do you see error messages about name duplication ("widget %s already
exists for context\n")? Or warning about already existing debugfs entries from
DAPM?

-- 
Péter

Re: Ambiguous DAPM widget names and DPCM

[Liam Girdwood]

On Tue, 2013-07-23 at 10:30 +0200, Daniel Mack wrote:
> Hi Mark,
> Hi Liam,
> 
> I spent some hours reverse-engineering the DPCM code, and I plan to
> prepare a patch to add some documentation once I've assembled a fully
> working setup. However, I have a general question regarding DAPM widget
> names.
> 
> DPCM uses DAPM widgets and routes in order to determine the FE <-> BE
> connections at runtime. The problem with this is that widget names are
> very ambiguous, as the automatically created widgets for DAIs are named
> after their stream_name properties, which contain "Playback" or
> "Capture" for all components in my case. Hence, the code which walks the
> connections (snd_soc_dapm_dai_get_connected_widgets()) will most likely
> match the wrong one (which has empty sound/sink lists), which results in
> a failure like this:
> 
> [   16.606268]  fe: ASoC: fe no valid playback route
> 
> I hacked the names of all components so they are unique, but even then,
> the problem is that snd_soc_dapm_new_dai_widgets() is called twice for
> the CPU DAI, once from soc_probe_platform(), and later from
> soc_probe_link_dais(), and both calls would create DAPM widgets with
> identical names, resulting in the same problem as described above.
> 
> I don't know how to handle this, but I believe that nobody was really
> hit by this issue yet, as DPCM doesn't seem to be used widely, at least
> not by any machine code in mainline.

You might be the first to hit this as I never hit it with OMAP4 or the
Haswell DSP code (but it's possible everything just worked for me).

Fwiw, the Haswell audio DSP code can be found here :-

https://git.kernel.org/cgit/linux/kernel/git/lrg/asoc.git/log/?h=intel/haswell-audio-dsp

Dynamic Compressed audio support is now in this branch too.

> 
> One idea would be to change the automatically generated names of DAI
> widgets in order to make them unique, or re-factor the lookup routines.
> 
> Any opinion on that? Do I miss a general consideration here?
> 

I did initially have something like that for OMAP that based the DAI
widget name on the DAI device name, but this also needed something that
would work for codecs with multiple DAI's. In the end it wasn't required
for OMAP, but it sounds like something like a unique name is required
now. 

> With more hacks in place, the setup somehow succeeds, but when the
> stream is opened, we'll hit a BUG() in sound/core/pcm_memory.c, because
> the FE device's DMA type is undefined. But that's another topic I'll
> address later.

Hmm, it looks like your FE is being treated as a regular static PCM in
this case.

Liam

Re: Ambiguous DAPM widget names and DPCM

[Patrick Lai]

On 7/23/2013 1:30 AM, Daniel Mack wrote:
>
> I don't know how to handle this, but I believe that nobody was really
> hit by this issue yet, as DPCM doesn't seem to be used widely, at least
> not by any machine code in mainline.

For your information, DPCM framework is heavily used by drivers
developed by Qualcomm Innovation Center, Inc. even though they are not
upstreamed. I 'll check internally if we came across similar issue or
not.

Thanks
Patrick

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.