[alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue

[Keyon Jie]

On Thu, 2020-01-16 at 11:27 +0100, Takashi Iwai wrote:
> On Thu, 16 Jan 2020 10:50:33 +0100,
> 
> Oh, you're right, and I completely misread the patch.
> 
> Now I took a coffee and can tell you the story behind the scene.
> 
> I believe the current code is intentionally limiting the size to the
> preallocated size.  This limitation was brought for not trying to
> allocate a larger buffer when the buffer has been preallocated.  In
> the past, most hardware allocated the continuous pages for a buffer
> and the allocation of a large buffer fails quite likely.  This was
> the
> reason of the buffer preallocation.  So, the driver wanted to tell
> the
> user-space the limit.  If user needs to have an extra large buffer,
> they are supposed to fiddle with prealloc procfs (either setting zero
> to clear the preallocation or setting a large enough buffer
> beforehand).

Thank you for the sharing, it is interesting and knowledge learned to
me.

> 
> For SG-buffers, though, limitation makes less sense than continuous
> pages.  e.g. a patch below removes the limitation for SG-buffers.
> But changing this would definitely cause the behavior difference, and
> I don't know whether it's a reasonable move -- I'm afraid that apps
> would start hogging too much memory if the limitation is gone.

I just went through all invoking to snd_pcm_lib_preallocate_pages*(),
for those SNDRV_DMA_TYPE_DEV, some of them set the *size* equal to the
*max*, some set the *max* several times to the *size*, IMHO, the *max*s
are matched to those hardware's limiatation, comparing to the *size*s,
aren't they?

In this case, I still think my patch hanle all
TYPE_DEV/SNDRV_DMA_TYPE_DEV/TYPE_SG/SNDRV_DMA_TYPE_DEV cases more
gracefully, we will still take the limitation from the specific driver
set, from the *max* param, and the test results looks very nice here,
we will take what the user space wanted for buffer-bytes via aply
exactly, as long as it is suitable for the interval and constraints.

What's your opinion about it?

> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
> index d4702cc1d376..6a6c3469bbcd 100644
> --- a/sound/core/pcm_memory.c
> +++ b/sound/core/pcm_memory.c
> @@ -96,6 +96,29 @@ void snd_pcm_lib_preallocate_free_for_all(struct
> snd_pcm *pcm)
>  }
>  EXPORT_SYMBOL(snd_pcm_lib_preallocate_free_for_all);
>  
> +/* set up substream->buffer_bytes_max, which is used in
> hw_constraint */
> +static void set_buffer_bytes_max(struct snd_pcm_substream
> *substream,
> +				 size_t size)
> +{
> +	substream->buffer_bytes_max = UINT_MAX;
> +
> +	if (!size)
> +		return; /* no preallocation */
> +
> +	/* for SG-buffers, no limitation is needed */
> +	switch (substream->dma_buffer.dev.type) {
> +#ifdef CONFIG_SND_DMA_SGBUF
> +	case SNDRV_DMA_TYPE_DEV_SG:
> +	case SNDRV_DMA_TYPE_DEV_UC_SG:
> +#endif
> +	case SNDRV_DMA_TYPE_VMALLOC:
> +		return;
> +	}
> +
> +	/* for continuous buffers, limit to the preallocated size */
> +	substream->buffer_bytes_max = size;
> +}
> +
>  #ifdef CONFIG_SND_VERBOSE_PROCFS
>  /*
>   * read callback for prealloc proc file
> @@ -156,10 +179,8 @@ static void
> snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
>  				buffer->error = -ENOMEM;

if we won't take this change from user's fiddling for SG buffer, we
should not reallocate dma pages here also?

Thanks,
~Keyon

>  				return;
>  			}
> -			substream->buffer_bytes_max = size;
> -		} else {
> -			substream->buffer_bytes_max = UINT_MAX;
>  		}
> +		set_buffer_bytes_max(substream, size);
>  		if (substream->dma_buffer.area)
>  			snd_dma_free_pages(&substream->dma_buffer);
>  		substream->dma_buffer = new_dmab;
> @@ -206,10 +227,8 @@ static void preallocate_pages(struct
> snd_pcm_substream *substream,
>  
>  	if (size > 0 && preallocate_dma && substream->number <
> maximum_substreams)
>  		preallocate_pcm_pages(substream, size);
> -
> -	if (substream->dma_buffer.bytes > 0)
> -		substream->buffer_bytes_max = substream-
> >dma_buffer.bytes;
>  	substream->dma_max = max;
> +	set_buffer_bytes_max(substream, substream->dma_buffer.bytes);
>  	if (max > 0)
>  		preallocate_info_init(substream);
>  	if (managed)
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel