[alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue
Takashi Iwai
tiwai at suse.de
Sun Jan 19 10:04:06 CET 2020
On Sun, 19 Jan 2020 09:11:17 +0100,
Keyon Jie wrote:
>
> On 2020/1/19 下午3:09, Takashi Iwai wrote:
> > On Sun, 19 Jan 2020 04:52:55 +0100,
> > Keyon Jie wrote:
> >>
> >>
> >> On 2020/1/17 下午7:12, Takashi Iwai wrote:
> >>> On Fri, 17 Jan 2020 11:43:24 +0100,
> >>> Keyon Jie wrote:
> >>>>
> >>>> In SOF driver, we don't use kernel config item like
> >>>> CONFIG_SND_HDA_PREALLOC_SIZE for HDA, the code for it is:
> >>>>
> >>>> snd_pcm_lib_preallocate_pages(pcm->streams[stream].substream,
> >>>> SNDRV_DMA_TYPE_DEV_SG, sdev->dev,
> >>>> le32_to_cpu(caps->buffer_size_min),
> >>>> le32_to_cpu(caps->buffer_size_max));
> >>>>
> >>>> So the preallocated size is configured via topology file, that is
> >>>> caps->buffer_size_min, no chance for PulseAudio to reconfigure it.
> >>>>
> >>>> So, it looks like we have to change it to this if we don't change the
> >>>> ALSA core:
> >>>>
> >>>> snd_pcm_lib_preallocate_pages(pcm->streams[stream].substream,
> >>>> SNDRV_DMA_TYPE_DEV_SG, sdev->dev,
> >>>> - le32_to_cpu(caps->buffer_size_min),
> >>>> + le32_to_cpu(caps->buffer_size_max),
> >>>> le32_to_cpu(caps->buffer_size_max));
> >>>
> >>> Yes, passing buffer_size_min for the preallocation sounds already
> >>> bad. The default value should be sufficient for usual operations, not
> >>> the cost-cutting minimum. Otherwise there is no merit of
> >>> preallocation.
> >>>
> >>> Alternatively, we may pass 0 there, indicating no limitation, too.
> >>> But, this would need a bit other adjustment, e.g. snd_pcm_hardware
> >>> should have lower buffer_bytes_max.
> >>
> >> Thank you Takashi, then let's follow it to pre-allocate with
> >> caps->buffer_size_max, as we don't specify any limitations in
> >> snd_pcm_hardware today, we want to leave it configurable to each
> >> specific topology file for different machines.
> >
> > How big is caps->buffer_size_max? Passing the value there means
> > actually trying to allocate the given size as default, and it'd be a
> > lot of waste if a too large value (e.g. 32MB) is passed there.
>
> 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.
> > 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.
> > 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.
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.
More information about the Alsa-devel
mailing list