[RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Codrin Ciubotariu]

This patchset adds a different snd_pcm_runtime in the BE's substream,
replacing the FE's snd_pcm_runtime. With a different structure, the BE
HW capabilities and constraints will no longer merge with the FE ones.
This allows for error detection if the be_hw_params_fixup() applies HW
parameters not supported by the BE DAIs. Also, it calculates values
needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
period size, if needed.

The first 4 patches are preparatory patches, that just group and export
functions used to allocate and initialize the snd_pcm_runtime. Also, the
functions that set and apply the HW constraints are exported.
The 5th patch does (almost) everything need to create the new snd_pcm_runtime
for BEs, which includes allocation, initializing the HW capabilities,
HW constraints and HW parameters. The BE HW parameters are no longer
copied from the FE. They are recalculated, based on HW capabilities,
constraints and the be_hw_params_fixup() callback.
The 6th and last patch basically adds support for the PCM generic
dmaengine to be used as a platform driver for BE DAI links. It allocates
a buffer, needed by the DMA transfers that do not support dev-to-dev
transfers between FE and BE DAIs.

This is a superset of
https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
which only handles the BE HW constraints. This patchset aims to be more
complete, defining a a snd_pcm_runtime between each FE and BE and can
be used between any DAI link connection. I am sure I am not handling all
the needed members of snd_pcm_runtime (such as handling
struct snd_pcm_mmap_status *status), but I would like to have your
feedback regarding this idea.

Codrin Ciubotariu (6):
  ALSA: core: pcm: Create helpers to allocate/free struct
    snd_pcm_runtime
  ALSA: pcm: Export constraints initialization functions
  ALSA: pcm: Check for substream->ops before substream->ops->mmap
  ALSA: pcm: Create function for snd_pcm_runtime initialization
  ASoC: soc-pcm: Create new snd_pcm_runtime for BE DAIs
  ASoC: dmaengine: Allocate buffer if substream is unmanaged

 include/sound/pcm.h                   |   7 ++
 sound/core/pcm.c                      |  82 ++++++++++-------
 sound/core/pcm_native.c               | 116 +++++++++++++-----------
 sound/soc/soc-generic-dmaengine-pcm.c |  18 ++++
 sound/soc/soc-pcm.c                   | 126 ++++++++++++++++++--------
 5 files changed, 229 insertions(+), 120 deletions(-)

-- 
2.27.0

[RFC PATCH 1/6] ALSA: core: pcm: Create helpers to allocate/free struct snd_pcm_runtime

[Codrin Ciubotariu]

Create helpers to be able to reuse code for allocation and freeing of
struct snd_pcm_runtime.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 include/sound/pcm.h |  2 ++
 sound/core/pcm.c    | 82 +++++++++++++++++++++++++++------------------
 2 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2e1200d17d0c..025b4d2ba0fe 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -574,6 +574,8 @@ static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)
 }
 #endif
 int snd_pcm_kernel_ioctl(struct snd_pcm_substream *substream, unsigned int cmd, void *arg);
+struct snd_pcm_runtime *snd_pcm_runtime_alloc(void);
+void snd_pcm_runtime_free(struct snd_pcm_runtime *runtime);
 int snd_pcm_open_substream(struct snd_pcm *pcm, int stream, struct file *file,
 			   struct snd_pcm_substream **rsubstream);
 void snd_pcm_release_substream(struct snd_pcm_substream *substream);
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index b163164a83ec..8ecb14b27460 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -876,6 +876,53 @@ static int snd_pcm_dev_free(struct snd_device *device)
 	return snd_pcm_free(pcm);
 }
 
+struct snd_pcm_runtime *snd_pcm_runtime_alloc(void)
+{
+	struct snd_pcm_runtime *runtime;
+	size_t size;
+
+	runtime = kzalloc(sizeof(*runtime), GFP_KERNEL);
+	if (!runtime)
+		return NULL;
+
+	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status));
+	runtime->status = alloc_pages_exact(size, GFP_KERNEL);
+	if (!runtime->status) {
+		kfree(runtime);
+		return NULL;
+	}
+	memset(runtime->status, 0, size);
+
+	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control));
+	runtime->control = alloc_pages_exact(size, GFP_KERNEL);
+	if (!runtime->control) {
+		free_pages_exact(runtime->status,
+				 PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
+		kfree(runtime);
+		return NULL;
+	}
+	memset(runtime->control, 0, size);
+
+	init_waitqueue_head(&runtime->sleep);
+	init_waitqueue_head(&runtime->tsleep);
+
+	runtime->status->state = SNDRV_PCM_STATE_OPEN;
+
+	return runtime;
+}
+EXPORT_SYMBOL_GPL(snd_pcm_runtime_alloc);
+
+void snd_pcm_runtime_free(struct snd_pcm_runtime *runtime)
+{
+	free_pages_exact(runtime->status,
+			 PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
+	free_pages_exact(runtime->control,
+			 PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control)));
+	kfree(runtime->hw_constraints.rules);
+	kfree(runtime);
+}
+EXPORT_SYMBOL_GPL(snd_pcm_runtime_free);
+
 int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 			     struct file *file,
 			     struct snd_pcm_substream **rsubstream)
@@ -885,7 +932,6 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 	struct snd_pcm_runtime *runtime;
 	struct snd_card *card;
 	int prefer_subdevice;
-	size_t size;
 
 	if (snd_BUG_ON(!pcm || !rsubstream))
 		return -ENXIO;
@@ -939,33 +985,10 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 	if (substream == NULL)
 		return -EAGAIN;
 
-	runtime = kzalloc(sizeof(*runtime), GFP_KERNEL);
-	if (runtime == NULL)
+	runtime = snd_pcm_runtime_alloc();
+	if (!runtime)
 		return -ENOMEM;
 
