Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Keyon Jie <yang.jie@linux.intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	"Rajwa, Marcin" <marcin.rajwa@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue
Date: Sun, 19 Jan 2020 10:04:06 +0100
Message-ID: <s5hftgbom0p.wl-tiwai@suse.de> (raw)
In-Reply-To: <c7a64462-1cc1-234b-ac96-7774e6116118@linux.intel.com>

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

  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
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 [this message]
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 publically 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=s5hftgbom0p.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=marcin.rajwa@linux.intel.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --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