[RFC] ASoC: pcm_dmaengine: Add support for BE DAIs
diff mbox series

Message ID 20201202085838.1779525-1-codrin.ciubotariu@microchip.com
State New, archived
Headers show
Series
  • [RFC] ASoC: pcm_dmaengine: Add support for BE DAIs
Related show

Commit Message

Codrin Ciubotariu Dec. 2, 2020, 8:58 a.m. UTC
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(-)

Comments

Mark Brown Dec. 8, 2020, 5:04 p.m. UTC | #1
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.
Codrin Ciubotariu Dec. 8, 2020, 7:26 p.m. UTC | #2
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
Mark Brown Dec. 8, 2020, 7:31 p.m. UTC | #3
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.
Codrin Ciubotariu Dec. 11, 2020, 6 p.m. UTC | #4
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
Mark Brown Dec. 14, 2020, 5:19 p.m. UTC | #5
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.

Patch
diff mbox series

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,