| Message ID | 20201202085838.1779525-1-codrin.ciubotariu@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ASoC: pcm_dmaengine: Add support for BE DAIs | expand |
On Wed, Dec 02, 2020 at 10:58:38AM +0200, Codrin Ciubotariu wrote: > This patch is more or less incomplete for the described scenario. This > is because DMAengine's pcm->config is ignored for the BE DAI link, so > runtime->hw is not updated. Also, since pcm_construct/destruct are not > called, the DMA channels are allocated only if DT is used. > Underrun/overrun support would also be a nice to have for the transfers > involving the buffer allocated for the BE. > One way to hold trach of these would be to use a substream_be->runtime > different than the one used for the FE. > Please share your thoughts. I have a hard time getting enthusiastic about this but I think that's more DPCM than anything else. Otherwise this looks sensible as far as it goes. I don't have particular thoughts on exposing errors for the BEs - we could do a dummy PCM, TBH that bodge was used in the past for CODEC<->CODEC links but it's obviously inelegant and messy so I'm not sure it'd help more than just doing something like log the messages in the kernel. It certainly doesn't seem good to introduce anything that is visible to userspace but is DPCM specific.
On 08.12.2020 19:04, Mark Brown wrote: > On Wed, Dec 02, 2020 at 10:58:38AM +0200, Codrin Ciubotariu wrote: > >> This patch is more or less incomplete for the described scenario. This >> is because DMAengine's pcm->config is ignored for the BE DAI link, so >> runtime->hw is not updated. Also, since pcm_construct/destruct are not >> called, the DMA channels are allocated only if DT is used. >> Underrun/overrun support would also be a nice to have for the transfers >> involving the buffer allocated for the BE. >> One way to hold trach of these would be to use a substream_be->runtime >> different than the one used for the FE. > >> Please share your thoughts. > > I have a hard time getting enthusiastic about this but I think that's > more DPCM than anything else. Otherwise this looks sensible as far as > it goes. I don't have particular thoughts on exposing errors for the > BEs - we could do a dummy PCM, TBH that bodge was used in the past for > CODEC<->CODEC links but it's obviously inelegant and messy so I'm not > sure it'd help more than just doing something like log the messages in > the kernel. It certainly doesn't seem good to introduce anything that > is visible to userspace but is DPCM specific. > It is DPCM indeed. Well, the first scenario (PCM1) is. I do not intend to create a PCM for the DAI link, when it is a BE. What I meant to say with the runtime->hw is that is mustn't be touched by the BE's platform, but there should be something similar (placed elsewhere) to consider pcm->config. The thing is that, in my case, exactly the same DAI link (same cpu, codec, platform components) can be a normal DAI link or a BE DAI link. I hope to register both PCMs (dynamic with FE and normal), to be able to skip the FE DAI link if it's not needed and use the normal PCM variant, with the same DAI components used in the BE DAI link. For this, I need a platform driver that is not interacting with substream->runtime when is part of a BE DAI link. I don't think anyone else is using the SOC generic dmaengine as a DAI platform component in a BE. I do not know too much about the dummy PCM. It seems like it is creating a card without DPCM links and fakes a buffer, which is not quite what I need. I will investigate more. Thank you for your sharing your ideas! Best regards, Codrin
On Tue, Dec 08, 2020 at 07:26:35PM +0000, Codrin.Ciubotariu@microchip.com wrote: > I do not know too much about the dummy PCM. It seems like it is creating > a card without DPCM links and fakes a buffer, which is not quite what I > need. I will investigate more. Right, that's what I was imagining the second runtime you proposed looking like.
On 08.12.2020 21:31, Mark Brown wrote: > On Tue, Dec 08, 2020 at 07:26:35PM +0000, Codrin.Ciubotariu@microchip.com wrote: > >> I do not know too much about the dummy PCM. It seems like it is creating >> a card without DPCM links and fakes a buffer, which is not quite what I >> need. I will investigate more. > > Right, that's what I was imagining the second runtime you proposed > looking like. > I don't need the whole struct snd_pcm_runtime, only struct snd_pcm_hardware, at least for now. I looked some more and a suitable place would be struct snd_soc_dpcm, since it is created at runtime and used anyway to characterize a FE <-> BE link. What do you think? Also, I noticed that the HW constraints added by a DAI driver (a codec for example) are added to PCM's runtime->hw_constraints, even if the DAI driver is part of a BE. Shouldn't these constraints be applied only to BE and eventually merge them to FE (struct snd_pcm_hardware) if dai_link->dpcm_merged_* are set? Best regards, Codrin
On Fri, Dec 11, 2020 at 06:00:55PM +0000, Codrin.Ciubotariu@microchip.com wrote: > Also, I noticed that the HW constraints added by a DAI driver (a codec > for example) are added to PCM's runtime->hw_constraints, even if the DAI > driver is part of a BE. Shouldn't these constraints be applied only to > BE and eventually merge them to FE (struct snd_pcm_hardware) if > dai_link->dpcm_merged_* are set? Probably, not sure why it's done like that.
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index 4d059ff2b2e4..5e96bc27628d 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -137,7 +137,9 @@ static void dmaengine_pcm_dma_complete(void *arg) if (prtd->pos >= snd_pcm_lib_buffer_bytes(substream)) prtd->pos = 0; - snd_pcm_period_elapsed(substream); + /* do no update period for an internal PCM */ + if (!substream->pcm->internal) + snd_pcm_period_elapsed(substream); } static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) @@ -147,6 +149,7 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) struct dma_async_tx_descriptor *desc; enum dma_transfer_direction direction; unsigned long flags = DMA_CTRL_ACK; + dma_addr_t addr; direction = snd_pcm_substream_to_dma_direction(substream); @@ -154,11 +157,15 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) flags |= DMA_PREP_INTERRUPT; prtd->pos = 0; + if (substream->pcm->internal) + addr = substream->dma_buffer.addr; + else + addr = substream->runtime->dma_addr; + desc = dmaengine_prep_dma_cyclic(chan, - substream->runtime->dma_addr, + addr, snd_pcm_lib_buffer_bytes(substream), snd_pcm_lib_period_bytes(substream), direction, flags); - if (!desc) return -ENOMEM; @@ -315,6 +322,11 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream, substream->runtime->private_data = prtd; + if (substream->pcm->internal) { + substream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_IRAM; + substream->dma_buffer.dev.dev = chan->device->dev; + } + return 0; } EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open); diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 9ef80a48707e..f403849cd1aa 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -97,6 +97,31 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, if (ret) return ret; } + if (!substream->pcm->internal) { + return snd_pcm_lib_malloc_pages(substream, + params_buffer_bytes(params)); + } + + /* allocate a buffer for BE DAI; for now, the buffer will have the same + * size as the buffer used by the FE + */ + if (snd_dma_alloc_pages(substream->dma_buffer.dev.type, + substream->dma_buffer.dev.dev, + params_buffer_bytes(params), + &substream->dma_buffer) < 0) + return -ENOMEM; + + return 0; +} + +static int dmaengine_pcm_hw_free(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + if (!substream->pcm->internal) + return 0; + + snd_dma_free_pages(&substream->dma_buffer); + substream->dma_buffer.area = NULL; return 0; } @@ -157,9 +182,12 @@ static int dmaengine_pcm_open(struct snd_soc_component *component, struct dma_chan *chan = pcm->chan[substream->stream]; int ret; - ret = dmaengine_pcm_set_runtime_hwparams(component, substream); - if (ret) - return ret; + /* do not touch runtime if this is an internal PCM */ + if (!substream->pcm->internal) { + ret = dmaengine_pcm_set_runtime_hwparams(component, substream); + if (ret) + return ret; + } return snd_dmaengine_pcm_open(substream, chan); } @@ -309,7 +337,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component, channel * (runtime->dma_bytes / runtime->channels); int ret; - if (is_playback) + if (!substream->pcm->internal && is_playback) if (copy_from_user(dma_ptr, buf, bytes)) return -EFAULT; @@ -319,7 +347,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component, return ret; } - if (!is_playback) + if (!substream->pcm->internal && !is_playback) if (copy_to_user(buf, dma_ptr, bytes)) return -EFAULT; @@ -332,6 +360,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component = { .open = dmaengine_pcm_open, .close = dmaengine_pcm_close, .hw_params = dmaengine_pcm_hw_params, + .hw_free = dmaengine_pcm_hw_free, .trigger = dmaengine_pcm_trigger, .pointer = dmaengine_pcm_pointer, .pcm_construct = dmaengine_pcm_new, @@ -344,6 +373,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component_process = { .close = dmaengine_pcm_close, .hw_params = dmaengine_pcm_hw_params, .trigger = dmaengine_pcm_trigger, + .hw_free = dmaengine_pcm_hw_free, .pointer = dmaengine_pcm_pointer, .copy_user = dmaengine_copy_user, .pcm_construct = dmaengine_pcm_new,
Considering the bellow scenarios: /- Gen DMA engine -\ 48KHz ******** ******* 44.1KHz ******** ******* ********* PCM0 <-FE DAI-> * FE * <-> * DSP * <-BE DAI-> * * * * * * * Ring * * * * Ring * <-> * CPU * <-> * Codec * * buff * ******* * buff * * DAI * * DAI * 44.1KHz ******** * * * * * * PCM1 <--------------------DAI-----------------> * * * * * * ******** ******* ********* For PCM0, we have two DAI links. The first DAI link is a FE, with a DSP as a CPU DAI and a platform driver .The second DAI link is a BE DAI link, with separate CPU, codec and platform drivers. We can also notice that there are two Ring buffers: the first one used by the DSP to communicate with the user-space and the second one used to move data between FE (DSP) and BE (CPU). PCM1 is a normal DAI link, with a CPU, codec and platform driver. It is exactly the previous BE DAI link from PCM0, so the samples from user-space are copied directly into the second Ring buffer. In this scenario, the BE DAI link driver should be the same, since it is decided at runtime whether the DAI is a used as a BE or not. The generic platform driver needs to be aware of this thing. For the BE case (PCM0), some callbacks of the platform driver are not called, hence the preallocated buffer is not available. Also, the PCM runtime strcture must not be touched, since it should be only used by the FE platform driver. With these changes, the generic dmaengine can also be used as a BE platform driver. Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- Hello, This patch is more or less incomplete for the described scenario. This is because DMAengine's pcm->config is ignored for the BE DAI link, so runtime->hw is not updated. Also, since pcm_construct/destruct are not called, the DMA channels are allocated only if DT is used. Underrun/overrun support would also be a nice to have for the transfers involving the buffer allocated for the BE. One way to hold trach of these would be to use a substream_be->runtime different than the one used for the FE. Please share your thoughts. sound/core/pcm_dmaengine.c | 18 ++++++++++-- sound/soc/soc-generic-dmaengine-pcm.c | 40 +++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 8 deletions(-)