Re: [alsa-devel] [PATCH 14/15] ARM: pxa: change SSP devices allocation
Arnd Bergmann arnd@arndb.de writes:
chop chop ... removed several mail recipients to leave only the ASoC / PXA subset ...
On Mon, Apr 2, 2018 at 4:26 PM, Robert Jarzmik robert.jarzmik@free.fr wrote:
+static struct pxa_ssp_info pxa_ssp_infos[] = {
{ .dma_chan_rx_name = "ssp1_rx", .dma_chan_tx_name = "ssp1_tx", },{ .dma_chan_rx_name = "ssp1_rx", .dma_chan_tx_name = "ssp1_tx", },{ .dma_chan_rx_name = "ssp2_rx", .dma_chan_tx_name = "ssp2_tx", },{ .dma_chan_rx_name = "ssp2_rx", .dma_chan_tx_name = "ssp2_tx", },{ .dma_chan_rx_name = "ssp3_rx", .dma_chan_tx_name = "ssp3_tx", },{ .dma_chan_rx_name = "ssp3_rx", .dma_chan_tx_name = "ssp3_tx", },{ .dma_chan_rx_name = "ssp4_rx", .dma_chan_tx_name = "ssp4_tx", },{ .dma_chan_rx_name = "ssp4_rx", .dma_chan_tx_name = "ssp4_tx", },+};
This part looks odd to me, you're adding an extra level of indirection to do two stages of lookups in some form of platform data.
That's unfortunately right.
Why can't you just always use "rx" and "tx" as the names here?
Well I couldn't. I'll explain you why, and maybe you'll find a better solution.
That all is related to how ASoC and SSP interact. If I remember correctly, here is how it works : - the DMA channel is requested in sound/arm/pxa2xx-pcm-lib.c:128 return snd_dmaengine_pcm_open( substream, dma_request_slave_channel(rtd->platform->dev, The trick is that the device here is _not_ the SSP one, it's if memory serves me well the pxa-pcm-audio one.
As a consequence, the device cannot be used to differenciate which SSP exactly is providing the sound samples stream. This information is nevertheless required to choose the correct requestor line, which is a 1-to-1 match to the SSP port.
The indirection in the channel name is used to choose the correct requestor line for a given SSP port providing the samples.
It also must be underlined that this dma request serves both AC97 and SSP as sample providers.
(also, I don't see why each line is duplicated, but I'm sure there's an easy answer for that).
Ahh that is an unfortunate rebase most probably :)
Cheers.
On Tue, Apr 3, 2018 at 5:32 PM, Robert Jarzmik robert.jarzmik@free.fr wrote:
Arnd Bergmann arnd@arndb.de writes:
chop chop ... removed several mail recipients to leave only the ASoC / PXA subset ...
On Mon, Apr 2, 2018 at 4:26 PM, Robert Jarzmik robert.jarzmik@free.fr wrote:
+static struct pxa_ssp_info pxa_ssp_infos[] = {
{ .dma_chan_rx_name = "ssp1_rx", .dma_chan_tx_name = "ssp1_tx", },{ .dma_chan_rx_name = "ssp1_rx", .dma_chan_tx_name = "ssp1_tx", },{ .dma_chan_rx_name = "ssp2_rx", .dma_chan_tx_name = "ssp2_tx", },{ .dma_chan_rx_name = "ssp2_rx", .dma_chan_tx_name = "ssp2_tx", },{ .dma_chan_rx_name = "ssp3_rx", .dma_chan_tx_name = "ssp3_tx", },{ .dma_chan_rx_name = "ssp3_rx", .dma_chan_tx_name = "ssp3_tx", },{ .dma_chan_rx_name = "ssp4_rx", .dma_chan_tx_name = "ssp4_tx", },{ .dma_chan_rx_name = "ssp4_rx", .dma_chan_tx_name = "ssp4_tx", },+};
This part looks odd to me, you're adding an extra level of indirection to do two stages of lookups in some form of platform data.
That's unfortunately right.
Why can't you just always use "rx" and "tx" as the names here?
Well I couldn't. I'll explain you why, and maybe you'll find a better solution.
That all is related to how ASoC and SSP interact. If I remember correctly, here is how it works :
the DMA channel is requested in sound/arm/pxa2xx-pcm-lib.c:128 return snd_dmaengine_pcm_open( substream, dma_request_slave_channel(rtd->platform->dev, The trick is that the device here is _not_ the SSP one, it's if memory serves me well the pxa-pcm-audio one.
As a consequence, the device cannot be used to differenciate which SSP exactly is providing the sound samples stream. This information is nevertheless required to choose the correct requestor line, which is a 1-to-1 match to the SSP port.
The indirection in the channel name is used to choose the correct requestor line for a given SSP port providing the samples.
It also must be underlined that this dma request serves both AC97 and SSP as sample providers.
I'm still unable to follow through that code, but I understand now that the device you pass to dma_request_slave_channel() is not the one we'd like it to be here.
Where exactly does that call to dma_request_chan() happen? Is this the one in dmaengine_pcm_new()? Could we perhaps put a pointer to the SSP device into snd_dmaengine_dai_dma_data?
Arnd
Arnd Bergmann arnd@arndb.de writes:
I'm still unable to follow through that code, but I understand now that the device you pass to dma_request_slave_channel() is not the one we'd like it to be here.
Where exactly does that call to dma_request_chan() happen? Is this the one in dmaengine_pcm_new()? Could we perhaps put a pointer to the SSP device into snd_dmaengine_dai_dma_data?
This is a sample stack I captured with an added WARN_ON(1), triggered by a userland "aplay Sultans_Of_Swing.wav" :)
[ 299.216743] [<c0012e80>] (unwind_backtrace) from [<c000f55c>] (show_stack+0x20/0x24) [ 299.223986] [<c000f55c>] (show_stack) from [<c0529d68>] (dump_stack+0x20/0x28) [ 299.231321] [<c0529d68>] (dump_stack) from [<c001de34>] (__warn+0xf0/0x11c) [ 299.238183] [<c001de34>] (__warn) from [<c001df94>] (warn_slowpath_null+0x4c/0x58) [ 299.245234] [<c001df94>] (warn_slowpath_null) from [<c02a2d38>] (dma_request_chan+0x40/0x228) [ 299.252550] [<c02a2d38>] (dma_request_chan) from [<c02a2f38>] (dma_request_slave_channel+0x18/0x24) [ 299.259855] [<c02a2f38>] (dma_request_slave_channel) from [<c03f76f0>] (__pxa2xx_pcm_open+0xf4/0x110) [ 299.266789] [<c03f76f0>] (__pxa2xx_pcm_open) from [<c0409ed0>] (soc_pcm_open+0xf8/0x9c8) [ 299.273932] [<c0409ed0>] (soc_pcm_open) from [<c03db9d4>] (snd_pcm_open_substream+0x9c/0x134) [ 299.281290] [<c03db9d4>] (snd_pcm_open_substream) from [<c03dbb28>] (snd_pcm_open+0xbc/0x22c) [ 299.288255] [<c03dbb28>] (snd_pcm_open) from [<c03dbd58>] (snd_pcm_playback_open+0x50/0x88) [ 299.295468] [<c03dbd58>] (snd_pcm_playback_open) from [<c03c9708>] (snd_open+0x124/0x144) [ 299.302897] [<c03c9708>] (snd_open) from [<c0117de4>] (chrdev_open+0x1a0/0x1f0) [ 299.310296] [<c0117de4>] (chrdev_open) from [<c010f4cc>] (do_dentry_open.constprop.0+0x1d4/0x31c) [ 299.317345] [<c010f4cc>] (do_dentry_open.constprop.0) from [<c011061c>] (vfs_open+0x7c/0x80) [ 299.324597] [<c011061c>] (vfs_open) from [<c0123080>] (path_openat+0xbe8/0xf90) [ 299.332003] [<c0123080>] (path_openat) from [<c0124240>] (do_filp_open+0x80/0xe4) [ 299.339044] [<c0124240>] (do_filp_open) from [<c0110a08>] (do_sys_open+0x148/0x1f8) [ 299.346225] [<c0110a08>] (do_sys_open) from [<c0110ae4>] (SyS_open+0x2c/0x30) [ 299.353505] [<c0110ae4>] (SyS_open) from [<c0009000>] (ret_fast_syscall+0x0/0x28)
Cheers.
On Thu, Apr 5, 2018 at 8:51 AM, Robert Jarzmik robert.jarzmik@free.fr wrote:
Arnd Bergmann arnd@arndb.de writes:
I'm still unable to follow through that code, but I understand now that the device you pass to dma_request_slave_channel() is not the one we'd like it to be here.
Where exactly does that call to dma_request_chan() happen? Is this the one in dmaengine_pcm_new()? Could we perhaps put a pointer to the SSP device into snd_dmaengine_dai_dma_data?
This is a sample stack I captured with an added WARN_ON(1), triggered by a userland "aplay Sultans_Of_Swing.wav" :)
[ 299.216743] [<c0012e80>] (unwind_backtrace) from [<c000f55c>] (show_stack+0x20/0x24) [ 299.223986] [<c000f55c>] (show_stack) from [<c0529d68>] (dump_stack+0x20/0x28) [ 299.231321] [<c0529d68>] (dump_stack) from [<c001de34>] (__warn+0xf0/0x11c) [ 299.238183] [<c001de34>] (__warn) from [<c001df94>] (warn_slowpath_null+0x4c/0x58) [ 299.245234] [<c001df94>] (warn_slowpath_null) from [<c02a2d38>] (dma_request_chan+0x40/0x228) [ 299.252550] [<c02a2d38>] (dma_request_chan) from [<c02a2f38>] (dma_request_slave_channel+0x18/0x24) [ 299.259855] [<c02a2f38>] (dma_request_slave_channel) from [<c03f76f0>] (__pxa2xx_pcm_open+0xf4/0x110) [ 299.266789] [<c03f76f0>] (__pxa2xx_pcm_open) from [<c0409ed0>] (soc_pcm_open+0xf8/0x9c8) [ 299.273932] [<c0409ed0>] (soc_pcm_open) from [<c03db9d4>] (snd_pcm_open_substream+0x9c/0x134)
Ok, so it seems we currently have a special case here that is only used by pxa/mmp and omap, and you can simply open-code that by calling dma_request_chan() from the pxa-specific __pxa2xx_pcm_open() with any device pointer you like, it doesn't have to be the main audio device.
I don't know exactly how the probing works, but I'd assume that we have the correct device pointers in pxa2xx_ac97_dev_probe() and asoc_ssp_probe(), or maybe in pxa2xx_ac97_*_startup() and pxa_ssp_startup().
Arnd
Arnd Bergmann arnd@arndb.de writes:
I don't know exactly how the probing works, but I'd assume that we have the correct device pointers in pxa2xx_ac97_dev_probe() and asoc_ssp_probe(), or maybe in pxa2xx_ac97_*_startup() and pxa_ssp_startup().
Yes, let's try this way, in the former patch
"ASoC: pxa: remove the dmaengine compat need" : +++ b/sound/arm/pxa2xx-pcm-lib.c @@ -126,7 +127,7 @@ int __pxa2xx_pcm_open(struct snd_pcm_substream *substream) return ret;
return snd_dmaengine_pcm_open( - substream, dma_request_slave_channel(rtd->platform->dev, + substream, dma_request_slave_channel(rtd->cpu_dai->dev, dma_params->chan_name)); }
The cpu_dai device should be either pxa27x_ac97 or pxa-ssp-dai.<Id>, and amend the dma slave map accordingly.
Cheers.
participants (2)
-
Arnd Bergmann -
Robert Jarzmik