Re: [PATCH] ASoC: meson: Use managed DMA buffer allocation

[Jerome Brunet <jbrunet@baylibre.com>]

On Fri 18 Dec 2020 at 18:41, Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 12/18/20 5:28 PM, Jerome Brunet wrote:
>> On Fri 18 Dec 2020 at 16:45, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> Using a managed buffer will pre-allocate the buffer using
>>> snd_pcm_lib_preallocate_pages() and automatically free it when the PCM is
>>> destroyed.
>>>
>>> In addition it will call snd_pcm_lib_malloc_pages() before the driver's
>>> hw_params() callback and snd_pcm_lib_free_pages() after the driver's
>>> hw_free() callback.
>>>
>>> This slightly reduces the boilerplate code of the driver.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>>   sound/soc/meson/aiu-fifo-i2s.c   |  1 -
>>>   sound/soc/meson/aiu-fifo-spdif.c |  1 -
>>>   sound/soc/meson/aiu-fifo.c       | 18 ++----------------
>>>   3 files changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/sound/soc/meson/aiu-fifo-i2s.c b/sound/soc/meson/aiu-fifo-i2s.c
>>> index d91b0d874342..2388a2d0b3a6 100644
>>> --- a/sound/soc/meson/aiu-fifo-i2s.c
>>> +++ b/sound/soc/meson/aiu-fifo-i2s.c
>>> @@ -124,7 +124,6 @@ const struct snd_soc_dai_ops aiu_fifo_i2s_dai_ops = {
>>>   	.trigger	= aiu_fifo_i2s_trigger,
>>>   	.prepare	= aiu_fifo_i2s_prepare,
>>>   	.hw_params	= aiu_fifo_i2s_hw_params,
>>> -	.hw_free	= aiu_fifo_hw_free,
>>>   	.startup	= aiu_fifo_startup,
>>>   	.shutdown	= aiu_fifo_shutdown,
>>>   };
>>> diff --git a/sound/soc/meson/aiu-fifo-spdif.c b/sound/soc/meson/aiu-fifo-spdif.c
>>> index 44eb6faacf44..2fb30f89bf7a 100644
>>> --- a/sound/soc/meson/aiu-fifo-spdif.c
>>> +++ b/sound/soc/meson/aiu-fifo-spdif.c
>>> @@ -158,7 +158,6 @@ const struct snd_soc_dai_ops aiu_fifo_spdif_dai_ops = {
>>>   	.trigger	= fifo_spdif_trigger,
>>>   	.prepare	= fifo_spdif_prepare,
>>>   	.hw_params	= fifo_spdif_hw_params,
>>> -	.hw_free	= aiu_fifo_hw_free,
>>>   	.startup	= aiu_fifo_startup,
>>>   	.shutdown	= aiu_fifo_shutdown,
>>>   };
>>> diff --git a/sound/soc/meson/aiu-fifo.c b/sound/soc/meson/aiu-fifo.c
>>> index aa88aae8e517..4ad23267cace 100644
>>> --- a/sound/soc/meson/aiu-fifo.c
>>> +++ b/sound/soc/meson/aiu-fifo.c
>>> @@ -99,11 +99,6 @@ int aiu_fifo_hw_params(struct snd_pcm_substream *substream,
>>>   	struct snd_soc_component *component = dai->component;
>>>   	struct aiu_fifo *fifo = dai->playback_dma_data;
>>>   	dma_addr_t end;
>>> -	int ret;
>>> -
>>> -	ret = snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
>>> -	if (ret < 0)
>>> -		return ret;
>>>     	/* Setup the fifo boundaries */
>>>   	end = runtime->dma_addr + runtime->dma_bytes - fifo->fifo_block;
>>> @@ -124,12 +119,6 @@ int aiu_fifo_hw_params(struct snd_pcm_substream *substream,
>>>   	return 0;
>>>   }
>>>   -int aiu_fifo_hw_free(struct snd_pcm_substream *substream,
>>> -		     struct snd_soc_dai *dai)
>>> -{
>>> -	return snd_pcm_lib_free_pages(substream);
>>> -}
>>> -
>>>   static irqreturn_t aiu_fifo_isr(int irq, void *dev_id)
>>>   {
>>>   	struct snd_pcm_substream *playback = dev_id;
>>> @@ -187,15 +176,12 @@ void aiu_fifo_shutdown(struct snd_pcm_substream *substream,
>>>   int aiu_fifo_pcm_new(struct snd_soc_pcm_runtime *rtd,
>>>   		     struct snd_soc_dai *dai)
>>>   {
>>> -	struct snd_pcm_substream *substream =
>>> -		rtd->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
>>>   	struct snd_card *card = rtd->card->snd_card;
>>>   	struct aiu_fifo *fifo = dai->playback_dma_data;
>>>   	size_t size = fifo->pcm->buffer_bytes_max;
>>>   -	snd_pcm_lib_preallocate_pages(substream,
>>> -				      SNDRV_DMA_TYPE_DEV,
>>> -				      card->dev, size, size);
>>> +	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
>>> +				       card->dev, size, size);
>> Hi Lars-Peter,
>>
>> These FIFOs only do playback so to avoid wasting memory
>> s/snd_pcm_set_managed_buffer_all/snd_pcm_set_managed_buffer ?
>
> snd_pcm_set_managed_buffer_all() will skip substreams that do not
> exist. E.g. if the there is not capture support it wont allocate
> memory for it.

Indeed, Thanks !

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
Tested-by: Jerome Brunet <jbrunet@baylibre.com>

>
> To keep things simple I prefer snd_pcm_set_managed_buffer_all(). snd_pcm_set_managed_buffer() only makes sense if you have a different DMA device for capture and playback or you want different buffer sizes.


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic