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 at 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 at 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