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

[Takashi Iwai <tiwai@suse.de>]

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel