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;