-	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status));
-	runtime->status = alloc_pages_exact(size, GFP_KERNEL);
-	if (runtime->status == NULL) {
-		kfree(runtime);
-		return -ENOMEM;
-	}
-	memset(runtime->status, 0, size);
-
-	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control));
-	runtime->control = alloc_pages_exact(size, GFP_KERNEL);
-	if (runtime->control == NULL) {
-		free_pages_exact(runtime->status,
-			       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
-		kfree(runtime);
-		return -ENOMEM;
-	}
-	memset(runtime->control, 0, size);
-
-	init_waitqueue_head(&runtime->sleep);
-	init_waitqueue_head(&runtime->tsleep);
-
-	runtime->status->state = SNDRV_PCM_STATE_OPEN;
-
 	substream->runtime = runtime;
 	substream->private_data = pcm->private_data;
 	substream->ref_count = 1;
@@ -985,11 +1008,6 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream)
 	runtime = substream->runtime;
 	if (runtime->private_free != NULL)
 		runtime->private_free(runtime);
-	free_pages_exact(runtime->status,
-		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
-	free_pages_exact(runtime->control,
-		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control)));
-	kfree(runtime->hw_constraints.rules);
 	/* Avoid concurrent access to runtime via PCM timer interface */
 	if (substream->timer) {
 		spin_lock_irq(&substream->timer->lock);
@@ -998,7 +1016,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream)
 	} else {
 		substream->runtime = NULL;
 	}
-	kfree(runtime);
+	snd_pcm_runtime_free(runtime);
 	put_pid(substream->pid);
 	substream->pid = NULL;
 	substream->pstr->substream_opened--;
-- 
2.27.0

[RFC PATCH 2/6] ALSA: pcm: Export constraints initialization functions

[Codrin Ciubotariu]

Export the functions needed to initialize the constraints. These functions
will be used in soc-pcm to initialize the BE constraints.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 include/sound/pcm.h     | 3 +++
 sound/core/pcm_native.c | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 025b4d2ba0fe..2907ed2b937f 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -1021,6 +1021,9 @@ int snd_pcm_hw_rule_add(struct snd_pcm_runtime *runtime,
 			snd_pcm_hw_rule_func_t func, void *private,
 			int dep, ...);
 
+int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream);
+int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream);
+
 /**
  * snd_pcm_hw_constraint_single() - Constrain parameter to a single value
  * @runtime: PCM runtime instance
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 8dbe86cf2e4f..54e0eab2a57e 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2400,7 +2400,7 @@ static int snd_pcm_hw_rule_buffer_bytes_max(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }		
 
-static int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream)
+int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_pcm_hw_constraints *constrs = &runtime->hw_constraints;
@@ -2523,8 +2523,9 @@ static int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream)
 		return err;
 	return 0;
 }
+EXPORT_SYMBOL(snd_pcm_hw_constraints_init);
 
-static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
+int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_pcm_hardware *hw = &runtime->hw;
@@ -2607,6 +2608,7 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 
 	return 0;
 }
+EXPORT_SYMBOL(snd_pcm_hw_constraints_complete);
 
 static void pcm_release_private(struct snd_pcm_substream *substream)
 {
-- 
2.27.0

[RFC PATCH 3/6] ALSA: pcm: Check for substream->ops before substream->ops->mmap

[Codrin Ciubotariu]

substreams, like the internal ones, might not have ops set. Check it,
before looking at substream->ops->mmap.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 sound/core/pcm_native.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 54e0eab2a57e..cb0164d55593 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -246,7 +246,7 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
 	if (!(substream->runtime->hw.info & SNDRV_PCM_INFO_MMAP))
 		return false;
 
-	if (substream->ops->mmap ||
+	if ((substream->ops && substream->ops->mmap) ||
 	    (substream->dma_buffer.dev.type != SNDRV_DMA_TYPE_DEV &&
 	     substream->dma_buffer.dev.type != SNDRV_DMA_TYPE_DEV_UC))
 		return true;
-- 
2.27.0

[RFC PATCH 4/6] ALSA: pcm: Create function for snd_pcm_runtime initialization

[Codrin Ciubotariu]

Group the setting of snd_pcm_runtime members in a separate function. This
allows for code reutilization. Also, check for substream->ops before
substream->ops->copy_user .

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 include/sound/pcm.h     |   2 +
 sound/core/pcm_native.c | 108 ++++++++++++++++++++++------------------
 2 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2907ed2b937f..8e6bd4525c02 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -576,6 +576,8 @@ static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)
 int snd_pcm_kernel_ioctl(struct snd_pcm_substream *substream, unsigned int cmd, void *arg);
 struct snd_pcm_runtime *snd_pcm_runtime_alloc(void);
 void snd_pcm_runtime_free(struct snd_pcm_runtime *runtime);
+void snd_pcm_runtime_set(struct snd_pcm_substream *substream,
+			 struct snd_pcm_hw_params *params);
 int snd_pcm_open_substream(struct snd_pcm *pcm, int stream, struct file *file,
 			   struct snd_pcm_substream **rsubstream);
 void snd_pcm_release_substream(struct snd_pcm_substream *substream);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index cb0164d55593..5b0e7ae2b1e7 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -658,13 +658,69 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm,
 	return 0;
 }
 
+void snd_pcm_runtime_set(struct snd_pcm_substream *substream,
+			 struct snd_pcm_hw_params *params)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	unsigned int bits;
+	snd_pcm_uframes_t frames;
+
+	runtime->access = params_access(params);
+	runtime->format = params_format(params);
+	runtime->subformat = params_subformat(params);
+	runtime->channels = params_channels(params);
+	runtime->rate = params_rate(params);
+	runtime->period_size = params_period_size(params);
+	runtime->periods = params_periods(params);
+	runtime->buffer_size = params_buffer_size(params);
+	runtime->info = params->info;
+	runtime->rate_num = params->rate_num;
+	runtime->rate_den = params->rate_den;
+	runtime->no_period_wakeup =
+			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
+			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
+
+	bits = snd_pcm_format_physical_width(runtime->format);
+	runtime->sample_bits = bits;
+	bits *= runtime->channels;
+	runtime->frame_bits = bits;
+	frames = 1;
+	while (bits % 8 != 0) {
+		bits *= 2;
+		frames *= 2;
+	}
+	runtime->byte_align = bits / 8;
+	runtime->min_align = frames;
+
+	/* Default sw params */
+	runtime->tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
+	runtime->period_step = 1;
+	runtime->control->avail_min = runtime->period_size;
+	runtime->start_threshold = 1;
+	runtime->stop_threshold = runtime->buffer_size;
+	runtime->silence_threshold = 0;
+	runtime->silence_size = 0;
+	runtime->boundary = runtime->buffer_size;
+	while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
+		runtime->boundary *= 2;
+
+	/* clear the buffer for avoiding possible kernel info leaks */
+	if (runtime->dma_area &&
+	    !(substream->ops && substream->ops->copy_user)) {
+		size_t size = runtime->dma_bytes;
+
+		if (runtime->info & SNDRV_PCM_INFO_MMAP)
+			size = PAGE_ALIGN(size);
+		memset(runtime->dma_area, 0, size);
+	}
+}
+EXPORT_SYMBOL(snd_pcm_runtime_set);
+
 static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *params)
 {
 	struct snd_pcm_runtime *runtime;
 	int err, usecs;
-	unsigned int bits;
-	snd_pcm_uframes_t frames;
 
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
@@ -715,53 +771,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 			goto _error;
 	}
 
