[RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs
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(-)
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--;
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) {
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;
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);
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;
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,
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
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-dmae...
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
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
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
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
participants (3)
-
Codrin Ciubotariu -
Codrin.Ciubotariu＠microchip.com
-
Takashi Iwai