| Message ID | 20230214161435.1088246-2-claudiu.beznea@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: mchp-pdmc: fix poc noises when starting capture | expand |
On 14.02.2023 23:26, Mark Brown wrote: > On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >> On 2/14/23 08:14, Claudiu Beznea wrote: >>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>> else >>> driver = &dmaengine_pcm_component; >>> + driver->start_dma_last = config->start_dma_last; >> This will break if you have multiple sound cards in the system. >> dmaengine_pcm_component must stay const. > Right, if we need to modify it we either need to select which of > multiple const structs to register or to take a copy and modify > that. I've not looked at the actual changes yet. OK, I will try that and return with a new patch. On the other hand do you think the other solution presented in cover letter would be better? From the cover letter: "The other solution that was identified for this was to extend the already existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside of this was that a potential struct snd_soc_dai_link::start_dma_last would have to be populated on sound card driver thus, had to be taken into account in all sound card drivers. At the moment, the mchp-pdmc is used only with simple-audio-card. In case of simple-audio-card a new DT binding would had to be introduced to specify this action on dai-link descriptions (as of my investigation)." Thank you, Claudiu
On 2/16/23 01:49, Claudiu.Beznea@microchip.com wrote: > On 14.02.2023 23:26, Mark Brown wrote: >> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >>> On 2/14/23 08:14, Claudiu Beznea wrote: >>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>>> else >>>> driver = &dmaengine_pcm_component; >>>> + driver->start_dma_last = config->start_dma_last; >>> This will break if you have multiple sound cards in the system. >>> dmaengine_pcm_component must stay const. >> Right, if we need to modify it we either need to select which of >> multiple const structs to register or to take a copy and modify >> that. I've not looked at the actual changes yet. > OK, I will try that and return with a new patch. > > On the other hand do you think the other solution presented in cover letter > would be better? From the cover letter: > > "The other solution that was identified for this was to extend the already > existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside > of this was that a potential struct snd_soc_dai_link::start_dma_last > would have to be populated on sound card driver thus, had to be taken > into account in all sound card drivers. At the moment, the mchp-pdmc is > used only with simple-audio-card. In case of simple-audio-card a new DT > binding would had to be introduced to specify this action on dai-link > descriptions (as of my investigation)." > Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In your code you iterate over all the components of the link and if any of them has it set the DMA is started last.
On 16.02.2023 15:53, Lars-Peter Clausen wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 2/16/23 01:49, Claudiu.Beznea@microchip.com wrote: >> On 14.02.2023 23:26, Mark Brown wrote: >>> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >>>> On 2/14/23 08:14, Claudiu Beznea wrote: >>>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>>>> else >>>>> driver = &dmaengine_pcm_component; >>>>> + driver->start_dma_last = config->start_dma_last; >>>> This will break if you have multiple sound cards in the system. >>>> dmaengine_pcm_component must stay const. >>> Right, if we need to modify it we either need to select which of >>> multiple const structs to register or to take a copy and modify >>> that. I've not looked at the actual changes yet. >> OK, I will try that and return with a new patch. >> >> On the other hand do you think the other solution presented in cover letter >> would be better? From the cover letter: >> >> "The other solution that was identified for this was to extend the already >> existing mechanism around struct snd_soc_dai_link::stop_dma_first. The >> downside >> of this was that a potential struct snd_soc_dai_link::start_dma_last >> would have to be populated on sound card driver thus, had to be taken >> into account in all sound card drivers. At the moment, the mchp-pdmc is >> used only with simple-audio-card. In case of simple-audio-card a new DT >> binding would had to be introduced to specify this action on dai-link >> descriptions (as of my investigation)." >> > Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In > your code you iterate over all the components of the link and if any of > them has it set the DMA is started last. Yes, that is also working. In this patch I chose to have it on DMA component as the operation is specific to DMA... It looked better to me this way. But is true that having it on mchp_pdmc_dai_component wouldn't affect the behavior. Thank you, Claudiu
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index 2df54cf02cb3..4e3dcb94576d 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -149,6 +149,7 @@ struct snd_dmaengine_pcm_config { const struct snd_pcm_hardware *pcm_hardware; unsigned int prealloc_buffer_size; + unsigned int start_dma_last; }; int snd_dmaengine_pcm_register(struct device *dev, diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 3203d35bc8c1..0814ed143864 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -190,6 +190,8 @@ struct snd_soc_component_driver { bool use_dai_pcm_id; /* use DAI link PCM ID as PCM device number */ int be_pcm_base; /* base device ID for all BE PCMs */ + unsigned int start_dma_last; + #ifdef CONFIG_DEBUG_FS const char *debugfs_prefix; #endif diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 3b99f619e37e..264e87af6b58 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -318,7 +318,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component, return 0; } -static const struct snd_soc_component_driver dmaengine_pcm_component = { +static struct snd_soc_component_driver dmaengine_pcm_component = { .name = SND_DMAENGINE_PCM_DRV_NAME, .probe_order = SND_SOC_COMP_ORDER_LATE, .open = dmaengine_pcm_open, @@ -329,7 +329,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component = { .pcm_construct = dmaengine_pcm_new, }; -static const struct snd_soc_component_driver dmaengine_pcm_component_process = { +static struct snd_soc_component_driver dmaengine_pcm_component_process = { .name = SND_DMAENGINE_PCM_DRV_NAME, .probe_order = SND_SOC_COMP_ORDER_LATE, .open = dmaengine_pcm_open, @@ -425,7 +425,7 @@ static const struct snd_dmaengine_pcm_config snd_dmaengine_pcm_default_config = int snd_dmaengine_pcm_register(struct device *dev, const struct snd_dmaengine_pcm_config *config, unsigned int flags) { - const struct snd_soc_component_driver *driver; + struct snd_soc_component_driver *driver; struct dmaengine_pcm *pcm; int ret; @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, else driver = &dmaengine_pcm_component; + driver->start_dma_last = config->start_dma_last; + ret = snd_soc_component_initialize(&pcm->component, driver, dev); if (ret) goto err_free_dma; diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 005b179a770a..ec429b93a4ee 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1088,22 +1088,39 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); - int ret = -EINVAL, _ret = 0; + struct snd_soc_component *component; + int ret = -EINVAL, _ret = 0, start_dma_last = 0, i; int rollback = 0; switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + /* Do we need to start dma first? */ + for_each_rtd_components(rtd, i, component) { + if (component->driver->start_dma_last) { + start_dma_last = 1; + break; + } + } + ret = snd_soc_link_trigger(substream, cmd, 0); if (ret < 0) goto start_err; - ret = snd_soc_pcm_component_trigger(substream, cmd, 0); - if (ret < 0) - goto start_err; + if (start_dma_last) { + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); + if (ret < 0) + goto start_err; + + ret = snd_soc_pcm_component_trigger(substream, cmd, 0); + } else { + ret = snd_soc_pcm_component_trigger(substream, cmd, 0); + if (ret < 0) + goto start_err; - ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); + } start_err: if (ret < 0) rollback = 1;
Add option to start DMA component after DAI trigger. This is done by filling the new struct snd_dmaengine_pcm_config::start_dma_last. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- include/sound/dmaengine_pcm.h | 1 + include/sound/soc-component.h | 2 ++ sound/soc/soc-generic-dmaengine-pcm.c | 8 +++++--- sound/soc/soc-pcm.c | 27 ++++++++++++++++++++++----- 4 files changed, 30 insertions(+), 8 deletions(-)