-	runtime->access = params_access(params);
-	runtime->format = params_format(params);
-	runtime->subformat = params_subformat(params);
-	runtime->channels = params_channels(params);
-	runtime->rate = params_rate(params);
-	runtime->period_size = params_period_size(params);
-	runtime->periods = params_periods(params);
-	runtime->buffer_size = params_buffer_size(params);
-	runtime->info = params->info;
-	runtime->rate_num = params->rate_num;
-	runtime->rate_den = params->rate_den;
-	runtime->no_period_wakeup =
-			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
-			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
-
-	bits = snd_pcm_format_physical_width(runtime->format);
-	runtime->sample_bits = bits;
-	bits *= runtime->channels;
-	runtime->frame_bits = bits;
-	frames = 1;
-	while (bits % 8 != 0) {
-		bits *= 2;
-		frames *= 2;
-	}
-	runtime->byte_align = bits / 8;
-	runtime->min_align = frames;
-
-	/* Default sw params */
-	runtime->tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
-	runtime->period_step = 1;
-	runtime->control->avail_min = runtime->period_size;
-	runtime->start_threshold = 1;
-	runtime->stop_threshold = runtime->buffer_size;
-	runtime->silence_threshold = 0;
-	runtime->silence_size = 0;
-	runtime->boundary = runtime->buffer_size;
-	while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
-		runtime->boundary *= 2;
-
-	/* clear the buffer for avoiding possible kernel info leaks */
-	if (runtime->dma_area && !substream->ops->copy_user) {
-		size_t size = runtime->dma_bytes;
-
-		if (runtime->info & SNDRV_PCM_INFO_MMAP)
-			size = PAGE_ALIGN(size);
-		memset(runtime->dma_area, 0, size);
-	}
+	snd_pcm_runtime_set(substream, params);
 
 	snd_pcm_timer_resolution_change(substream);
 	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
-- 
2.27.0

[RFC PATCH 5/6] ASoC: soc-pcm: Create new snd_pcm_runtime for BE DAIs

[Codrin Ciubotariu]

The BE DAIs are different than the FE DAIs. They have different HW
capabilities, different HW constraints and different HW parameters. Also,
the buffer used to read/write data from/to user-space to/from FE DAIs has
nothing to do with the BE DAIs. For this reason, this patch creates a new
snd_pcm_runtime for the BE DAIs. The new structure can be used to better
represent the HW capabilities, so the HW parameters are no longer copied
from the FE, but created separately. For BE DAIs that need a buffer to move
the data to/from the FE DAIs (no dev-to-dev DMA capability), the new
snd_pcm_runtime can store the needed parameters to allocate the buffer and
set the DMA transfers.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 sound/soc/soc-pcm.c | 126 +++++++++++++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 36 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 8659089a87a0..7d95df20541e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1134,7 +1134,6 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
 
 	dpcm->be = be;
 	dpcm->fe = fe;
-	be->dpcm[stream].runtime = fe->dpcm[stream].runtime;
 	dpcm->state = SND_SOC_DPCM_LINK_STATE_NEW;
 	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
 	list_add(&dpcm->list_be, &fe->dpcm[stream].be_clients);
@@ -1150,34 +1149,6 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
 	return 1;
 }
 
-/* reparent a BE onto another FE */
-static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe,
-			struct snd_soc_pcm_runtime *be, int stream)
-{
-	struct snd_soc_dpcm *dpcm;
-	struct snd_pcm_substream *fe_substream, *be_substream;
-
-	/* reparent if BE is connected to other FEs */
-	if (!be->dpcm[stream].users)
-		return;
-
-	be_substream = snd_soc_dpcm_get_substream(be, stream);
-
-	for_each_dpcm_fe(be, stream, dpcm) {
-		if (dpcm->fe == fe)
-			continue;
-
-		dev_dbg(fe->dev, "reparent %s path %s %s %s\n",
-			stream ? "capture" : "playback",
-			dpcm->fe->dai_link->name,
-			stream ? "<-" : "->", dpcm->be->dai_link->name);
-
-		fe_substream = snd_soc_dpcm_get_substream(dpcm->fe, stream);
-		be_substream->runtime = fe_substream->runtime;
-		break;
-	}
-}
-
 /* disconnect a BE and FE */
 void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 {
@@ -1196,9 +1167,6 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 			stream ? "capture" : "playback", fe->dai_link->name,
 			stream ? "<-" : "->", dpcm->be->dai_link->name);
 
-		/* BEs still alive need new FE */
-		dpcm_be_reparent(fe, dpcm->be, stream);
-
 		dpcm_remove_debugfs_state(dpcm);
 
 		spin_lock_irqsave(&fe->card->dpcm_lock, flags);
@@ -1468,7 +1436,12 @@ void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
 		}
 
 		soc_pcm_close(be_substream);
-		be_substream->runtime = NULL;
+
+		if (be_substream->runtime) {
+			snd_pcm_runtime_free(be_substream->runtime);
+			be_substream->runtime = NULL;
+		}
+
 		be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
 	}
 }
@@ -1482,6 +1455,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 	/* only startup BE DAIs that are either sinks or sources to this FE DAI */
 	for_each_dpcm_be(fe, stream, dpcm) {
 		struct snd_pcm_substream *be_substream;
+		struct snd_pcm_runtime *runtime;
 
 		be = dpcm->be;
 		be_substream = snd_soc_dpcm_get_substream(be, stream);
@@ -1514,7 +1488,23 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 		dev_dbg(be->dev, "ASoC: open %s BE %s\n",
 			stream ? "capture" : "playback", be->dai_link->name);
 
-		be_substream->runtime = be->dpcm[stream].runtime;
+		runtime = snd_pcm_runtime_alloc();
+		if (!runtime) {
+			err = -ENOMEM;
+			goto unwind;
+		}
+
+		be_substream->runtime = runtime;
+
+		/* initialize the BE constraints */
+		err = snd_pcm_hw_constraints_init(be_substream);
+		if (err < 0) {
+			dev_err(be->dev,
+				"dpcm_be_hw_constraints_init failed: %d\n",
+				err);
+			goto unwind;
+		}
+
 		err = soc_pcm_open(be_substream);
 		if (err < 0) {
 			be->dpcm[stream].users--;
@@ -1527,6 +1517,14 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 			goto unwind;
 		}
 
+		err = snd_pcm_hw_constraints_complete(be_substream);
+		if (err < 0) {
+			dev_err(fe->dev,
+				"snd_pcm_hw_constraints_complete failed: %d\n",
+				err);
+			goto unwind;
+		}
+
 		be->dpcm[stream].state = SND_SOC_DPCM_STATE_OPEN;
 		count++;
 	}
@@ -1897,6 +1895,14 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
 		if (ret < 0)
 			goto unwind;
 
+		/* apply constrains */
+		dpcm->hw_params.rmask = ~0U;
+		ret = snd_pcm_hw_refine(be_substream, &dpcm->hw_params);
+		if (ret < 0) {
+			dev_err(fe->dev, "failed to refine hw parameters: %d\n", ret);
+			goto unwind;
+		}
+
 		/* copy the fixed-up hw params for BE dai */
 		memcpy(&be->dpcm[stream].hw_params, &dpcm->hw_params,
 		       sizeof(struct snd_pcm_hw_params));
@@ -1918,6 +1924,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
 			goto unwind;
 
 		be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
+		snd_pcm_runtime_set(be_substream, &dpcm->hw_params);
 	}
 	return 0;
 
@@ -1949,17 +1956,64 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
 	return ret;
 }
 
+static int dpcm_be_dai_hw_params_init(struct snd_soc_pcm_runtime *fe, int stream)
+{
+	struct snd_pcm_hw_params *params = &fe->dpcm[stream].hw_params;
+	int k;
+
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK;
+	     k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++)
+		snd_mask_any(hw_param_mask(params, k));
+
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL;
+	     k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++)
+		snd_interval_any(hw_param_interval(params, k));
+
+	return 0;
+}
+
 static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_pcm_hw_params *params)
 {
 	struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
 	int ret, stream = substream->stream;
+	struct snd_interval *t, *dpcm_t;
 
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
-	memcpy(&fe->dpcm[stream].hw_params, params,
-			sizeof(struct snd_pcm_hw_params));
+	/* initialize the BE HW params */
+	dpcm_be_dai_hw_params_init(fe, stream);
+
+	/* FIXME: a very low period time will make the CPU take too many
+	 * interrupts, which might end up not having enough time to actually
+	 * fill the buffer(s); for now, the BE min period time will be half of
+	 * the FE min period time
+	 */
+	t = hw_param_interval(params, SNDRV_PCM_HW_PARAM_PERIOD_TIME);
+	dpcm_t = hw_param_interval(&fe->dpcm[stream].hw_params,
+				   SNDRV_PCM_HW_PARAM_PERIOD_TIME);
+	dpcm_t->min = t->min / 2;
+
+	if (fe->dai_link->dpcm_merged_format) {
+		memcpy(hw_param_interval(&fe->dpcm[stream].hw_params,
+					 SNDRV_PCM_HW_PARAM_FORMAT),
+		       hw_param_interval(params, SNDRV_PCM_HW_PARAM_FORMAT),
+		       sizeof(struct snd_interval));
+	}
+	if (fe->dai_link->dpcm_merged_chan) {
+		memcpy(hw_param_interval(&fe->dpcm[stream].hw_params,
+					 SNDRV_PCM_HW_PARAM_CHANNELS),
+		       hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS),
+		       sizeof(struct snd_interval));
+	}
+	if (fe->dai_link->dpcm_merged_rate) {
+		memcpy(hw_param_interval(&fe->dpcm[stream].hw_params,
+					 SNDRV_PCM_HW_PARAM_RATE),
+		       hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE),
+		       sizeof(struct snd_interval));
+	}
+
 	ret = dpcm_be_dai_hw_params(fe, stream);
 	if (ret < 0)
 		goto out;
-- 
2.27.0

[RFC PATCH 6/6] ASoC: dmaengine: Allocate buffer if substream is unmanaged

