[alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Takashi Iwai]

Hi,

while recently working on the ALSA memory allocator API cleanup, I
noticed that dw-hdmi bridge driver seems doing weird about the buffer
management.  It pre-allocates the usual device buffers fully at the
probe time, while each stream allocates the buffer via the vmalloc
helpers and replaces with it at each open.

I guess it's no expected behavior?  It's basically a full waste of
resources, and the vmalloc buffer isn't guaranteed to work well for
mmap on every architecture.

Below is the patch to address it.  Can anyone check whether this still
works?

Since I have a cleanup series and this is involved, I'd like to take
the fix through my tree once after it's confirmed (and get ACK if
possible).


Thanks!

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations

The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
while it re-allocates and releases vmalloc pages.  This is not only a
lot of waste resources but also causes the mmap malfunction.

Change / drop the vmalloc-related code and use the standard buffer
allocation / release code instead.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index 2b7539701b42..8fe7a6e8ff94 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -384,15 +384,14 @@ static int dw_hdmi_close(struct snd_pcm_substream *substream)
 
 static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
 {
-	return snd_pcm_lib_free_vmalloc_buffer(substream);
+	return snd_pcm_lib_free_pages(substream);
 }
 
 static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params)
 {
 	/* Allocate the PCM runtime buffer, which is exposed to userspace. */
-	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
-						params_buffer_bytes(params));
+	return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
 }
 
 static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
@@ -511,7 +510,6 @@ static const struct snd_pcm_ops snd_dw_hdmi_ops = {
 	.prepare = dw_hdmi_prepare,
 	.trigger = dw_hdmi_trigger,
 	.pointer = dw_hdmi_pointer,
-	.page = snd_pcm_lib_get_vmalloc_page,
 };
 
 static int snd_dw_hdmi_probe(struct platform_device *pdev)
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Neil Armstrong]

Hi,

On 05/11/2019 08:55, Takashi Iwai wrote:
> Hi,
> 
> while recently working on the ALSA memory allocator API cleanup, I
> noticed that dw-hdmi bridge driver seems doing weird about the buffer
> management.  It pre-allocates the usual device buffers fully at the
> probe time, while each stream allocates the buffer via the vmalloc
> helpers and replaces with it at each open.
> 
> I guess it's no expected behavior?  It's basically a full waste of
> resources, and the vmalloc buffer isn't guaranteed to work well for
> mmap on every architecture.
> 
> Below is the patch to address it.  Can anyone check whether this still
> works?

I don't have the setup to check, but this has been pushed by Russell I Added in CC.

I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.

Neil

> 
> Since I have a cleanup series and this is involved, I'd like to take
> the fix through my tree once after it's confirmed (and get ACK if
> possible).
> 
> 
> Thanks!
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> 
> The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> while it re-allocates and releases vmalloc pages.  This is not only a
> lot of waste resources but also causes the mmap malfunction.
> 
> Change / drop the vmalloc-related code and use the standard buffer
> allocation / release code instead.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> index 2b7539701b42..8fe7a6e8ff94 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> @@ -384,15 +384,14 @@ static int dw_hdmi_close(struct snd_pcm_substream *substream)
>  
>  static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
>  {
> -	return snd_pcm_lib_free_vmalloc_buffer(substream);
> +	return snd_pcm_lib_free_pages(substream);
>  }
>  
>  static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_pcm_hw_params *params)
>  {
>  	/* Allocate the PCM runtime buffer, which is exposed to userspace. */
> -	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
> -						params_buffer_bytes(params));
> +	return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
>  }
>  
>  static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
> @@ -511,7 +510,6 @@ static const struct snd_pcm_ops snd_dw_hdmi_ops = {
>  	.prepare = dw_hdmi_prepare,
>  	.trigger = dw_hdmi_trigger,
>  	.pointer = dw_hdmi_pointer,
> -	.page = snd_pcm_lib_get_vmalloc_page,
>  };
>  
>  static int snd_dw_hdmi_probe(struct platform_device *pdev)
> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Russell King - ARM Linux admin]

On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> Hi,
> 
> On 05/11/2019 08:55, Takashi Iwai wrote:
> > Hi,
> > 
> > while recently working on the ALSA memory allocator API cleanup, I
> > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > management.  It pre-allocates the usual device buffers fully at the
> > probe time, while each stream allocates the buffer via the vmalloc
> > helpers and replaces with it at each open.
> > 
> > I guess it's no expected behavior?  It's basically a full waste of
> > resources, and the vmalloc buffer isn't guaranteed to work well for
> > mmap on every architecture.
> > 
> > Below is the patch to address it.  Can anyone check whether this still
> > works?
> 
> I don't have the setup to check, but this has been pushed by Russell I Added in CC.
> 
> I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
> 
> Neil
> 
> > 
> > Since I have a cleanup series and this is involved, I'd like to take
> > the fix through my tree once after it's confirmed (and get ACK if
> > possible).
> > 
> > 
> > Thanks!
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> > 
> > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > while it re-allocates and releases vmalloc pages.  This is not only a
> > lot of waste resources but also causes the mmap malfunction.
> > 
> > Change / drop the vmalloc-related code and use the standard buffer
> > allocation / release code instead.

I think getting rid of the vmalloc code here is a mistake - I seem to
remember using the standard buffer allocation causes failures, due to
memory fragmentation.  Since the hardware is limited to DMA from at
most one page, there is no reason for this driver to require contiguous
pages, hence why it's using - and should use - vmalloc pages.  vmalloc
is way kinder to the MM subsystem than trying to request large order
contiguous pages.

So, NAK on this patch.

> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > index 2b7539701b42..8fe7a6e8ff94 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > @@ -384,15 +384,14 @@ static int dw_hdmi_close(struct snd_pcm_substream *substream)
> >  
> >  static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
> >  {
> > -	return snd_pcm_lib_free_vmalloc_buffer(substream);
> > +	return snd_pcm_lib_free_pages(substream);
> >  }
> >  
> >  static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
> >  	struct snd_pcm_hw_params *params)
> >  {
> >  	/* Allocate the PCM runtime buffer, which is exposed to userspace. */
> > -	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
> > -						params_buffer_bytes(params));
> > +	return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
> >  }
> >  
> >  static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
> > @@ -511,7 +510,6 @@ static const struct snd_pcm_ops snd_dw_hdmi_ops = {
> >  	.prepare = dw_hdmi_prepare,
> >  	.trigger = dw_hdmi_trigger,
> >  	.pointer = dw_hdmi_pointer,
> > -	.page = snd_pcm_lib_get_vmalloc_page,
> >  };
> >  
> >  static int snd_dw_hdmi_probe(struct platform_device *pdev)
> > 
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Takashi Iwai]

On Tue, 05 Nov 2019 17:02:15 +0100,
Russell King - ARM Linux admin wrote:
> 
> On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> > Hi,
> > 
> > On 05/11/2019 08:55, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > while recently working on the ALSA memory allocator API cleanup, I
> > > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > > management.  It pre-allocates the usual device buffers fully at the
> > > probe time, while each stream allocates the buffer via the vmalloc
> > > helpers and replaces with it at each open.
> > > 
> > > I guess it's no expected behavior?  It's basically a full waste of
> > > resources, and the vmalloc buffer isn't guaranteed to work well for
> > > mmap on every architecture.
> > > 
> > > Below is the patch to address it.  Can anyone check whether this still
> > > works?
> > 
> > I don't have the setup to check, but this has been pushed by Russell I Added in CC.
> > 
> > I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
> > 
> > Neil
> > 
> > > 
> > > Since I have a cleanup series and this is involved, I'd like to take
> > > the fix through my tree once after it's confirmed (and get ACK if
> > > possible).
> > > 
> > > 
> > > Thanks!
> > > 
> > > Takashi
> > > 
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> > > 
> > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > > while it re-allocates and releases vmalloc pages.  This is not only a
> > > lot of waste resources but also causes the mmap malfunction.
> > > 
> > > Change / drop the vmalloc-related code and use the standard buffer
> > > allocation / release code instead.
> 
> I think getting rid of the vmalloc code here is a mistake - I seem to
> remember using the standard buffer allocation causes failures, due to
> memory fragmentation.  Since the hardware is limited to DMA from at
> most one page, there is no reason for this driver to require contiguous
> pages, hence why it's using - and should use - vmalloc pages.  vmalloc
> is way kinder to the MM subsystem than trying to request large order
> contiguous pages.
> 
> So, NAK on this patch.

OK, then we should do other way round, rather drop the buffer
preallocation instead.  Currently vmalloc buffer is always allocated
at each open and overrides the preallocated buffer, so the whole 64k
and more are wasted for no use.

(BTW, the current code has this snippet:

	/* Limit the buffer size to the size of the preallocated buffer */
	ret = snd_pcm_hw_constraint_minmax(runtime,
					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
					   0, substream->dma_buffer.bytes);

... and this would have to limit the buffer size only to the
preallocated size -- which essentially makes the argument above
invalid.  However, this check looks actually bogus, the constraint
parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE.  It
might be the reason it worked somehow...)

So below is the revised patch.  Could you guys check it again?

There I copied the comment as is, although the 512k mentioned there
looks inconsistent with the actual code.  Should it be 1M?


Thanks!

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/bridge: dw-hdmi: A couple of fixes wrt buffer allocation

The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
while it always re-allocates and releases vmalloc pages, hence the
preallocated pages are never used.  Also, the current code contains
the hw constraint to limit the buffer size to the preallocated size.
It doesn't work as expected, however, because it's applying to an
incorrect parameter (SNDRV_PCM_HW_PARAM_BUFFER_SIZE instead of
_BUFFER_BYTES).

This patch tries to address these issues: drop the unnecessary buffer
preallocation and fix the hw constraint to the right parameter.  Since
the buffer preallocation is dropped, the max buffer size that was
passed to the preallocation is passed directly now to the hw
constraint call.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index 2b7539701b42..3efbbc59994b 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -337,10 +337,14 @@ static int dw_hdmi_open(struct snd_pcm_substream *substream)
 	if (ret < 0)
 		return ret;
 
-	/* Limit the buffer size to the size of the preallocated buffer */
+	/*
+	 * Limit the buffer size:
+	 * to support 8-channel 96kHz audio reliably, we need 512k
+	 * to satisfy alsa with our restricted period (ERR004323).
+	 */
 	ret = snd_pcm_hw_constraint_minmax(runtime,
-					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-					   0, substream->dma_buffer.bytes);
+					   SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
+					   0, 1024 * 1024);
 	if (ret < 0)
 		return ret;
 
@@ -560,13 +564,6 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
 	strlcpy(pcm->name, DRIVER_NAME, sizeof(pcm->name));
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_dw_hdmi_ops);
 
-	/*
-	 * To support 8-channel 96kHz audio reliably, we need 512k
-	 * to satisfy alsa with our restricted period (ERR004323).
-	 */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
-			dev, 128 * 1024, 1024 * 1024);
-
 	ret = snd_card_register(card);
 	if (ret < 0)
 		goto err;
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Takashi Iwai]

On Tue, 05 Nov 2019 17:33:44 +0100,
Takashi Iwai wrote:
> 
> On Tue, 05 Nov 2019 17:02:15 +0100,
> Russell King - ARM Linux admin wrote:
> > 
> > On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> > > Hi,
> > > 
> > > On 05/11/2019 08:55, Takashi Iwai wrote:
> > > > Hi,
> > > > 
> > > > while recently working on the ALSA memory allocator API cleanup, I
> > > > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > > > management.  It pre-allocates the usual device buffers fully at the
> > > > probe time, while each stream allocates the buffer via the vmalloc
> > > > helpers and replaces with it at each open.
> > > > 
> > > > I guess it's no expected behavior?  It's basically a full waste of
> > > > resources, and the vmalloc buffer isn't guaranteed to work well for
> > > > mmap on every architecture.
> > > > 
> > > > Below is the patch to address it.  Can anyone check whether this still
> > > > works?
> > > 
> > > I don't have the setup to check, but this has been pushed by Russell I Added in CC.
> > > 
> > > I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
> > > 
> > > Neil
> > > 
> > > > 
> > > > Since I have a cleanup series and this is involved, I'd like to take
> > > > the fix through my tree once after it's confirmed (and get ACK if
> > > > possible).
> > > > 
> > > > 
> > > > Thanks!
> > > > 
> > > > Takashi
> > > > 
> > > > -- 8< --
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> > > > 
> > > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > > > while it re-allocates and releases vmalloc pages.  This is not only a
> > > > lot of waste resources but also causes the mmap malfunction.
> > > > 
> > > > Change / drop the vmalloc-related code and use the standard buffer
> > > > allocation / release code instead.
> > 
> > I think getting rid of the vmalloc code here is a mistake - I seem to
> > remember using the standard buffer allocation causes failures, due to
> > memory fragmentation.  Since the hardware is limited to DMA from at
> > most one page, there is no reason for this driver to require contiguous
> > pages, hence why it's using - and should use - vmalloc pages.  vmalloc
> > is way kinder to the MM subsystem than trying to request large order
> > contiguous pages.
> > 
> > So, NAK on this patch.
> 
> OK, then we should do other way round, rather drop the buffer
> preallocation instead.  Currently vmalloc buffer is always allocated
> at each open and overrides the preallocated buffer, so the whole 64k
> and more are wasted for no use.
> 
> (BTW, the current code has this snippet:
> 
> 	/* Limit the buffer size to the size of the preallocated buffer */
> 	ret = snd_pcm_hw_constraint_minmax(runtime,
> 					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
> 					   0, substream->dma_buffer.bytes);
> 
> ... and this would have to limit the buffer size only to the
> preallocated size -- which essentially makes the argument above
> invalid.  However, this check looks actually bogus, the constraint
> parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE.  It
> might be the reason it worked somehow...)
> 
> So below is the revised patch.  Could you guys check it again?
> 
> There I copied the comment as is, although the 512k mentioned there
> looks inconsistent with the actual code.  Should it be 1M?

