Re: [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue

[Keyon Jie <yang.jie@xxxxxxxxxxxxxxx>]

On 2020/1/19 下午5:04, Takashi Iwai wrote:
> On Sun, 19 Jan 2020 09:11:17 +0100,
> Keyon Jie wrote:
> > On 2020/1/19 下午3:09, Takashi Iwai wrote:
> > It varies for each stream, most of them are 65536 Bytes only, whereas
> > one for Wake-On-Voice might need a > 4 Seconds buffer could be up to
> > about 1~2MBytes, and another one for deep-buffer playback can be up to
> > about 8MBytes.
> Hm, so this varies so much depending on the use case?
> I thought it comes from the topology file and it's essentially
> consistent over various purposes.

Yes, we add different buffer_bytes_max limitation to each stream 
depending on its use case, basically we set it to the maximum value we 
claim to support only, we don't want to waste any of the system memory.

> > > I think we can go for passing zero as default, which means skipping
> > > preallocation.  In addition, we may add an upper limit of the total
> > Just did an experiment and this works for me, I believe we still need
> > to call snd_pcm_set_managed_buffer() though the preallocation is
> > skipped in this, right?
> No, snd_pcm_set_managed_buffer() is the new PCM preallocation API.
> The old snd_pcm_lib_preallocate*() is almost gone.

What I asked is actually that since the preallocation will be 
skipped(with passing size=0), can we just not calling 
snd_pcm_set_managed_buffer() or snd_pcm_lib_preallocate*() in our SOF 
PCM driver? I believe no(we still need the invoking to do initialization 
except buffer allocating)?

> > > amount of allocation per card, controlled in pcm_memory.c, for
> > > example.  This logic can be applied to the legacy HDA, too.
> > >
> > > This should be relatively easy, and I'll provide the patch in the next
> > > week.
> > OK, that's fine for me also, thank you.
> Below is a quick hack for HDA.  We still need the certain amount of
> preallocation for non-x86 systems that don't support SG-buffers, so
> a bit of trick is applied to Kconfig.
>
> Totally untested, as usual.

Did a quick test(plus passing 0 size for preallocate in SOF PCM driver) 
and it works for my use case(no regression comparing that without 
applying this patch), Thank you.

Thanks,
~Keyon

> thanks,
>
> Takashi
>
> ---
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 0e14b7a3e67b..ac8b692b69b4 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -120,6 +120,9 @@ struct snd_card {
>   	int sync_irq;			/* assigned irq, used for PCM sync */
>   	wait_queue_head_t remove_sleep;
>   
> +	size_t total_pcm_alloc_bytes;	/* total amount of allocated buffers */
> +	struct mutex memory_mutex;	/* protection for the above */
> +
>   #ifdef CONFIG_PM
>   	unsigned int power_state;	/* power state */
>   	wait_queue_head_t power_sleep;
> diff --git a/sound/core/init.c b/sound/core/init.c
> index faa9f03c01ca..b02a99766351 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -211,6 +211,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
>   	INIT_LIST_HEAD(&card->ctl_files);
>   	spin_lock_init(&card->files_lock);
>   	INIT_LIST_HEAD(&card->files_list);
> +	mutex_init(&card->memory_mutex);
>   #ifdef CONFIG_PM
>   	init_waitqueue_head(&card->power_sleep);
>   #endif
> diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
> index d4702cc1d376..4883b0ccd475 100644
> --- a/sound/core/pcm_memory.c
> +++ b/sound/core/pcm_memory.c
> @@ -27,6 +27,37 @@ MODULE_PARM_DESC(maximum_substreams, "Maximum substreams with preallocated DMA m
>   
>   static const size_t snd_minimum_buffer = 16384;
>   
> +static unsigned long max_alloc_per_card = 32UL * 1024UL * 1024UL * 1024UL;
> +module_param(max_alloc_per_card, ulong, 0644);
> +MODULE_PARM_DESC(max_alloc_per_card, "Max total allocation bytes per card.");
> +
> +static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
> +			  size_t size, struct snd_dma_buffer *dmab)
> +{
> +	int err;
> +
> +	if (card->total_pcm_alloc_bytes + size > max_alloc_per_card)
> +		return -ENOMEM;
> +	err = snd_dma_alloc_pages(type, dev, size, dmab);
> +	if (!err) {
> +		mutex_lock(&card->memory_mutex);
> +		card->total_pcm_alloc_bytes += dmab->bytes;
> +		mutex_unlock(&card->memory_mutex);
> +	}
> +	return err;
> +}
> +
> +static void do_free_pages(struct snd_card *card, struct snd_dma_buffer *dmab)
> +{
> +	if (!dmab->area)
> +		return;
> +	mutex_lock(&card->memory_mutex);
> +	WARN_ON(card->total_pcm_alloc_bytes < dmab->bytes);
> +	card->total_pcm_alloc_bytes -= dmab->bytes;
> +	mutex_unlock(&card->memory_mutex);
> +	snd_dma_free_pages(dmab);
> +	dmab->area = NULL;
> +}
>   
>   /*
>    * try to allocate as the large pages as possible.
> @@ -37,16 +68,15 @@ static const size_t snd_minimum_buffer = 16384;
>   static int preallocate_pcm_pages(struct snd_pcm_substream *substream, size_t size)
>   {
>   	struct snd_dma_buffer *dmab = &substream->dma_buffer;
> +	struct snd_card *card = substream->pcm->card;
>   	size_t orig_size = size;
>   	int err;
>   
>   	do {
> -		if ((err = snd_dma_alloc_pages(dmab->dev.type, dmab->dev.dev,
> -					       size, dmab)) < 0) {
> -			if (err != -ENOMEM)
> -				return err; /* fatal error */
> -		} else
> -			return 0;
> +		err = do_alloc_pages(card, dmab->dev.type, dmab->dev.dev,
> +				     size, dmab);
> +		if (err != -ENOMEM)
> +			return err;
>   		size >>= 1;
>   	} while (size >= snd_minimum_buffer);
>   	dmab->bytes = 0; /* tell error */
> @@ -62,10 +92,7 @@ static int preallocate_pcm_pages(struct snd_pcm_substream *substream, size_t siz
>    */
>   static void snd_pcm_lib_preallocate_dma_free(struct snd_pcm_substream *substream)
>   {
> -	if (substream->dma_buffer.area == NULL)
> -		return;
> -	snd_dma_free_pages(&substream->dma_buffer);
> -	substream->dma_buffer.area = NULL;
> +	do_free_pages(substream->pcm->card, &substream->dma_buffer);
>   }
>   
>   /**
> @@ -130,6 +157,7 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
>   					       struct snd_info_buffer *buffer)
>   {
>   	struct snd_pcm_substream *substream = entry->private_data;
> +	struct snd_card *card = substream->pcm->card;
>   	char line[64], str[64];
>   	size_t size;
>   	struct snd_dma_buffer new_dmab;
> @@ -150,9 +178,10 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
>   		memset(&new_dmab, 0, sizeof(new_dmab));
>   		new_dmab.dev = substream->dma_buffer.dev;
>   		if (size > 0) {
> -			if (snd_dma_alloc_pages(substream->dma_buffer.dev.type,
> -						substream->dma_buffer.dev.dev,
> -						size, &new_dmab) < 0) {
> +			if (do_alloc_pages(card,
> +					   substream->dma_buffer.dev.type,
> +					   substream->dma_buffer.dev.dev,
> +					   size, &new_dmab) < 0) {
>   				buffer->error = -ENOMEM;
>   				return;
>   			}
> @@ -161,7 +190,7 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
>   			substream->buffer_bytes_max = UINT_MAX;
>   		}
>   		if (substream->dma_buffer.area)
> -			snd_dma_free_pages(&substream->dma_buffer);
> +			do_free_pages(card, &substream->dma_buffer);
>   		substream->dma_buffer = new_dmab;
>   	} else {
>   		buffer->error = -EINVAL;
> @@ -346,6 +375,7 @@ struct page *snd_pcm_sgbuf_ops_page(struct snd_pcm_substream *substream, unsigne
>    */
>   int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
>   {
> +	struct snd_card *card = substream->pcm->card;
>   	struct snd_pcm_runtime *runtime;
>   	struct snd_dma_buffer *dmab = NULL;
>   
> @@ -374,9 +404,10 @@ int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
>   		if (! dmab)
>   			return -ENOMEM;
>   		dmab->dev = substream->dma_buffer.dev;
> -		if (snd_dma_alloc_pages(substream->dma_buffer.dev.type,
> -					substream->dma_buffer.dev.dev,
> -					size, dmab) < 0) {
> +		if (do_alloc_pages(card,
> +				   substream->dma_buffer.dev.type,
> +				   substream->dma_buffer.dev.dev,
> +				   size, dmab) < 0) {
>   			kfree(dmab);
>   			return -ENOMEM;
>   		}
> @@ -397,6 +428,7 @@ EXPORT_SYMBOL(snd_pcm_lib_malloc_pages);
>    */
>   int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream)
>   {
> +	struct snd_card *card = substream->pcm->card;
>   	struct snd_pcm_runtime *runtime;
>   
>   	if (PCM_RUNTIME_CHECK(substream))
> @@ -406,7 +438,7 @@ int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream)
>   		return 0;
>   	if (runtime->dma_buffer_p != &substream->dma_buffer) {
>   		/* it's a newly allocated buffer.  release it now. */
> -		snd_dma_free_pages(runtime->dma_buffer_p);
> +		do_free_pages(card, runtime->dma_buffer_p);
>   		kfree(runtime->dma_buffer_p);
>   	}
>   	snd_pcm_set_runtime_buffer(substream, NULL);
> diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
> index b0c88fe040ee..4ca6b09056f3 100644
> --- a/sound/hda/Kconfig
> +++ b/sound/hda/Kconfig
> @@ -21,14 +21,16 @@ config SND_HDA_EXT_CORE
>          select SND_HDA_CORE
>   
>   config SND_HDA_PREALLOC_SIZE
> -	int "Pre-allocated buffer size for HD-audio driver"
> +	int "Pre-allocated buffer size for HD-audio driver" if !SND_DMA_SGBUF
>   	range 0 32768
> -	default 64
> +	default 0 if SND_DMA_SGBUF
> +	default 64 if !SND_DMA_SGBUF
>   	help
>   	  Specifies the default pre-allocated buffer-size in kB for the
>   	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
>   	  for systems using PulseAudio.  The default 64 is chosen just
>   	  for compatibility reasons.
> +	  On x86 systems, the default is zero as we need no preallocation.
>   
>   	  Note that the pre-allocation size can be changed dynamically
>   	  via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel