[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 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.
> > > 
> > > 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