From: Takashi Iwai <tiwai@suse.de> To: Keyon Jie <yang.jie@linux.intel.com> Cc: alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue Date: Thu, 16 Jan 2020 11:27:37 +0100 Message-ID: <s5hsgkf7l2e.wl-tiwai@suse.de> (raw) In-Reply-To: <97bbe88d1a6b63fe8e9b02bf0c5ce4a80553c48d.camel@linux.intel.com> On Thu, 16 Jan 2020 10:50:33 +0100, Keyon Jie wrote: > > On Thu, 2020-01-16 at 08:15 +0100, Takashi Iwai wrote: > > On Thu, 16 Jan 2020 05:53:18 +0100, > > Keyon Jie wrote: > > > With today's code, we preallocate DMA buffer for substreams at > > > pcm_new() > > > stage, and the substream->buffer_bytes_max and substream->dma_max > > > will > > > save as the actually preallocated buffer size and maximum size that > > > the > > > dma buffer can be expanded by at hw_params() state, > > > correspondingly. > > > > No, it's other way round: the former, buffer_bytes_max, is the max > > size defined by the driver (i.e. passed in snd_pcm_hardware) and the > > latter, dma_max, is the max preallocation size (passed to > > preallocation helper). > > Hi Takashi, thanks for your comment. > > First of all, have you ever hit issue I mentioned in the commit message > that we can't set buffer_bytes larger than the preallocated dma bytes? > > I found this issue in kinds of platforms, not only on SOF/SoC ones, but > also on legacy HDA ones. > > Secondly, I am not clear about the design intention of the substream- > >buffer_bytes_max and substream->dma_max, if it is as you commented > above, can you help answer my questions below inline the code? > > void snd_pcm_lib_preallocate_pages(struct snd_pcm_substream *substream, > int type, struct device *data, > size_t size, size_t max) > > static void preallocate_pages(struct snd_pcm_substream *substream, > int type, struct device *data, > size_t size, size_t max, bool managed) > { > ... > if (substream->dma_buffer.bytes > 0) > substream->buffer_bytes_max = substream- > >dma_buffer.bytes;//Keyon: this is the actual allocated buffer bytes, > what is the intention here and why it is assigned to buffer_bytes_max > which will be used to constrain on the _HW_PARAM_BUFFER_BYTES later? > > substream->dma_max = max; //Keyon: looks here it is where the > *max* param used only if we don't define SND_VERBOSE_PROCFS? what > relationship can we have with the preallocation itself? > ... > } 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). 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. 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; 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@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply index Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-16 4:53 Keyon Jie 2020-01-16 7:15 ` Takashi Iwai 2020-01-16 9:50 ` Keyon Jie 2020-01-16 10:27 ` Takashi Iwai [this message] 2020-01-16 11:25 ` Keyon Jie 2020-01-16 11:50 ` Takashi Iwai 2020-01-16 14:14 ` Jie, Yang 2020-01-16 15:31 ` Jie, Yang 2020-01-16 16:07 ` Takashi Iwai 2020-01-16 16:39 ` Pierre-Louis Bossart 2020-01-16 17:25 ` Rajwa, Marcin 2020-01-16 17:40 ` Pierre-Louis Bossart 2020-01-16 20:37 ` Takashi Iwai 2020-01-17 5:30 ` Keyon Jie 2020-01-17 7:57 ` Takashi Iwai 2020-01-17 10:13 ` Keyon Jie 2020-01-17 10:30 ` Takashi Iwai 2020-01-17 10:56 ` Keyon Jie 2020-01-17 11:15 ` Takashi Iwai 2020-01-17 5:37 ` Keyon Jie 2020-01-17 8:00 ` Takashi Iwai 2020-01-17 10:43 ` Keyon Jie 2020-01-17 11:12 ` Takashi Iwai 2020-01-19 3:52 ` Keyon Jie 2020-01-19 7:09 ` Takashi Iwai 2020-01-19 8:11 ` Keyon Jie 2020-01-19 9:04 ` Takashi Iwai 2020-01-19 10:14 ` Keyon Jie 2020-01-19 10:43 ` Takashi Iwai 2020-01-20 2:23 ` Keyon Jie 2020-01-16 15:45 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5hsgkf7l2e.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=yang.jie@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Alsa-Devel Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \ alsa-devel@alsa-project.org public-inbox-index alsa-devel Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git