... and reading the patch again, I found that the hw constraint call
can be dropped as well.  The dw_hdmi_hw definition already contains
the max buffer size.

Below is the re-revised patch.  Please check it.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/bridge: dw-hdmi: Drop superfluous buffer preallocation

The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
while it always re-allocates and releases vmalloc pages, hence the
preallocated pages are never used.  Also, the current code contains
the hw constraint to limit the buffer size to the preallocated size.
It doesn't work as expected, however, because it's applying to an
incorrect parameter (SNDRV_PCM_HW_PARAM_BUFFER_SIZE instead of
_BUFFER_BYTES).

This patch tries to address these issues: drop the unnecessary buffer
preallocation and also drop the unnecessary hw constriant.  Without
the buffer preallocation, the hw constraint becomes superfluous, as
dw_hdmi_hw definition already contains the buffer byte size
constraints.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index 2b7539701b42..2778d6bcc14e 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -337,13 +337,6 @@ static int dw_hdmi_open(struct snd_pcm_substream *substream)
 	if (ret < 0)
 		return ret;
 
-	/* Limit the buffer size to the size of the preallocated buffer */
-	ret = snd_pcm_hw_constraint_minmax(runtime,
-					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-					   0, substream->dma_buffer.bytes);
-	if (ret < 0)
-		return ret;
-
 	/* Clear FIFO */
 	writeb_relaxed(HDMI_AHB_DMA_CONF0_SW_FIFO_RST,
 		       base + HDMI_AHB_DMA_CONF0);
@@ -560,13 +553,6 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
 	strlcpy(pcm->name, DRIVER_NAME, sizeof(pcm->name));
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_dw_hdmi_ops);
 
-	/*
-	 * To support 8-channel 96kHz audio reliably, we need 512k
-	 * to satisfy alsa with our restricted period (ERR004323).
-	 */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
-			dev, 128 * 1024, 1024 * 1024);
-
 	ret = snd_card_register(card);
 	if (ret < 0)
 		goto err;
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Russell King - ARM Linux admin]

On Tue, Nov 05, 2019 at 05:44:26PM +0100, Takashi Iwai wrote:
> On Tue, 05 Nov 2019 17:33:44 +0100,
> Takashi Iwai wrote:
> > 
> > On Tue, 05 Nov 2019 17:02:15 +0100,
> > Russell King - ARM Linux admin wrote:
> > > 
> > > On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> > > > Hi,
> > > > 
> > > > On 05/11/2019 08:55, Takashi Iwai wrote:
> > > > > Hi,
> > > > > 
> > > > > while recently working on the ALSA memory allocator API cleanup, I
> > > > > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > > > > management.  It pre-allocates the usual device buffers fully at the
> > > > > probe time, while each stream allocates the buffer via the vmalloc
> > > > > helpers and replaces with it at each open.
> > > > > 
> > > > > I guess it's no expected behavior?  It's basically a full waste of
> > > > > resources, and the vmalloc buffer isn't guaranteed to work well for
> > > > > mmap on every architecture.
> > > > > 
> > > > > Below is the patch to address it.  Can anyone check whether this still
> > > > > works?
> > > > 
> > > > I don't have the setup to check, but this has been pushed by Russell I Added in CC.
> > > > 
> > > > I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
> > > > 
> > > > Neil
> > > > 
> > > > > 
> > > > > Since I have a cleanup series and this is involved, I'd like to take
> > > > > the fix through my tree once after it's confirmed (and get ACK if
> > > > > possible).
> > > > > 
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > > Takashi
> > > > > 
> > > > > -- 8< --
> > > > > From: Takashi Iwai <tiwai@suse.de>
> > > > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> > > > > 
> > > > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > > > > while it re-allocates and releases vmalloc pages.  This is not only a
> > > > > lot of waste resources but also causes the mmap malfunction.
> > > > > 
> > > > > Change / drop the vmalloc-related code and use the standard buffer
> > > > > allocation / release code instead.
> > > 
> > > I think getting rid of the vmalloc code here is a mistake - I seem to
> > > remember using the standard buffer allocation causes failures, due to
> > > memory fragmentation.  Since the hardware is limited to DMA from at
> > > most one page, there is no reason for this driver to require contiguous
> > > pages, hence why it's using - and should use - vmalloc pages.  vmalloc
> > > is way kinder to the MM subsystem than trying to request large order
> > > contiguous pages.
> > > 
> > > So, NAK on this patch.
> > 
> > OK, then we should do other way round, rather drop the buffer
> > preallocation instead.  Currently vmalloc buffer is always allocated
> > at each open and overrides the preallocated buffer, so the whole 64k
> > and more are wasted for no use.
> > 
> > (BTW, the current code has this snippet:
> > 
> > 	/* Limit the buffer size to the size of the preallocated buffer */
> > 	ret = snd_pcm_hw_constraint_minmax(runtime,
> > 					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
> > 					   0, substream->dma_buffer.bytes);
> > 
> > ... and this would have to limit the buffer size only to the
> > preallocated size -- which essentially makes the argument above
> > invalid.  However, this check looks actually bogus, the constraint
> > parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE.  It
> > might be the reason it worked somehow...)
> > 
> > So below is the revised patch.  Could you guys check it again?
> > 
> > There I copied the comment as is, although the 512k mentioned there
> > looks inconsistent with the actual code.  Should it be 1M?
> 
> ... and reading the patch again, I found that the hw constraint call
> can be dropped as well.  The dw_hdmi_hw definition already contains
> the max buffer size.
> 
> Below is the re-revised patch.  Please check it.

I was slightly wrong - sorry.  It's been a long time since I looked at
this driver, or even used it - but it is the only driver that supports
HDMI audio on iMX6 platforms, so I guess there are lots of users out
there... or maybe not if none of them use mainline kernels.

The hardware is capable of reading across a page.  However, the hardware
is _not_ capable of reading any data that is formatted as ALSA APIs
allow it to be.  The driver has to reformat the ALSA supplied sound
buffers to the layout the hardware requires.

To do this, we have two different buffers:

- The substream buffer is the buffer which the hardware reads from.
- The runtime buffer is the buffer which ALSA uses.

The call to snd_pcm_lib_preallocate_pages_for_all() allocates the
hardware buffer, which is a single contiguous buffer of fixed size.

The user buffer is allocated with snd_pcm_lib_alloc_vmalloc_buffer().

Hence, the driver makes use of both.  You can't get rid of either
of them.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Incorrect buffer handling in dw-hdmi bridge audio

[Takashi Iwai]

On Tue, 05 Nov 2019 18:02:03 +0100,
Russell King - ARM Linux admin wrote:
> 
> On Tue, Nov 05, 2019 at 05:44:26PM +0100, Takashi Iwai wrote:
> > On Tue, 05 Nov 2019 17:33:44 +0100,
> > Takashi Iwai wrote:
> > > 
> > > On Tue, 05 Nov 2019 17:02:15 +0100,
> > > Russell King - ARM Linux admin wrote:
> > > > 
> > > > On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> > > > > Hi,
> > > > > 
> > > > > On 05/11/2019 08:55, Takashi Iwai wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > while recently working on the ALSA memory allocator API cleanup, I
> > > > > > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > > > > > management.  It pre-allocates the usual device buffers fully at the
> > > > > > probe time, while each stream allocates the buffer via the vmalloc
> > > > > > helpers and replaces with it at each open.
> > > > > > 
> > > > > > I guess it's no expected behavior?  It's basically a full waste of
> > > > > > resources, and the vmalloc buffer isn't guaranteed to work well for
> > > > > > mmap on every architecture.
> > > > > > 
> > > > > > Below is the patch to address it.  Can anyone check whether this still
> > > > > > works?
> > > > > 
> > > > > I don't have the setup to check, but this has been pushed by Russell I Added in CC.
> > > > > 
> > > > > I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
> > > > > 
> > > > > Neil
> > > > > 
> > > > > > 
> > > > > > Since I have a cleanup series and this is involved, I'd like to take
> > > > > > the fix through my tree once after it's confirmed (and get ACK if
> > > > > > possible).
> > > > > > 
> > > > > > 
> > > > > > Thanks!
> > > > > > 
> > > > > > Takashi
> > > > > > 
> > > > > > -- 8< --
> > > > > > From: Takashi Iwai <tiwai@suse.de>
> > > > > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> > > > > > 
> > > > > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > > > > > while it re-allocates and releases vmalloc pages.  This is not only a
> > > > > > lot of waste resources but also causes the mmap malfunction.
> > > > > > 
> > > > > > Change / drop the vmalloc-related code and use the standard buffer
> > > > > > allocation / release code instead.
> > > > 
> > > > I think getting rid of the vmalloc code here is a mistake - I seem to
> > > > remember using the standard buffer allocation causes failures, due to
> > > > memory fragmentation.  Since the hardware is limited to DMA from at
> > > > most one page, there is no reason for this driver to require contiguous
> > > > pages, hence why it's using - and should use - vmalloc pages.  vmalloc
> > > > is way kinder to the MM subsystem than trying to request large order
> > > > contiguous pages.
> > > > 
> > > > So, NAK on this patch.
> > > 
> > > OK, then we should do other way round, rather drop the buffer
> > > preallocation instead.  Currently vmalloc buffer is always allocated
> > > at each open and overrides the preallocated buffer, so the whole 64k
> > > and more are wasted for no use.
> > > 
> > > (BTW, the current code has this snippet:
> > > 
> > > 	/* Limit the buffer size to the size of the preallocated buffer */
> > > 	ret = snd_pcm_hw_constraint_minmax(runtime,
> > > 					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
> > > 					   0, substream->dma_buffer.bytes);
> > > 
> > > ... and this would have to limit the buffer size only to the
> > > preallocated size -- which essentially makes the argument above
> > > invalid.  However, this check looks actually bogus, the constraint
> > > parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE.  It
> > > might be the reason it worked somehow...)
> > > 
> > > So below is the revised patch.  Could you guys check it again?
> > > 
> > > There I copied the comment as is, although the 512k mentioned there
> > > looks inconsistent with the actual code.  Should it be 1M?
> > 
> > ... and reading the patch again, I found that the hw constraint call
> > can be dropped as well.  The dw_hdmi_hw definition already contains
> > the max buffer size.
> > 
> > Below is the re-revised patch.  Please check it.
> 
> I was slightly wrong - sorry.  It's been a long time since I looked at
> this driver, or even used it - but it is the only driver that supports
> HDMI audio on iMX6 platforms, so I guess there are lots of users out
> there... or maybe not if none of them use mainline kernels.
> 
> The hardware is capable of reading across a page.  However, the hardware
> is _not_ capable of reading any data that is formatted as ALSA APIs
> allow it to be.  The driver has to reformat the ALSA supplied sound
> buffers to the layout the hardware requires.
> 
> To do this, we have two different buffers:
> 
> - The substream buffer is the buffer which the hardware reads from.
> - The runtime buffer is the buffer which ALSA uses.
> 
> The call to snd_pcm_lib_preallocate_pages_for_all() allocates the
> hardware buffer, which is a single contiguous buffer of fixed size.
> 
> The user buffer is allocated with snd_pcm_lib_alloc_vmalloc_buffer().
> 
> Hence, the driver makes use of both.  You can't get rid of either
> of them.

Ah, thanks, it makes sense and enlightens me!

OK, so we need to keep both buffers.  But the typo of hw constraint
parameter should be still corrected, but it's a minor issue.

It's a pity that this driver will be the only one who still needs the
explicit calls of snd_pcm_lib_*vmalloc*() helpers after my cleanup
series.  I might come up with another cleanup, but then let's keep the
buffer stuff as is.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel