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);@@ -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@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel