| Message ID | 20130810161123.GW23006@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote: > > On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote: > > > > > I put it to you that DPCM in mainline is incomplete and nonfunctional > > > as it currently stands. Moreover, it requires either those who know > > > that code to continue to develop it, or someone else who understands > > > the direction that this code is supposed to go picks up to complete it. > > > > I'd be most distressed if it were far off working; it's close enough to > > the out of tree code I've worked with and I know there were drivers in > > progress when it was submitted. We've certainly had at least one bug > > fix from the out of tree users, I'd be surprised if we had anything more > > substantial than bitrot in the current code. > > Right, so, I've looked at this again today, and I've sort-of got it > working. I'm not so sure about that: Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = d2450000 [00000008] *pgd=12285831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT ARM Modules linked in: fuse bnep rfcomm bluetooth ext2 ext3 jbd snd_soc_spdif_tx m25p80 orion_wdt mtd snd_soc_kirkwood snd_soc_kirkwood_spdif CPU: 0 PID: 2514 Comm: vlc Not tainted 3.10.0+ #652 task: d8102800 ti: d38fa000 task.ti: d38fa000 PC is at snd_pcm_info+0xc8/0xd8 LR is at 0x30232065 pc : [<c030f724>] lr : [<30232065>] psr: a00f0013 sp : d38fbea8 ip : d8c2ead0 fp : c05de6d8 r10: c05de7d0 r9 : fffffdfd r8 : 00000000 r7 : d8c268a8 r6 : d8c26800 r5 : d8c26c00 r4 : d8c2ea00 r3 : 00000000 r2 : d8c2ea00 r1 : 00000001 r0 : d8c26c00 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c5387d Table: 12450019 DAC: 00000015 Process vlc (pid: 2514, stack limit = 0xd38fa248) Stack: (0xd38fbea8 to 0xd38fc000) bea0: c0af7144 d8c2ea00 d8c26c00 ab5032b8 00000001 c030f768 bec0: 00000000 d8c20000 ab5032b8 c030a67c 0000001b d116b840 d8380330 c1205531 bee0: 0000001b d116b840 d8263fc0 d8c20000 ab5032b8 c03056b0 00000001 c05e6e80 bf00: c05e6e88 c05be828 00020120 00000000 d38fa000 600f0013 00000001 0000001b bf20: d38fa000 00020000 ab5032b8 c0088fec 00000001 00000000 d38fa000 00000000 bf40: 600f0013 ca17c380 0000001b ab5032b8 d8380330 0000001b d38fa000 00020000 bf60: ab5032b8 c00e50bc c00edecc 00020000 ab5032b8 00000001 ca17c380 ab5032b8 bf80: c1205531 c00e5394 ab50366c 00000001 00000000 000120b0 ab50366c 00000036 bfa0: c000e5a8 c000e3e0 00000000 000120b0 0000001b c1205531 ab5032b8 a91a3e10 bfc0: 00000000 000120b0 ab50366c 00000036 ab503454 00000001 00000000 ab5032b8 bfe0: b69a00f4 ab5032a4 b6933109 b6e0d07c a00f0010 0000001b aaaaaaaa aaaaaaaa [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c) [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) from [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60) [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48) Code: e1a00005 e59530dc e3a01001 e1a02004 (e5933008) That is caused by: /* AB: FIXME!!! This is definitely nonsense */ if (runtime) { info->sync = runtime->sync; substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_INFO, info); } where substream->ops is NULL. So, we're back to square one and DPCM not being correctly functional.
On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote: > It would appear that the problem is that the AIF widgets are not finding > their stream - the implicit routes are not being created. It _appears_ > that the strean name in widgets are only ever connected to the streams > within the same DAPM context. That shouldn't be the case in general - DAPM binds first to a widget in the local context for disambiguation and then falls back to a search of the gobal context. However it is possible that you've got an ordering issue during registration. > So, it's because the capture isn't wired up. The capture isn't wired > up on this platform, so how do I tell ASoC not to bother with the > capture side with DPCM? Creating a link for the capture side to the > dummy codec is wrong, because that allows capture to be brought up when > in fact there is no capture wired up on the board (and means that we'll > end up trying to use unconnected inputs.) What I'd expect to happen is that the wiring for the SoC internal bits can be there since it physically is there but we don't generate anything that userspace can see unless we also provide a back end it can be wired up to. We probably need to fill that bit in though since I don't think anyone ever tried to use this stuff on a unidirectional system before. > but doesn't stop those warnings (not really surprising, because it isn't > the card level which is being complained about). Probably the easiest > way to stop that appearing is to just disable OSS compatibility. > That's something which should be documented until it is fixed - that > DPCM is currently incompatible with OSS. That's not likely to ever be supported even if anyone cared, very few ASoC drivers will work with OSS due to the very poor mapping between OSS parameter configuration and ALSA parameter configuration. > So, there are two issues which still need resolving: > 1. The registration of Codec widgets from the platform introduced by > Liam in be09ad90e17b79fdb0d513a31e814ff4d42e3dff > ASoC: core: Add platform DAI widget mapping Well, that patch isn't visibly doing anything with CODECs - it's acting on the CPU and platform, not on the CODECs. I think what's happening there is that if you've got the same device for the DAI and platform then the widgets will be created twice, once by the DAI and once by the platform. The current code is fine if they're separate devices but will fail if they are the same device. We just need to make the DAPM context per device I think, possibly as part of moving over to components (based on the work that Morimoto-san started) though we could do it without that. > 2. How to deal with disconnected front end streams which have no backend > provided by their connected codec (in this case, front end can do > capture and playback, back end can only do playback.) Like I say I think we should just not be complaining if there's no capture back end available. Probably just checking if there's any at all is good enough for realistic uses.
On Sat, 2013-08-10 at 22:13 +0100, Russell King - ARM Linux wrote: > On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote: > > On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote: > > > On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote: > > > > > > > I put it to you that DPCM in mainline is incomplete and nonfunctional > > > > as it currently stands. Moreover, it requires either those who know > > > > that code to continue to develop it, or someone else who understands > > > > the direction that this code is supposed to go picks up to complete it. > > > > > > I'd be most distressed if it were far off working; it's close enough to > > > the out of tree code I've worked with and I know there were drivers in > > > progress when it was submitted. We've certainly had at least one bug > > > fix from the out of tree users, I'd be surprised if we had anything more > > > substantial than bitrot in the current code. > > > > Right, so, I've looked at this again today, and I've sort-of got it > > working. > > I'm not so sure about that: Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow. Liam
On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote: > Russell, I'm just back from holiday now and have done a quick re test on > Haswell. I can see the correct DAPM path and DPCM state as expected for > Haswell, but let me get through my Inbox today and I'll see where we > have some differences tomorrow. Liam, welcome back. Looking at this last night, I notice this: * Creates a new internal PCM instance with no userspace device or procfs * entries. This is used by ASoC Back End PCMs in order to create a PCM that * will only be used internally by kernel drivers. i.e. it cannot be opened * by userspace. It provides existing ASoC components drivers with a substream * and access to any private data. * * The pcm operators have to be set afterwards to the new instance * via snd_pcm_set_ops(). Note the final paragraph. So, soc-pcm.c does this: /* create the PCM */ if (rtd->dai_link->no_pcm) { snprintf(new_name, sizeof(new_name), "(%s)", rtd->dai_link->stream_name); ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, playback, capture, &pcm); } else { ... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops); if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); ... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret; Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream. The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.
On Mon, 2013-08-12 at 09:28 +0100, Russell King - ARM Linux wrote: > On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote: > > Russell, I'm just back from holiday now and have done a quick re test on > > Haswell. I can see the correct DAPM path and DPCM state as expected for > > Haswell, but let me get through my Inbox today and I'll see where we > > have some differences tomorrow. > > Liam, welcome back. > > Looking at this last night, I notice this: > > * Creates a new internal PCM instance with no userspace device or procfs > * entries. This is used by ASoC Back End PCMs in order to create a PCM that > * will only be used internally by kernel drivers. i.e. it cannot be opened > * by userspace. It provides existing ASoC components drivers with a substream > * and access to any private data. > * > * The pcm operators have to be set afterwards to the new instance > * via snd_pcm_set_ops(). > > Note the final paragraph. So, soc-pcm.c does this: > > /* create the PCM */ > if (rtd->dai_link->no_pcm) { > snprintf(new_name, sizeof(new_name), "(%s)", > rtd->dai_link->stream_name); > > ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, > playback, capture, &pcm); > } else { > ... > if (rtd->dai_link->no_pcm) { > if (playback) > pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; > if (capture) > pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; > goto out; > } > ... > if (playback) > snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops); > > if (capture) > snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); > ... > out: > dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, > cpu_dai->name); > return ret; > > Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, > because it uses the dummy DAI, has both a playback and a capture > stream. > > The above is the only place where the ops are set on the ALSA stream(s) > by ASoC, which means there's no way that the internal streams can ever > have a non-NULL ops pointer, and suggests a possible reason why I get > this oops. Ah, Ok. The OMAP4 ABE driver always had ops so never hit this (probably true for Qualcomm driver too). I think we need a check at some point so that we don't dereference this if no ops are set. Iiuc, there should be no harm in setting the ops (since ASoC checks for NULL) for either playback or capture as a workaround in the short term until the fix is upstream. Liam
On Tue, Aug 13, 2013 at 03:59:12PM +0100, Liam Girdwood wrote: > On Mon, 2013-08-12 at 09:28 +0100, Russell King - ARM Linux wrote: > > On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote: > > > Russell, I'm just back from holiday now and have done a quick re test on > > > Haswell. I can see the correct DAPM path and DPCM state as expected for > > > Haswell, but let me get through my Inbox today and I'll see where we > > > have some differences tomorrow. > > > > Liam, welcome back. > > > > Looking at this last night, I notice this: > > > > * Creates a new internal PCM instance with no userspace device or procfs > > * entries. This is used by ASoC Back End PCMs in order to create a PCM that > > * will only be used internally by kernel drivers. i.e. it cannot be opened > > * by userspace. It provides existing ASoC components drivers with a substream > > * and access to any private data. > > * > > * The pcm operators have to be set afterwards to the new instance > > * via snd_pcm_set_ops(). > > > > Note the final paragraph. So, soc-pcm.c does this: > > > > /* create the PCM */ > > if (rtd->dai_link->no_pcm) { > > snprintf(new_name, sizeof(new_name), "(%s)", > > rtd->dai_link->stream_name); > > > > ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, > > playback, capture, &pcm); > > } else { > > ... > > if (rtd->dai_link->no_pcm) { > > if (playback) > > pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; > > if (capture) > > pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; > > goto out; > > } > > ... > > if (playback) > > snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops); > > > > if (capture) > > snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); > > ... > > out: > > dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, > > cpu_dai->name); > > return ret; > > > > Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, > > because it uses the dummy DAI, has both a playback and a capture > > stream. > > > > The above is the only place where the ops are set on the ALSA stream(s) > > by ASoC, which means there's no way that the internal streams can ever > > have a non-NULL ops pointer, and suggests a possible reason why I get > > this oops. > > Ah, Ok. The OMAP4 ABE driver always had ops so never hit this (probably > true for Qualcomm driver too). I think we need a check at some point so > that we don't dereference this if no ops are set. > > Iiuc, there should be no harm in setting the ops (since ASoC checks for > NULL) for either playback or capture as a workaround in the short term > until the fix is upstream. So where do we stand with all this stuff? As far as I'm concerned, there's nothing more _I_ can do with DPCM to make it work. DPCM seems to be broken. Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't. So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline. Either that, or I need you to ack the patches and tell Mark to accept them because they're actually correct and its just the core DPCM code which won't allow them to work (which is what I suspect the real situation to be.)
On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote: > Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM > and refuses to take patches which don't. No, that's for simultaneous use of I2S and S/PDIF. Using one or the other shouldn't be a problem, that's just a single DAI and a single PCM which has always been supported. > So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood > into mainline. I would suggest getting the either/or support merged first.
On Tue, Aug 20, 2013 at 12:44:21PM +0100, Mark Brown wrote: > On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote: > > > Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM > > and refuses to take patches which don't. > > No, that's for simultaneous use of I2S and S/PDIF. Using one or the > other shouldn't be a problem, that's just a single DAI and a single PCM > which has always been supported. > > > So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood > > into mainline. > > I would suggest getting the either/or support merged first. Sorry, I don't know how to do that.
On Tue, Aug 20, 2013 at 12:49:49PM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 20, 2013 at 12:44:21PM +0100, Mark Brown wrote: > > On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote: > > > > > Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM > > > and refuses to take patches which don't. > > > > No, that's for simultaneous use of I2S and S/PDIF. Using one or the > > other shouldn't be a problem, that's just a single DAI and a single PCM > > which has always been supported. > > > > > So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood > > > into mainline. > > > > I would suggest getting the either/or support merged first. > > Sorry, I don't know how to do that. Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam directly to effectively overrule you, because I believe your position to be wrong on these patches, and based on an incorrect understanding about DPCM - as I've already evidenced earlier in this thread by illustrating that the DAPM setup I create in the CPU DAI part of these patches is exactly the same as Liam creates in his Haswell driver. Moreover, I believe these patches to be _almost_ correct to what Liam suggests is required, so there's really no reason not to take them. (The only thing difference is that the AIF widgets should be registered against the CPU DAI DAPM context when DPCM becomes usable.) Since you continue to refuse to take the patches, but haven't given any further reasons why, and I've shown your original objections to be provably false, you leave me no other options but to try and bypass you, especially when you have plainly stated that you don't care about Kirkwood stuff.
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote: > Let's be a little more clear about that: I don't know how to do that > because that's the approach taken by _these_ very patches which you've > rejected for "abusing the ASoC core". That's why I'm asking Liam The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core. If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example. Just to be clear when I say either/or I'm talking about a DAI driver that can either run in S/PDIF mode or run in I2S mode in a given machine but not support both being hooked up in the same machine. Obviously this isn't the end goal but it might be a useful intermediate step if you find you are are blocked. > Since you continue to refuse to take the patches, but haven't given any > further reasons why, and I've shown your original objections to be > provably false, you leave me no other options but to try and bypass To reiterate please submit patches if you believe everything is OK now and then let's go through and review them. > you, especially when you have plainly stated that you don't care about > Kirkwood stuff. I think you'll find that anything I've said along those lines has been in relation to your strongly worded requests for me to make changes for you.
On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote: > On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote: > > > Let's be a little more clear about that: I don't know how to do that > > because that's the approach taken by _these_ very patches which you've > > rejected for "abusing the ASoC core". That's why I'm asking Liam > > The patches I can recall seeing recently have all had some workarounds > in the core which would need to be resolved differently, though it's > possible I missed that being done in some version in your mails as there > have generally also been a lot of modifications adding debug statements > in the core. The "workarounds in the core" are because there's bugs in the core that I have no idea how to solve. You are allegedly the maintainer for the core code, and so you should understand that code, so you are best placed to say how the core code should be fixed. I'm willing to do the patch generation to fix them but *you* need to give some guidance here - something that you seem incapable to do. At the moment, the only fix I can see being workable is to comment out the broken bit in the core code. If the problem is that you don't understand the issue, then you could try replying with some questions about it. If the problem is that you don't understand the code, well... there's not much point in continuing this discussion until you've had time to study and understand that code. > If you've got code you think is in a good state to submit then please do > send it as a normal patch series, the last one I've got here has "ASoC: > HACK: avoid creating duplicated widgets" as part of it for example. That patch still hasn't gone away, and is still required, because there has been no guidance or comments about the problem. Let's explain it yet again... You have said "there is no problem registering the platform and the CPU dai from the same device structure". Let's assume that's a fact and see what happens in the core code: static int soc_probe_platform(struct snd_soc_card *card, struct snd_soc_platform *platform) { /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) continue; snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } } static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) { if (!cpu_dai->probed && cpu_dai->driver->probe_order == order) { if (!cpu_dai->codec) { cpu_dai->dapm.card = card; if (!try_module_get(cpu_dai->dev->driver->owner)) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } Now, the CPU DAI is added to the dai_list (it has to be there to be found so the DAI link can bind it, and so soc_probe_link_dais() can be called.) Think about what happens with the above code if platform->dev is the same as the device used for the CPU DAI (dai->dev) - which can happen when the platform and CPU DAI are registered from the same platform_device, which you claim is legal with ASoC. Now, look at snd_soc_dapm_new_dai_widgets(): int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai) { if (dai->driver->playback.stream_name) { ... dai->playback_widget = w; } if (dai->driver->capture.stream_name) { ... dai->capture_widget = w; } What happens if the widgets which are bound to are the first set that are created, but they're overwritten when the second set get created? (And that _does_ happen.) The second set are the ones activated when the audio device is opened, not the first set. Now, there's nothing new in the above, I've already explained all the above to you several times. I've had nothing of any help what so ever back from you on this. I've asked you how to solve this. I've had absolutely nothing back. So what am I supposed to do here? Stuff doesn't work with the core code how it is, so I took the only solution _you_ left me by your silence, which is to hack the core code. At this point, you leave me with no other conclusion but to assume that the reason you are being so unhelpful is that you don't understand this code, and that you don't know what the right solution is. I dare you to tell me I'm wrong: and the only thing that will convince me that I'm wrong is if you tell me how you'd like the above issue to be solved. > Just to be clear when I say either/or I'm talking about a DAI driver > that can either run in S/PDIF mode or run in I2S mode in a given machine > but not support both being hooked up in the same machine. Obviously > this isn't the end goal but it might be a useful intermediate step if > you find you are are blocked. Well, it doesn't take much to realise that the CPU DAI needs to know whether the _single_ attached codec is SPDIF or I2S. How it does that is via which AIFs are bound - and as DPCM doesn't work at the moment, you can only do that by having _one_ DAI link specified, and DAPM routes between the AIFs and the codecs streams. If you know a better way, you could try saying what that is, because at the moment what I've presented in these patches is the best that I can do with the limited knowledge of ASoC. I'm not going to litter the driver with #ifdef's to work around this when there's a perfectly good way to work around it at runtime - which I've proven can work if the core code issue I point out above gets fixed. > > Since you continue to refuse to take the patches, but haven't given any > > further reasons why, and I've shown your original objections to be > > provably false, you leave me no other options but to try and bypass > > To reiterate please submit patches if you believe everything is OK now > and then let's go through and review them. You mean like your poor review attempt on the first round where you completely ignored the issue with the core code (the HACK patch)? You mean like your poor review stating that the DAPM infrastructure I added in the CPU level code was all wrong, but it turns out that it's exactly the same as Liam's DPCM setup. Why should I submit a new round of patches which haven't changed in any meaningful way (which I've already stated) for another review by someone who seems not to know what they're talking about? Can't you go back and look at the existing set again and provide a better quality of review, maybe providing some suggestions on the core issue which I keep pointing out? This is precisely why I've called for Liam to look at them instead; I feel that I will get a much better quality of review from Liam, and maybe even some helpful suggestions about how to solve some of the remaining issues - something which has been totally lacking in every response from you. > > you, especially when you have plainly stated that you don't care about > > Kirkwood stuff. > > I think you'll find that anything I've said along those lines has been > in relation to your strongly worded requests for me to make changes for > you. No, my strongly worded emails are directed at you not to ask you to make changes for me, but because you are being obstructive, uncooperative, and unhelpful. Apart from "use DPCM" and "DAPM is just a graph walk" I've had nothing of any real use what so ever back from you. Much of my effort has been based around my own interpretations and debugging of the ASoC code inspite of you, which may or may not be correct. However, one thing I do know is that I've pointed out the bug above multiple times to you and there's still no movement on it - which just confirms my conclusion that you are just being plain obstructive.
On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote: > On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote: > > On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote: > > > > > Let's be a little more clear about that: I don't know how to do that > > > because that's the approach taken by _these_ very patches which you've > > > rejected for "abusing the ASoC core". That's why I'm asking Liam > > > > The patches I can recall seeing recently have all had some workarounds > > in the core which would need to be resolved differently, though it's > > possible I missed that being done in some version in your mails as there > > have generally also been a lot of modifications adding debug statements > > in the core. > > The "workarounds in the core" are because there's bugs in the core that > I have no idea how to solve. You are allegedly the maintainer for the > core code, and so you should understand that code, so you are best placed > to say how the core code should be fixed. I'm willing to do the patch > generation to fix them but *you* need to give some guidance here - > something that you seem incapable to do. At the moment, the only fix I > can see being workable is to comment out the broken bit in the core code. > I'll fix this issue as I've replied privately, but you know it's not appropriate to just comment stuff out in core code (especially if you don't fully understand it). I'm sure you would complain loudly to me if I tried to do a similar HACK in the ARM core. > If the problem is that you don't understand the issue, then you could try > replying with some questions about it. > > If the problem is that you don't understand the code, well... there's not > much point in continuing this discussion until you've had time to study > and understand that code. > > > If you've got code you think is in a good state to submit then please do > > send it as a normal patch series, the last one I've got here has "ASoC: > > HACK: avoid creating duplicated widgets" as part of it for example. > > That patch still hasn't gone away, and is still required, because there > has been no guidance or comments about the problem. Let's explain it > yet again... > > You have said "there is no problem registering the platform and the CPU > dai from the same device structure". Let's assume that's a fact and see > what happens in the core code: > > static int soc_probe_platform(struct snd_soc_card *card, > struct snd_soc_platform *platform) > { > /* Create DAPM widgets for each DAI stream */ > list_for_each_entry(dai, &dai_list, list) { > if (dai->dev != platform->dev) > continue; > > snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); > } > } > > static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) > { > if (!cpu_dai->probed && > cpu_dai->driver->probe_order == order) { > if (!cpu_dai->codec) { > cpu_dai->dapm.card = card; > if (!try_module_get(cpu_dai->dev->driver->owner)) > return -ENODEV; > > list_add(&cpu_dai->dapm.list, &card->dapm_list); > snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); > } > > Now, the CPU DAI is added to the dai_list (it has to be there to be found > so the DAI link can bind it, and so soc_probe_link_dais() can be called.) > > Think about what happens with the above code if platform->dev is the same > as the device used for the CPU DAI (dai->dev) - which can happen when the > platform and CPU DAI are registered from the same platform_device, which > you claim is legal with ASoC. > > Now, look at snd_soc_dapm_new_dai_widgets(): > > int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, > struct snd_soc_dai *dai) > { > if (dai->driver->playback.stream_name) { > ... > dai->playback_widget = w; > } > if (dai->driver->capture.stream_name) { > ... > dai->capture_widget = w; > } > > What happens if the widgets which are bound to are the first set that > are created, but they're overwritten when the second set get created? > (And that _does_ happen.) The second set are the ones activated when > the audio device is opened, not the first set. > > Now, there's nothing new in the above, I've already explained all the > above to you several times. I've had nothing of any help what so ever > back from you on this. I've asked you how to solve this. I've had > absolutely nothing back. So what am I supposed to do here? Stuff > doesn't work with the core code how it is, so I took the only solution > _you_ left me by your silence, which is to hack the core code. > It does seem that your configuration is different to the configurations that work well on Haswell, OMAP4 and Qualcomm and that's probably why you are the only person reporting this atm. I also think the tight coupling between the I2S and SPDIF HW made your problem far more complex and therefore more difficult (for me at least) to follow when the signal to noise ratio of this and related threads started to deteriorate. Both Mark and are are happy to fix things, but please remember that we can't just jump and schedule this work in as top priority, we have to prioritise work on severity and impact alongside that of our employers and customers. I'm sure if things were the other way around (e.g the problem was in the ARM core) then Mark would have to wait for you to respond and fix the issue in your time frame. I'm also certain Mark would not start making the conversation personal either. As I've said, I'll do a proper fix for patch 4 and CC you on the submission. The rest of the series looked ok and then I'm sure Mark will take it. Liam
On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote: > On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote: > > On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote: > > > On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote: > > > > > > > Let's be a little more clear about that: I don't know how to do that > > > > because that's the approach taken by _these_ very patches which you've > > > > rejected for "abusing the ASoC core". That's why I'm asking Liam > > > > > > The patches I can recall seeing recently have all had some workarounds > > > in the core which would need to be resolved differently, though it's > > > possible I missed that being done in some version in your mails as there > > > have generally also been a lot of modifications adding debug statements > > > in the core. > > > > The "workarounds in the core" are because there's bugs in the core that > > I have no idea how to solve. You are allegedly the maintainer for the > > core code, and so you should understand that code, so you are best placed > > to say how the core code should be fixed. I'm willing to do the patch > > generation to fix them but *you* need to give some guidance here - > > something that you seem incapable to do. At the moment, the only fix I > > can see being workable is to comment out the broken bit in the core code. > > > > I'll fix this issue as I've replied privately, but you know it's not > appropriate to just comment stuff out in core code (especially if you > don't fully understand it). I'm sure you would complain loudly to me if > I tried to do a similar HACK in the ARM core. Hang on, tell me exactly *where* I've asked for the hack to be merged. The answer is: I haven't. What I've been asking for all along is how this should be solved, and getting nowhere - whether I ask in a reasonable and calm manner or have to take it to extremes like I have done now. > > If the problem is that you don't understand the issue, then you could try > > replying with some questions about it. > > > > If the problem is that you don't understand the code, well... there's not > > much point in continuing this discussion until you've had time to study > > and understand that code. > > > > > If you've got code you think is in a good state to submit then please do > > > send it as a normal patch series, the last one I've got here has "ASoC: > > > HACK: avoid creating duplicated widgets" as part of it for example. > > > > That patch still hasn't gone away, and is still required, because there > > has been no guidance or comments about the problem. Let's explain it > > yet again... > > > > You have said "there is no problem registering the platform and the CPU > > dai from the same device structure". Let's assume that's a fact and see > > what happens in the core code: > > > > static int soc_probe_platform(struct snd_soc_card *card, > > struct snd_soc_platform *platform) > > { > > /* Create DAPM widgets for each DAI stream */ > > list_for_each_entry(dai, &dai_list, list) { > > if (dai->dev != platform->dev) > > continue; > > > > snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); > > } > > } > > > > static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) > > { > > if (!cpu_dai->probed && > > cpu_dai->driver->probe_order == order) { > > if (!cpu_dai->codec) { > > cpu_dai->dapm.card = card; > > if (!try_module_get(cpu_dai->dev->driver->owner)) > > return -ENODEV; > > > > list_add(&cpu_dai->dapm.list, &card->dapm_list); > > snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); > > } > > > > Now, the CPU DAI is added to the dai_list (it has to be there to be found > > so the DAI link can bind it, and so soc_probe_link_dais() can be called.) > > > > Think about what happens with the above code if platform->dev is the same > > as the device used for the CPU DAI (dai->dev) - which can happen when the > > platform and CPU DAI are registered from the same platform_device, which > > you claim is legal with ASoC. > > > > Now, look at snd_soc_dapm_new_dai_widgets(): > > > > int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, > > struct snd_soc_dai *dai) > > { > > if (dai->driver->playback.stream_name) { > > ... > > dai->playback_widget = w; > > } > > if (dai->driver->capture.stream_name) { > > ... > > dai->capture_widget = w; > > } > > > > What happens if the widgets which are bound to are the first set that > > are created, but they're overwritten when the second set get created? > > (And that _does_ happen.) The second set are the ones activated when > > the audio device is opened, not the first set. > > > > Now, there's nothing new in the above, I've already explained all the > > above to you several times. I've had nothing of any help what so ever > > back from you on this. I've asked you how to solve this. I've had > > absolutely nothing back. So what am I supposed to do here? Stuff > > doesn't work with the core code how it is, so I took the only solution > > _you_ left me by your silence, which is to hack the core code. > > > > It does seem that your configuration is different to the configurations > that work well on Haswell, OMAP4 and Qualcomm and that's probably why > you are the only person reporting this atm. I also think the tight > coupling between the I2S and SPDIF HW made your problem far more complex > and therefore more difficult (for me at least) to follow when the > signal to noise ratio of this and related threads started to > deteriorate. Erm, you have completely the wrong end of the stick here. This has nothing to do with I2S and SPDIF at all. It's about having the _same_ struct device assocated with both the platform/DMA backend, registered by snd_soc_register_platform() and the CPU DAI registered via snd_soc_register_component(). SPDIF or I2S doesn't come into this bug.
On Thu, 2013-08-22 at 21:16 +0100, Russell King - ARM Linux wrote: > On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote: > > On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote: > > > On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote: > > > > On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote: > > > > > > > > > Let's be a little more clear about that: I don't know how to do that > > > > > because that's the approach taken by _these_ very patches which you've > > > > > rejected for "abusing the ASoC core". That's why I'm asking Liam > > > > > > > > The patches I can recall seeing recently have all had some workarounds > > > > in the core which would need to be resolved differently, though it's > > > > possible I missed that being done in some version in your mails as there > > > > have generally also been a lot of modifications adding debug statements > > > > in the core. > > > > > > The "workarounds in the core" are because there's bugs in the core that > > > I have no idea how to solve. You are allegedly the maintainer for the > > > core code, and so you should understand that code, so you are best placed > > > to say how the core code should be fixed. I'm willing to do the patch > > > generation to fix them but *you* need to give some guidance here - > > > something that you seem incapable to do. At the moment, the only fix I > > > can see being workable is to comment out the broken bit in the core code. > > > > > > > I'll fix this issue as I've replied privately, but you know it's not > > appropriate to just comment stuff out in core code (especially if you > > don't fully understand it). I'm sure you would complain loudly to me if > > I tried to do a similar HACK in the ARM core. > > Hang on, tell me exactly *where* I've asked for the hack to be merged. The > answer is: I haven't. What I've been asking for all along is how this > should be solved, and getting nowhere - whether I ask in a reasonable and > calm manner or have to take it to extremes like I have done now. > You asked me privately to Ack the series so you could carry it in your own tree for upstreaming. Sorry if I misunderstood this request, but it seemed straightforward. > > > If the problem is that you don't understand the issue, then you could try > > > replying with some questions about it. > > > > > > If the problem is that you don't understand the code, well... there's not > > > much point in continuing this discussion until you've had time to study > > > and understand that code. > > > > > > > If you've got code you think is in a good state to submit then please do > > > > send it as a normal patch series, the last one I've got here has "ASoC: > > > > HACK: avoid creating duplicated widgets" as part of it for example. > > > > > > That patch still hasn't gone away, and is still required, because there > > > has been no guidance or comments about the problem. Let's explain it > > > yet again... > > > > > > You have said "there is no problem registering the platform and the CPU > > > dai from the same device structure". Let's assume that's a fact and see > > > what happens in the core code: > > > > > > static int soc_probe_platform(struct snd_soc_card *card, > > > struct snd_soc_platform *platform) > > > { > > > /* Create DAPM widgets for each DAI stream */ > > > list_for_each_entry(dai, &dai_list, list) { > > > if (dai->dev != platform->dev) > > > continue; > > > > > > snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); > > > } > > > } > > > > > > static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) > > > { > > > if (!cpu_dai->probed && > > > cpu_dai->driver->probe_order == order) { > > > if (!cpu_dai->codec) { > > > cpu_dai->dapm.card = card; > > > if (!try_module_get(cpu_dai->dev->driver->owner)) > > > return -ENODEV; > > > > > > list_add(&cpu_dai->dapm.list, &card->dapm_list); > > > snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); > > > } > > > > > > Now, the CPU DAI is added to the dai_list (it has to be there to be found > > > so the DAI link can bind it, and so soc_probe_link_dais() can be called.) > > > > > > Think about what happens with the above code if platform->dev is the same > > > as the device used for the CPU DAI (dai->dev) - which can happen when the > > > platform and CPU DAI are registered from the same platform_device, which > > > you claim is legal with ASoC. > > > > > > Now, look at snd_soc_dapm_new_dai_widgets(): > > > > > > int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, > > > struct snd_soc_dai *dai) > > > { > > > if (dai->driver->playback.stream_name) { > > > ... > > > dai->playback_widget = w; > > > } > > > if (dai->driver->capture.stream_name) { > > > ... > > > dai->capture_widget = w; > > > } > > > > > > What happens if the widgets which are bound to are the first set that > > > are created, but they're overwritten when the second set get created? > > > (And that _does_ happen.) The second set are the ones activated when > > > the audio device is opened, not the first set. > > > > > > Now, there's nothing new in the above, I've already explained all the > > > above to you several times. I've had nothing of any help what so ever > > > back from you on this. I've asked you how to solve this. I've had > > > absolutely nothing back. So what am I supposed to do here? Stuff > > > doesn't work with the core code how it is, so I took the only solution > > > _you_ left me by your silence, which is to hack the core code. > > > > > > > It does seem that your configuration is different to the configurations > > that work well on Haswell, OMAP4 and Qualcomm and that's probably why > > you are the only person reporting this atm. I also think the tight > > coupling between the I2S and SPDIF HW made your problem far more complex > > and therefore more difficult (for me at least) to follow when the > > signal to noise ratio of this and related threads started to > > deteriorate. > > Erm, you have completely the wrong end of the stick here. This has > nothing to do with I2S and SPDIF at all. > > It's about having the _same_ struct device assocated with both the > platform/DMA backend, registered by snd_soc_register_platform() and > the CPU DAI registered via snd_soc_register_component(). SPDIF or > I2S doesn't come into this bug. I was trying to highlight that your HW is the only one with the shared struct device and this iiuc was due to the tight coupling between I2s and SPDIF. Liam
On Fri, Aug 23, 2013 at 01:13:14PM +0100, Liam Girdwood wrote: > On Thu, 2013-08-22 at 21:16 +0100, Russell King - ARM Linux wrote: > > On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote: > > > I'll fix this issue as I've replied privately, but you know it's not > > > appropriate to just comment stuff out in core code (especially if you > > > don't fully understand it). I'm sure you would complain loudly to me if > > > I tried to do a similar HACK in the ARM core. > > > > Hang on, tell me exactly *where* I've asked for the hack to be merged. The > > answer is: I haven't. What I've been asking for all along is how this > > should be solved, and getting nowhere - whether I ask in a reasonable and > > calm manner or have to take it to extremes like I have done now. > > You asked me privately to Ack the series so you could carry it in your > own tree for upstreaming. Sorry if I misunderstood this request, but it > seemed straightforward. What I'm after are acks for those patches which are acceptable - which I believe is the entire series with the exception of the HACK patch. > > Erm, you have completely the wrong end of the stick here. This has > > nothing to do with I2S and SPDIF at all. > > > > It's about having the _same_ struct device assocated with both the > > platform/DMA backend, registered by snd_soc_register_platform() and > > the CPU DAI registered via snd_soc_register_component(). SPDIF or > > I2S doesn't come into this bug. > > I was trying to highlight that your HW is the only one with the shared > struct device and this iiuc was due to the tight coupling between I2s > and SPDIF. Not at all - the tight coupling of the DMA and the CPU DAI backend is merely because there is no actual separation of those two items: the DMA engine is tightly coupled with the audio interface itself. They both share the same register space, and they both make use of the same iomapping created by kirkwood_i2s_dev_probe() in the current tree. The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been trying to get addressed for the last month. Moreover, the split is undesirable when switching to device tree, and as all Marvell support is moving to device tree, having the DMA and CPU DAI backends combined (as Mark has told me several times is fully supported in ASoC) will be required. Bear in mind that ARM is no longer accepting new non-DT support for Marvell stuff, so this is something which must be fixed if any new Marvell platform is to be supported.
On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 23, 2013 at 01:13:14PM +0100, Liam Girdwood wrote: > > You asked me privately to Ack the series so you could carry it in your > > own tree for upstreaming. Sorry if I misunderstood this request, but it > > seemed straightforward. > What I'm after are acks for those patches which are acceptable - which > I believe is the entire series with the exception of the HACK patch. If there no dependency then why is that patch included as part of this series, especially so early on? Obviously we shouldn't cause problems for existing machines in mainline so I stopped applying patches which seemed like they might depend on that one. I had at the time been expecting a revised version of the series to follow in due course with a better fix as at the time the hack was sent you'd only just determined what the problem was. > The split of the DMA backend from the CPU DAI backend is something > which early ASoC forced, but that has "mostly" been fixed with (as > far as I'm currently aware through testing) the problem I've been Essentially all the dmaengine based platforms in mainline use a shared device for DMA and DAI; I'm fairly sure someone would have mentioned if there were problems. As you have been repeatedly told the Kirkwood drivers are the first drivers submitted to mainline which use DPCM and therefore it is not surprising that there are a few issues which need to be worked through, there were a few revisions to the framework which went in as a result of review during the mainline merge. The problem you are seeing here is due to this being the first platform with a *shared* device to use DPCM.
On Fri, Aug 23, 2013 at 05:58:00PM +0100, Mark Brown wrote: > On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote: > > The split of the DMA backend from the CPU DAI backend is something > > which early ASoC forced, but that has "mostly" been fixed with (as > > far as I'm currently aware through testing) the problem I've been > > Essentially all the dmaengine based platforms in mainline use a shared > device for DMA and DAI; I'm fairly sure someone would have mentioned if > there were problems. > > As you have been repeatedly told the Kirkwood drivers are the first > drivers submitted to mainline which use DPCM and therefore it is not > surprising that there are a few issues which need to be worked through, > there were a few revisions to the framework which went in as a result of > review during the mainline merge. The problem you are seeing here is > due to this being the first platform with a *shared* device to use DPCM. So that's why it fails _without_ any DPCM stuff? That's why the codecs fail to have their set_bias stuff called? Look Mark, frankly, shut your fucking mouth up about DPCM. This bug has nothing what so ever to do with DPCM, and the more times you try and state that doesn't change that *FACT*. And it is a FACT. You've had this problem described by IRC, including extracts from the ASoC code indicating where things go wrong. I've shown you the debug I've used. I've shown you the result of that debug. You've had descriptions of this problem via email too. Yet you refuse to acknowledge that there could possibly be a problem here. All the time that you insist that there isn't a problem against factual evidence, you're just making yourself look totally incompetent and obstructive - not only that but you're making yourself look like a total idiot. Your comments above are just PLAIN WRONG. I'm sick to death with you. You are not a fit person to hold the maintainership role for ASoC in my opinion, and you are long past having any ability to change my opinion on that anymore, given your obtuseness against this bug. I live in hope that one day you'll recognise your mistake here and appologise to me - but frankly I think that's something that you would find absolutely impossible to do.
On Fri, Aug 23, 2013 at 06:45:05PM +0100, Russell King - ARM Linux wrote: > Look Mark, frankly, shut your fucking mouth up about DPCM. This bug has > nothing what so ever to do with DPCM, and the more times you try and > state that doesn't change that *FACT*. And it is a FACT. You must know that this and the rest of your e-mail are not an appropriate or constructive way to interact with people. I now regret that have not confronted you on this more openly and directly before but unfortunately I had hoped that stepping back would help with deescalation. I know I have needed to take a step back on several occasions myself in order to be calmer in my response. > So that's why it fails _without_ any DPCM stuff? That's why the codecs > fail to have their set_bias stuff called? So, it is true that the widgets get double created in all cases where the same struct device is used for both platform and DAI. However if DPCM is not used nothing really notices since the SoC side widgets don't do anything useful. The relevance of DPCM is that it tries to use the widgets and therefore causes practical problems in normal use, otherwise it's just a memory leak on init. As you know the issue with the DAPMless CODECs was unrelated to this, was due to poor test coverage of such devices and has been resolved by making DAPM mandatory for CODECs to eliminate the possibility of similar regressions caused by the need for special casing. > You've had this problem described by IRC, including extracts from the > ASoC code indicating where things go wrong. I've shown you the debug > I've used. I've shown you the result of that debug. You've had > descriptions of this problem via email too. Yet you refuse to acknowledge > that there could possibly be a problem here. I am fairly sure that I have agreed that there is a bug and at the very least not said that everything is fine. Just for the record there clearly is a bug with double creation of DAI widgets when the same struct device is used to register both a DAI and a platform. You will also recall that Liam said he would provide a patch when he has time. As I have told you before your generally confrontational and demanding approach is not a good one for getting help from people, it doesn't help push your problems up the priority list or generate positive attention. Especially with the more extreme examples it can actively work against engagement; at an unconscious level it makes things less fun and at a conscious level not only are breaks for calm needed but I know that I at least have no desire to encourage anyone to believe that this is a good way of getting results.
On Thu, Aug 29, 2013 at 11:12:35PM +0200, Liam Girdwood wrote: > Do you not see how statements like this will just end up making you look > like someone who is very difficult to work with when faced with a technical > disagreement ? I had acknowledged to you that this had not been tested for > your configuration before you sent this email and that I would fix the > issue. Sorry, you're making the assumption that there's a _technical_ disagreement here. That's not how I view any of this; I believe that what's been going on has been a stream of misinformation and obstruction which is a totally different issue. While it may be true that the bug affects DPCM _too_, it is not DPCM which is the problem. That's pretty obvious when you consider that the patch series which I posted (which is non-DPCM) trips over the bug as well. Is it reasonable to continually claim that a bug is due to DPCM when the code being used which provokes it doesn't make any use of DPCM. It's that kind of misinformation that I can't stand, and I will make it plainly obvious when there's blatent misinformation being banded out. As a result of this, I can never trust anything Mark says; that's something which I established about a fortnight ago when this same old wrong story was trotted out for the I don't know how many time. Of course people are going to get frustrated and annoyed if that's the kind of behaviour that's being presented: when the facts show that something is wrong, repeating it many times doesn't make it any more right. All that it does is waste time. I note that Mark has finally agreed earlier this week for the *first* time (in reply to the email you quoted) that the bug doesn't necessarily have anything to do with DPCM - which is at last some progress. To have taken four weeks to get to that point is pretty direbolical. What about the oops bug in ALSA PCM that DPCM causes... am I going to have to spend another month trying to get that bug recognised? (That has been ignored too.) If this is what it takes, then I'm seriously going to consider rewriting this as a plain ALSA driver and ditch ASoC; ASoC seems to be too buggy at the moment and it takes too long just to get bugs recognised. It's not the actual fixing that I'm particularly worried about: it's a display that the maintainer correctly recognises the bug, and has the capability to fix it - and if they don't have time, they can communicate how they'd like it to be fixed or pass it to someone who does. Every time I've asked about how to fix the widget overwriting bug, the replies (if any) have avoided the question or given the same old misinformation. You only need to go back and look at the initial patch submission to see that; the patch containing my hacked workaround has never had any kind of response. Is it acceptable to think "the submitter doesn't expect that to be merged so I'll ignore it"? The fact that any kind of hack appears in a patch series means that there's a problem which needs to be solved, and the submitter may not know how it should be fixed. That's definitely the case with this patch set: more so since I'd already asked the question about how it should be solved and already got nowhere. All in all, there's been: 1. The combining of the CPU and platform code(s) under one device is absolutely no problem for ASoC, there's multiple drivers which do this already. While this is a true statement, it creates the bug at the heart of this thread - and the bug _occuring_ has nothing to do with DPCM. The bug is caused solely by the combining of the two components. 2. Stream names in DAIs don't have to be globally unique. This is false if you want to try and bind to specific streams - even more so when you can end up with multiple codecs with DPCM, where all their output streams are called "Playback". It may be that the stream names are not supposed to be used in DAPM routes, but that is not documented, and to date no one has made such a statement. 3. The infamous flippant "DAPM is just a graph walk" statement - it's more complicated than that because DAPM widgets have types which affect how the graph is walked, and the activation in multiple points in the graph make it more difficult to understand. As for the DAPM contexts which affect how the graph gets built, and in some cases whether some links even get created. No, it's more than "just a graph walk" - while true at a very basic level, it provides nothing of any use to anyone trying to understand this code - the response may as well have been "It's written in C". (In the course of this, I've given an overview of it to a small number of people, and it takes a lot more than six words to even begin to describe what it is.) 4. By adding the AIFs and routes, I'm bypassing/abusing the ASoC DAI core. If this were to be the case, why does your DPCM driver set them up. Therefore, my conclusion is that Mark was wrong about this too. Yes, it may be that for the time being it's not that appealing, because as DPCM doesn't work in mainline, I'm unable to separate the DAI link between the CPU and the Codec. That doesn't make the creation of the routes any more wrong when they're the exact same routes which need to be created for DPCM - especially when they're relied upon for there to be any audio output what so ever from the driver. 5. Mark's "suggestion" that I can get this driver to work without this hack... I still don't see how, and Mark hasn't effectively explained how either - and I'm left wondering whether Mark even read the patch to see how it worked, and to see how and why it uses the AIF widgets, which brings into question the "review" which was done. While the simple solution would be to add some ifdefs, that is not acceptable when you consider that this driver is already part of a multi-platform kernel - hard-coding it to one kind of output would definitely cause regressions. Also consider that this patch series was already my best effort at getting this going without using DPCM and without adding ifdefs without causing regressions for other Kirkwood platforms. If those issues mean that I'm difficult to work with... well... I suspect anyone in my position receiving those kinds of responses would also end up getting extremely frustrated, as I have.
On Fri, Aug 30, 2013 at 12:27:05PM +0100, Russell King - ARM Linux wrote: > 1. The combining of the CPU and platform code(s) under one device is > absolutely no problem for ASoC, there's multiple drivers which do this > already. While this is a true statement, it creates the bug at the > heart of this thread - and the bug _occuring_ has nothing to do with > DPCM. The bug is caused solely by the combining of the two components. Right, having gone back and looked at this in-depth again, and tried some experiments, I no longer need this problem fixed for the non-DPCM code to work, but I do still need to do this: > 4. By adding the AIFs and routes, I'm bypassing/abusing the ASoC DAI core. > If this were to be the case, why does your DPCM driver set them up. > Therefore, my conclusion is that Mark was wrong about this too. Yes, > it may be that for the time being it's not that appealing, because as > DPCM doesn't work in mainline, I'm unable to separate the DAI link > between the CPU and the Codec. That doesn't make the creation of > the routes any more wrong when they're the exact same routes which > need to be created for DPCM - especially when they're relied upon for > there to be any audio output what so ever from the driver. so I can detect which output paths to enable. I will be posting updated patches shortly, which will just be those which Mark hasn't taken yet.
diff --git a/sound/soc/codecs/spdif_transciever.c b/sound/soc/codecs/spdif_transciever.c index 553708d..fde0150 100644 --- a/sound/soc/codecs/spdif_transciever.c +++ b/sound/soc/codecs/spdif_transciever.c @@ -36,7 +36,7 @@ static const struct snd_soc_dapm_widget dit_widgets[] = { }; static const const struct snd_soc_dapm_route dit_routes[] = { - { "spdif-out", NULL, "Playback" }, + { "spdif-out", NULL, "spdif-Playback" }, }; static struct snd_soc_codec_driver soc_codec_spdif_dit = { @@ -49,7 +49,7 @@ static struct snd_soc_codec_driver soc_codec_spdif_dit = { static struct snd_soc_dai_driver dit_stub_dai = { .name = "dit-hifi", .playback = { - .stream_name = "Playback", + .stream_name = "spdif-Playback", .channels_min = 1, .channels_max = 384, .rates = STUB_RATES, diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 05d977a..66fc76c 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -500,8 +500,11 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd, static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the device + */ + struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev); if (SND_SOC_DAPM_EVENT_ON(event)) priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN; @@ -514,8 +517,11 @@ static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the device + */ + struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev); if (SND_SOC_DAPM_EVENT_ON(event)) priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN; @@ -553,8 +559,7 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai) } /* It appears that these can't be attached to the CPU DAI */ - snd_soc_dapm_new_controls(&dai->platform->dapm, - widgets, ARRAY_SIZE(widgets)); + snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets)); /* put system in a "safe" state : */ /* disable audio interrupts */ diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c index e5257ec..d1ee8f7 100644 --- a/sound/soc/kirkwood/kirkwood-spdif.c +++ b/sound/soc/kirkwood/kirkwood-spdif.c @@ -57,7 +57,7 @@ static struct snd_soc_ops kirkwood_spdif_ops = { }; static const struct snd_soc_dapm_route routes[] = { - { "Playback", NULL, "spdifdo" }, + { "spdif-Playback", NULL, "spdifdo" }, }; static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { @@ -75,9 +75,18 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { { .name = "S/PDIF1", - .stream_name = "IEC958 Playback", + .stream_name = "Audio Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "mvebu-audio.1", + .codec_name = "snd-soc-dummy", + .codec_dai_name = "snd-soc-dummy-dai", + .dynamic = 1, + }, { + .name = "Codec", + .stream_name = "IEC958 Playback", + .cpu_dai_name = "snd-soc-dummy-dai", + .platform_name = "snd-soc-dummy", + .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, @@ -108,7 +117,7 @@ static int kirkwood_spdif_probe(struct platform_device *pdev) card->dai_link = kirkwood_spdif_dai0; else card->dai_link = kirkwood_spdif_dai1; - card->num_links = 1; + card->num_links = 2; card->dapm_routes = routes; card->num_dapm_routes = ARRAY_SIZE(routes); card->dev = &pdev->dev; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 48e883e..9176815 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1158,6 +1158,7 @@ static int soc_probe_platform(struct snd_soc_card *card, snd_soc_dapm_new_controls(&platform->dapm, driver->dapm_widgets, driver->num_dapm_widgets); +#if 0 /* This breaks DAPM on Kirkwood */ /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) @@ -1165,6 +1166,7 @@ static int soc_probe_platform(struct snd_soc_card *card, snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } +#endif platform->dapm.idle_bias_off = 1; @@ -1362,7 +1364,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); -// snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); + snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } if (cpu_dai->driver->probe) { diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ccb6be4..381df28 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1634,8 +1634,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream) /* there is no point preparing this FE if there are no BEs */ if (list_empty(&fe->dpcm[stream].be_clients)) { - dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n", - fe->dai_link->name); +// dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n", +// fe->dai_link->name); ret = -EINVAL; goto out; }