[Codrin Ciubotariu]

pcm_construct/pcm_destruct callbacks are not called for BE DAIs. This means
that, if the generic dmaengine driver is used, there is no managed buffer
allocation (or pre-allocation). To be able to use the generic dmaengine
driver for BE DAI links, allocate the buffer in the hw_params callback if
there is no managed buffer.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 sound/soc/soc-generic-dmaengine-pcm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 9ef80a48707e..c915da71e1cd 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -98,6 +98,22 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component,
 			return ret;
 	}
 
+	if (!substream->managed_buffer_alloc) {
+		substream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_IRAM;
+		substream->dma_buffer.dev.dev = chan->device->dev;
+		return snd_pcm_lib_malloc_pages(substream,
+						params_buffer_bytes(params));
+	}
+
+	return 0;
+}
+
+static int dmaengine_pcm_hw_free(struct snd_soc_component *component,
+				 struct snd_pcm_substream *substream)
+{
+	if (!substream->managed_buffer_alloc)
+		return snd_pcm_lib_free_pages(substream);
+
 	return 0;
 }
 
@@ -332,6 +348,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,
@@ -343,6 +360,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component_process = {
 	.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,
 	.copy_user	= dmaengine_copy_user,
-- 
2.27.0

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Takashi Iwai]

On Wed, 19 May 2021 12:48:36 +0200,
Codrin Ciubotariu wrote:
> 
> This patchset adds a different snd_pcm_runtime in the BE's substream,
> replacing the FE's snd_pcm_runtime. With a different structure, the BE
> HW capabilities and constraints will no longer merge with the FE ones.
> This allows for error detection if the be_hw_params_fixup() applies HW
> parameters not supported by the BE DAIs. Also, it calculates values
> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
> period size, if needed.
> 
> The first 4 patches are preparatory patches, that just group and export
> functions used to allocate and initialize the snd_pcm_runtime. Also, the
> functions that set and apply the HW constraints are exported.
> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
> for BEs, which includes allocation, initializing the HW capabilities,
> HW constraints and HW parameters. The BE HW parameters are no longer
> copied from the FE. They are recalculated, based on HW capabilities,
> constraints and the be_hw_params_fixup() callback.
> The 6th and last patch basically adds support for the PCM generic
> dmaengine to be used as a platform driver for BE DAI links. It allocates
> a buffer, needed by the DMA transfers that do not support dev-to-dev
> transfers between FE and BE DAIs.
> 
> This is a superset of
> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
> which only handles the BE HW constraints. This patchset aims to be more
> complete, defining a a snd_pcm_runtime between each FE and BE and can
> be used between any DAI link connection. I am sure I am not handling all
> the needed members of snd_pcm_runtime (such as handling
> struct snd_pcm_mmap_status *status), but I would like to have your
> feedback regarding this idea.

I'm also concerned about the handling of other fields in runtime
object, maybe allocating a complete runtime object for each BE is an
overkill and fragile.  Could it be rather only hw_constraints to be
unique for each BE, instead?

Also, the last patch allows only IRAM type, which sounds already
doubtful.  The dmaengine code should be generic.

Last but not least, one minor nitpick: please use EXPORT_SYMBOL_GPL()
for the newly introduced symbols.


thanks,

Takashi

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Codrin.Ciubotariu]

On 19.05.2021 17:15, Takashi Iwai wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, 19 May 2021 12:48:36 +0200,
> Codrin Ciubotariu wrote:
>>
>> This patchset adds a different snd_pcm_runtime in the BE's substream,
>> replacing the FE's snd_pcm_runtime. With a different structure, the BE
>> HW capabilities and constraints will no longer merge with the FE ones.
>> This allows for error detection if the be_hw_params_fixup() applies HW
>> parameters not supported by the BE DAIs. Also, it calculates values
>> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
>> period size, if needed.
>>
>> The first 4 patches are preparatory patches, that just group and export
>> functions used to allocate and initialize the snd_pcm_runtime. Also, the
>> functions that set and apply the HW constraints are exported.
>> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
>> for BEs, which includes allocation, initializing the HW capabilities,
>> HW constraints and HW parameters. The BE HW parameters are no longer
>> copied from the FE. They are recalculated, based on HW capabilities,
>> constraints and the be_hw_params_fixup() callback.
>> The 6th and last patch basically adds support for the PCM generic
>> dmaengine to be used as a platform driver for BE DAI links. It allocates
>> a buffer, needed by the DMA transfers that do not support dev-to-dev
>> transfers between FE and BE DAIs.
>>
>> This is a superset of
>> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
>> which only handles the BE HW constraints. This patchset aims to be more
>> complete, defining a a snd_pcm_runtime between each FE and BE and can
>> be used between any DAI link connection. I am sure I am not handling all
>> the needed members of snd_pcm_runtime (such as handling
>> struct snd_pcm_mmap_status *status), but I would like to have your
>> feedback regarding this idea.
> 
> I'm also concerned about the handling of other fields in runtime
> object, maybe allocating a complete runtime object for each BE is an
> overkill and fragile.  Could it be rather only hw_constraints to be
> unique for each BE, instead?

I tried with only the hw constraints in the previous patchset and it's 
difficult to handle the snd_pcm_hw_rule_add() calls, without changing 
the function's declaration. This solution requires no changes to 
constraints API, nor to their 'clients'. I agree that handling all the 
runtime fields might be over-complicated. From what I see, the scary 
ones are used to describe the buffer and the status of the transfers. I 
do not think there are BEs that use these values at this moment (the FE 
ones). I think that the HW params, private section, hardware description 
and maybe DMA members (at least in my case) are mostly needed by BEs.

> Also, the last patch allows only IRAM type, which sounds already
> doubtful.  The dmaengine code should be generic.

