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@xxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
