[alsa-devel] Ambiguous DAPM widget names and DPCM
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
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.
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/haswe...
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
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
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.
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
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.
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
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?
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
participants (5)
-
Daniel Mack -
Liam Girdwood -
Mark Brown -
Patrick Lai -
Peter Ujfalusi