dmaengine, when used with normal PCM, preallocates only IRAM buffers 
[1]. This BE buffer would only be needed if DMA dev-to-mem or mem-to-dev 
transfers are needed, between FE and BE. I agree that it could be 
handled differently, I added it here mostly to express my goal, which is 
to use the generic dmaengine for BEs. My DMA has no dev-to-dev DMA 
capability, so I need a buffer to move the data between FE and BE.

> 
> Last but not least, one minor nitpick: please use EXPORT_SYMBOL_GPL()
> for the newly introduced symbols.

Sure, this is an oversight on my side. I will make all of them 
EXPORT_SYMBOL_GPL.

Thank you very much for your review!

Best regards,
Codrin


[1] 
https://elixir.bootlin.com/linux/v5.13-rc2/source/sound/soc/soc-generic-dmaengine-pcm.c#L266

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Takashi Iwai]

On Wed, 19 May 2021 17:08:10 +0200,
<Codrin.Ciubotariu@microchip.com> wrote:
> 
> On 19.05.2021 17:15, Takashi Iwai wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, 19 May 2021 12:48:36 +0200,
> > Codrin Ciubotariu wrote:
> >>
> >> This patchset adds a different snd_pcm_runtime in the BE's substream,
> >> replacing the FE's snd_pcm_runtime. With a different structure, the BE
> >> HW capabilities and constraints will no longer merge with the FE ones.
> >> This allows for error detection if the be_hw_params_fixup() applies HW
> >> parameters not supported by the BE DAIs. Also, it calculates values
> >> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
> >> period size, if needed.
> >>
> >> The first 4 patches are preparatory patches, that just group and export
> >> functions used to allocate and initialize the snd_pcm_runtime. Also, the
> >> functions that set and apply the HW constraints are exported.
> >> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
> >> for BEs, which includes allocation, initializing the HW capabilities,
> >> HW constraints and HW parameters. The BE HW parameters are no longer
> >> copied from the FE. They are recalculated, based on HW capabilities,
> >> constraints and the be_hw_params_fixup() callback.
> >> The 6th and last patch basically adds support for the PCM generic
> >> dmaengine to be used as a platform driver for BE DAI links. It allocates
> >> a buffer, needed by the DMA transfers that do not support dev-to-dev
> >> transfers between FE and BE DAIs.
> >>
> >> This is a superset of
> >> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
> >> which only handles the BE HW constraints. This patchset aims to be more
> >> complete, defining a a snd_pcm_runtime between each FE and BE and can
> >> be used between any DAI link connection. I am sure I am not handling all
> >> the needed members of snd_pcm_runtime (such as handling
> >> struct snd_pcm_mmap_status *status), but I would like to have your
> >> feedback regarding this idea.
> > 
> > I'm also concerned about the handling of other fields in runtime
> > object, maybe allocating a complete runtime object for each BE is an
> > overkill and fragile.  Could it be rather only hw_constraints to be
> > unique for each BE, instead?
> 
> I tried with only the hw constraints in the previous patchset and it's 
> difficult to handle the snd_pcm_hw_rule_add() calls, without changing 
> the function's declaration. This solution requires no changes to 
> constraints API, nor to their 'clients'. I agree that handling all the 
> runtime fields might be over-complicated. From what I see, the scary 
> ones are used to describe the buffer and the status of the transfers. I 
> do not think there are BEs that use these values at this moment (the FE 
> ones). I think that the HW params, private section, hardware description 
> and maybe DMA members (at least in my case) are mostly needed by BEs.

OK, I'll check your previous series again, but my gut feeling is for
pursuit to the hw_constraints hacks.  e.g. we may split
snd_pcm_hw_constraints and snd_pcm_hw_rule, too, if that matters.

> > Also, the last patch allows only IRAM type, which sounds already
> > doubtful.  The dmaengine code should be generic.
> 
> dmaengine, when used with normal PCM, preallocates only IRAM buffers 
> [1]. This BE buffer would only be needed if DMA dev-to-mem or mem-to-dev 
> transfers are needed, between FE and BE. I agree that it could be 
> handled differently, I added it here mostly to express my goal, which is 
> to use the generic dmaengine for BEs. My DMA has no dev-to-dev DMA 
> capability, so I need a buffer to move the data between FE and BE.

Ah, right, I overlooked that part...  It's confusing.  Maybe it's in
that style just because it falls back to SNDRV_DMA_TYPE_DEV?
It's another discussion, in anyway.


thanks,

Takashi

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Codrin.Ciubotariu]

On 19.05.2021 18:41, Takashi Iwai wrote:
> On Wed, 19 May 2021 17:08:10 +0200,
> <Codrin.Ciubotariu@microchip.com> wrote:
>>
>> On 19.05.2021 17:15, Takashi Iwai wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, 19 May 2021 12:48:36 +0200,
>>> Codrin Ciubotariu wrote:
>>>>
>>>> This patchset adds a different snd_pcm_runtime in the BE's substream,
>>>> replacing the FE's snd_pcm_runtime. With a different structure, the BE
>>>> HW capabilities and constraints will no longer merge with the FE ones.
>>>> This allows for error detection if the be_hw_params_fixup() applies HW
>>>> parameters not supported by the BE DAIs. Also, it calculates values
>>>> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
>>>> period size, if needed.
>>>>
>>>> The first 4 patches are preparatory patches, that just group and export
>>>> functions used to allocate and initialize the snd_pcm_runtime. Also, the
>>>> functions that set and apply the HW constraints are exported.
>>>> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
>>>> for BEs, which includes allocation, initializing the HW capabilities,
>>>> HW constraints and HW parameters. The BE HW parameters are no longer
>>>> copied from the FE. They are recalculated, based on HW capabilities,
>>>> constraints and the be_hw_params_fixup() callback.
>>>> The 6th and last patch basically adds support for the PCM generic
>>>> dmaengine to be used as a platform driver for BE DAI links. It allocates
>>>> a buffer, needed by the DMA transfers that do not support dev-to-dev
>>>> transfers between FE and BE DAIs.
>>>>
>>>> This is a superset of
>>>> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
>>>> which only handles the BE HW constraints. This patchset aims to be more
>>>> complete, defining a a snd_pcm_runtime between each FE and BE and can
>>>> be used between any DAI link connection. I am sure I am not handling all
>>>> the needed members of snd_pcm_runtime (such as handling
>>>> struct snd_pcm_mmap_status *status), but I would like to have your
>>>> feedback regarding this idea.
>>>
>>> I'm also concerned about the handling of other fields in runtime
>>> object, maybe allocating a complete runtime object for each BE is an
>>> overkill and fragile.  Could it be rather only hw_constraints to be
>>> unique for each BE, instead?
>>
>> I tried with only the hw constraints in the previous patchset and it's
>> difficult to handle the snd_pcm_hw_rule_add() calls, without changing
>> the function's declaration. This solution requires no changes to
>> constraints API, nor to their 'clients'. I agree that handling all the
>> runtime fields might be over-complicated. From what I see, the scary
>> ones are used to describe the buffer and the status of the transfers. I
>> do not think there are BEs that use these values at this moment (the FE
>> ones). I think that the HW params, private section, hardware description
>> and maybe DMA members (at least in my case) are mostly needed by BEs.
> 
> OK, I'll check your previous series again, but my gut feeling is for
> pursuit to the hw_constraints hacks.  e.g. we may split
> snd_pcm_hw_constraints and snd_pcm_hw_rule, too, if that matters.

Something like adding snd_pcm_hw_rule directly under
snd_pcm_runtime, to store the BE constraints? It could work, but I think 
we should also be able to remove rules, if one BE gets disconnected. 
This means that we will need a way to identify or separate them, for 
each BE, right?

> 
>>> Also, the last patch allows only IRAM type, which sounds already
>>> doubtful.  The dmaengine code should be generic.
>>
>> dmaengine, when used with normal PCM, preallocates only IRAM buffers
>> [1]. This BE buffer would only be needed if DMA dev-to-mem or mem-to-dev
>> transfers are needed, between FE and BE. I agree that it could be
>> handled differently, I added it here mostly to express my goal, which is
>> to use the generic dmaengine for BEs. My DMA has no dev-to-dev DMA
>> capability, so I need a buffer to move the data between FE and BE.
> 
> Ah, right, I overlooked that part...  It's confusing.  Maybe it's in
> that style just because it falls back to SNDRV_DMA_TYPE_DEV?
> It's another discussion, in anyway.

Right, it falls back to SNDRV_DMA_TYPE_DEV. [1]

Thanks and best regards,
Codrin

[1] 
https://elixir.bootlin.com/linux/v5.13-rc2/source/sound/core/memalloc.c#L154

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Takashi Iwai]

On Thu, 20 May 2021 15:59:02 +0200,
<Codrin.Ciubotariu@microchip.com> wrote:
> 
> On 19.05.2021 18:41, Takashi Iwai wrote:
> > On Wed, 19 May 2021 17:08:10 +0200,
> > <Codrin.Ciubotariu@microchip.com> wrote:
> >>
> >> On 19.05.2021 17:15, Takashi Iwai wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Wed, 19 May 2021 12:48:36 +0200,
> >>> Codrin Ciubotariu wrote:
> >>>>
> >>>> This patchset adds a different snd_pcm_runtime in the BE's substream,
> >>>> replacing the FE's snd_pcm_runtime. With a different structure, the BE
> >>>> HW capabilities and constraints will no longer merge with the FE ones.
> >>>> This allows for error detection if the be_hw_params_fixup() applies HW
> >>>> parameters not supported by the BE DAIs. Also, it calculates values
> >>>> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
> >>>> period size, if needed.
> >>>>
> >>>> The first 4 patches are preparatory patches, that just group and export
> >>>> functions used to allocate and initialize the snd_pcm_runtime. Also, the
> >>>> functions that set and apply the HW constraints are exported.
> >>>> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
> >>>> for BEs, which includes allocation, initializing the HW capabilities,
> >>>> HW constraints and HW parameters. The BE HW parameters are no longer
> >>>> copied from the FE. They are recalculated, based on HW capabilities,
> >>>> constraints and the be_hw_params_fixup() callback.
> >>>> The 6th and last patch basically adds support for the PCM generic
> >>>> dmaengine to be used as a platform driver for BE DAI links. It allocates
> >>>> a buffer, needed by the DMA transfers that do not support dev-to-dev
> >>>> transfers between FE and BE DAIs.
> >>>>
> >>>> This is a superset of
> >>>> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
> >>>> which only handles the BE HW constraints. This patchset aims to be more
> >>>> complete, defining a a snd_pcm_runtime between each FE and BE and can
> >>>> be used between any DAI link connection. I am sure I am not handling all
> >>>> the needed members of snd_pcm_runtime (such as handling
> >>>> struct snd_pcm_mmap_status *status), but I would like to have your
> >>>> feedback regarding this idea.
> >>>
> >>> I'm also concerned about the handling of other fields in runtime
> >>> object, maybe allocating a complete runtime object for each BE is an
> >>> overkill and fragile.  Could it be rather only hw_constraints to be
> >>> unique for each BE, instead?
> >>
> >> I tried with only the hw constraints in the previous patchset and it's
> >> difficult to handle the snd_pcm_hw_rule_add() calls, without changing
> >> the function's declaration. This solution requires no changes to
> >> constraints API, nor to their 'clients'. I agree that handling all the
> >> runtime fields might be over-complicated. From what I see, the scary
> >> ones are used to describe the buffer and the status of the transfers. I
> >> do not think there are BEs that use these values at this moment (the FE
> >> ones). I think that the HW params, private section, hardware description
> >> and maybe DMA members (at least in my case) are mostly needed by BEs.
> > 
> > OK, I'll check your previous series again, but my gut feeling is for
> > pursuit to the hw_constraints hacks.  e.g. we may split
> > snd_pcm_hw_constraints and snd_pcm_hw_rule, too, if that matters.
> 
> Something like adding snd_pcm_hw_rule directly under
> snd_pcm_runtime, to store the BE constraints? It could work, but I think 
> we should also be able to remove rules, if one BE gets disconnected. 
> This means that we will need a way to identify or separate them, for 
> each BE, right?

Well, if each BE needs a different set of hw constraint rules, it
needs its own unique copies instead of sharing the rules.  Is it your
requirement?


Takashi

Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[Codrin.Ciubotariu]

On 21.05.2021 17:26, Takashi Iwai wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 20 May 2021 15:59:02 +0200,
> <Codrin.Ciubotariu@microchip.com> wrote:
>>
>> On 19.05.2021 18:41, Takashi Iwai wrote:
>>> On Wed, 19 May 2021 17:08:10 +0200,
>>> <Codrin.Ciubotariu@microchip.com> wrote:
>>>>
>>>> On 19.05.2021 17:15, Takashi Iwai wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Wed, 19 May 2021 12:48:36 +0200,
>>>>> Codrin Ciubotariu wrote:
>>>>>>
>>>>>> This patchset adds a different snd_pcm_runtime in the BE's substream,
>>>>>> replacing the FE's snd_pcm_runtime. With a different structure, the BE
>>>>>> HW capabilities and constraints will no longer merge with the FE ones.
>>>>>> This allows for error detection if the be_hw_params_fixup() applies HW
>>>>>> parameters not supported by the BE DAIs. Also, it calculates values
>>>>>> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
>>>>>> period size, if needed.
>>>>>>
>>>>>> The first 4 patches are preparatory patches, that just group and export
>>>>>> functions used to allocate and initialize the snd_pcm_runtime. Also, the
>>>>>> functions that set and apply the HW constraints are exported.
>>>>>> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
>>>>>> for BEs, which includes allocation, initializing the HW capabilities,
>>>>>> HW constraints and HW parameters. The BE HW parameters are no longer
>>>>>> copied from the FE. They are recalculated, based on HW capabilities,
>>>>>> constraints and the be_hw_params_fixup() callback.
>>>>>> The 6th and last patch basically adds support for the PCM generic
>>>>>> dmaengine to be used as a platform driver for BE DAI links. It allocates
>>>>>> a buffer, needed by the DMA transfers that do not support dev-to-dev
>>>>>> transfers between FE and BE DAIs.
>>>>>>
>>>>>> This is a superset of
>>>>>> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
>>>>>> which only handles the BE HW constraints. This patchset aims to be more
>>>>>> complete, defining a a snd_pcm_runtime between each FE and BE and can
>>>>>> be used between any DAI link connection. I am sure I am not handling all
>>>>>> the needed members of snd_pcm_runtime (such as handling
>>>>>> struct snd_pcm_mmap_status *status), but I would like to have your
>>>>>> feedback regarding this idea.
>>>>>
>>>>> I'm also concerned about the handling of other fields in runtime
>>>>> object, maybe allocating a complete runtime object for each BE is an
>>>>> overkill and fragile.  Could it be rather only hw_constraints to be
>>>>> unique for each BE, instead?
>>>>
>>>> I tried with only the hw constraints in the previous patchset and it's
>>>> difficult to handle the snd_pcm_hw_rule_add() calls, without changing
>>>> the function's declaration. This solution requires no changes to
>>>> constraints API, nor to their 'clients'. I agree that handling all the
>>>> runtime fields might be over-complicated. From what I see, the scary
>>>> ones are used to describe the buffer and the status of the transfers. I
>>>> do not think there are BEs that use these values at this moment (the FE
>>>> ones). I think that the HW params, private section, hardware description
>>>> and maybe DMA members (at least in my case) are mostly needed by BEs.
>>>
>>> OK, I'll check your previous series again, but my gut feeling is for
>>> pursuit to the hw_constraints hacks.  e.g. we may split
>>> snd_pcm_hw_constraints and snd_pcm_hw_rule, too, if that matters.
>>
>> Something like adding snd_pcm_hw_rule directly under
>> snd_pcm_runtime, to store the BE constraints? It could work, but I think
>> we should also be able to remove rules, if one BE gets disconnected.
>> This means that we will need a way to identify or separate them, for
>> each BE, right?
> 
> Well, if each BE needs a different set of hw constraint rules, it
> needs its own unique copies instead of sharing the rules.  Is it your
> requirement?

Yes, I am thinking that the constraints should be grouped for one BE, 
since the HW parameters are the same for all the DAIs(CPUs and codecs) 
in a BE.
Also, I think that the separation of snd_pcm_hardware is important. From 
what I understand now, the BE CPUs snd_soc_pcm_stream structures are 
ignored entirely and the codecs snd_soc_pcm_stream structures are 
considered only if fe->dai_link->dpcm_merged_* flags are set [1]. This 
means that be_hw_params_fixup() callback (BE HW params) is not affected 
by the BE drivers' snd_soc_pcm_stream. We are relying on the fact that 
developers know what capabilities the BE DAIs have, in order to set 
be_hw_params_fixup. Even though HW capabilities are subject to change if 
HW bugs are discovered... Am I making sense?
I still prefer the separate snd_pcm_runtime, since, for my case, I am 
able to set different runtime->(rate, channels, subformat, frame_bits, 
sample_bits, period/buffer_size, etc.) and later use bytes_to_* and 
*_to_bytes() callbacks freely, but if this step is too big, maybe we can 
split it up and start with something we can all agree on the HW constraints.

Thank you for your time!

Best regards,
Codrin

[1] 
https://elixir.bootlin.com/linux/v5.13-rc3/source/sound/soc/soc-pcm.c#L1768