diff mbox series

media: mediatek: vcodec: Alloc DMA memory with DMA_ATTR_ALLOC_SINGLE_PAGES

Message ID 20240422100354.1.I58b4456c014a4d678455a4ec09b908b1c71c3017@changeid
State New
Headers show
Series media: mediatek: vcodec: Alloc DMA memory with DMA_ATTR_ALLOC_SINGLE_PAGES | expand

Commit Message

Doug Anderson April 22, 2024, 5:03 p.m. UTC 
As talked about in commit 14d3ae2efeed ("ARM: 8507/1: dma-mapping: Use
DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc"), it doesn't
really make sense to try to allocate contiguous chunks of memory for
video encoding/decoding. Let's switch the Mediatek vcodec driver to
pass DMA_ATTR_ALLOC_SINGLE_PAGES and take some of the stress off the
memory subsystem.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
NOTE: I haven't personally done massive amounts of testing with this
change, but I originally added the DMA_ATTR_ALLOC_SINGLE_PAGES flag
specifically for the video encoding / decoding cases and I know it
helped avoid memory problems in the past on other systems. Colleagues
of mine have told me that with this change memory problems are harder
to reproduce, so it seems like we should consider doing it.

 .../media/platform/mediatek/vcodec/common/mtk_vcodec_util.c    | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Fei Shao April 26, 2024, 10:20 a.m. UTC | #1 
Hi Nicolas,

On Tue, Apr 23, 2024 at 2:52 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Apr 22, 2024 at 11:27 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> >
> > Hi,
> >
> > Le lundi 22 avril 2024 à 10:03 -0700, Douglas Anderson a écrit :
> > > As talked about in commit 14d3ae2efeed ("ARM: 8507/1: dma-mapping: Use
> > > DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc"), it doesn't
> > > really make sense to try to allocate contiguous chunks of memory for
> > > video encoding/decoding. Let's switch the Mediatek vcodec driver to
> > > pass DMA_ATTR_ALLOC_SINGLE_PAGES and take some of the stress off the
> > > memory subsystem.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > NOTE: I haven't personally done massive amounts of testing with this
> > > change, but I originally added the DMA_ATTR_ALLOC_SINGLE_PAGES flag
> > > specifically for the video encoding / decoding cases and I know it
> > > helped avoid memory problems in the past on other systems. Colleagues
> > > of mine have told me that with this change memory problems are harder
> > > to reproduce, so it seems like we should consider doing it.
> >
> > One thing to improve in your patch submission is to avoid abstracting the
> > problems. Patch review and pulling is based on a technical rational and very
> > rarely on the trust that it helps someone somewhere in some unknown context.
> > What kind of memory issues are you facing ? What is the technical advantage of
> > using DMA_ATTR_ALLOC_SINGLE_PAGES over the current approach that helps fixing
> > the issue? I do expect this to be documented in the commit message itselfé.
>
> Right. The problem here is that I'm not _directly_ facing any problems
> here and I also haven't done massive amounts of analysis of the
> Mediatek video codec. I know that some of my colleagues have run into
> issues on Mediatek devices where the system starts getting
> unresponsive when lots of videos are decoded in parallel. That
> reminded me of the old problem I debugged in 2015 on Rockchip
> platforms and is talked about a bunch in the referenced commit
> 14d3ae2efeed ("ARM: 8507/1: dma-mapping: Use
> DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc") so I wrote up
> this patch. The referenced commit contains quite a bit of details
> about the problems faced back in 2015.
>
> When I asked, my colleagues said that my patch seemed to help, though
> it was more of a qualitative statement than a quantitative one.

The story behind this is that I'm looking into an issue on the MediaTek
MT8188 Chromebook, where in some scenarios the system may emit 30+
video decoders concurrently (e.g. generating thumbnails for excess
amount of video files etc.), and such behavior can easily hang the
system if it has a smaller amount of memory (<4GB).

In addition to seeking mitigation in the user space software side,
we're also looking for ways to optimize how the video decoders use
memory, so Doug suggested this improvement.
My preliminary experiment showed that it has some positive impact -
the system doesn't freeze up completely with it and is still
responsive in the UART serial console. However, just like mentioned, I
didn’t have any rigorous numbers to support it.

To test the patch better, today I set up a local WebRTC demo to
simulate a video conference with 49 people where the mocked input
stream is captured from the device's own front camera.
With that, the original system easily hung in less than one minute
with less than 40MB available memory at the time; but with the change,
the system ran for several minutes and had an average of over 100MB
memory. It's not a huge improvement, but it's something.

I know this isn't the most scientific experiment, but I hope it’s a
good enough representation of one of the multi video decoder use
cases, and gives you some confidence that the patch is worth merging.

With the test above I think I can give this:
Tested-by: Fei Shao <fshao@chromium.org>

And, since this patch LGTM and I support it, here's my humble
Reviewed-by: Fei Shao <fshao@chromium.org>

Regards,
Fei

>
> I wasn't 100% sure if it was worth sending the patch up at this point,
> but logically, I think it makes sense. There aren't great reasons to
> hog all the large chunks of memory for video decoding.
>
> -Doug
Nicolas Dufresne May 1, 2024, 6:31 p.m. UTC | #2 
Le vendredi 26 avril 2024 à 18:20 +0800, Fei Shao a écrit :
> Hi Nicolas,
> 
> On Tue, Apr 23, 2024 at 2:52 PM Doug Anderson <dianders@chromium.org> wrote:
> > 
> > Hi,
> > 
> > On Mon, Apr 22, 2024 at 11:27 AM Nicolas Dufresne
> > <nicolas.dufresne@collabora.com> wrote:
> > > 
> > > Hi,
> > > 
> > > Le lundi 22 avril 2024 à 10:03 -0700, Douglas Anderson a écrit :
> > > > As talked about in commit 14d3ae2efeed ("ARM: 8507/1: dma-mapping: Use
> > > > DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc"), it doesn't
> > > > really make sense to try to allocate contiguous chunks of memory for
> > > > video encoding/decoding. Let's switch the Mediatek vcodec driver to
> > > > pass DMA_ATTR_ALLOC_SINGLE_PAGES and take some of the stress off the
> > > > memory subsystem.
> > > > 
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > > NOTE: I haven't personally done massive amounts of testing with this
> > > > change, but I originally added the DMA_ATTR_ALLOC_SINGLE_PAGES flag
> > > > specifically for the video encoding / decoding cases and I know it
> > > > helped avoid memory problems in the past on other systems. Colleagues
> > > > of mine have told me that with this change memory problems are harder
> > > > to reproduce, so it seems like we should consider doing it.
> > > 
> > > One thing to improve in your patch submission is to avoid abstracting the
> > > problems. Patch review and pulling is based on a technical rational and very
> > > rarely on the trust that it helps someone somewhere in some unknown context.
> > > What kind of memory issues are you facing ? What is the technical advantage of
> > > using DMA_ATTR_ALLOC_SINGLE_PAGES over the current approach that helps fixing
> > > the issue? I do expect this to be documented in the commit message itselfé.
> > 
> > Right. The problem here is that I'm not _directly_ facing any problems
> > here and I also haven't done massive amounts of analysis of the
> > Mediatek video codec. I know that some of my colleagues have run into
> > issues on Mediatek devices where the system starts getting
> > unresponsive when lots of videos are decoded in parallel. That
> > reminded me of the old problem I debugged in 2015 on Rockchip
> > platforms and is talked about a bunch in the referenced commit
> > 14d3ae2efeed ("ARM: 8507/1: dma-mapping: Use
> > DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc") so I wrote up
> > this patch. The referenced commit contains quite a bit of details
> > about the problems faced back in 2015.
> > 
> > When I asked, my colleagues said that my patch seemed to help, though
> > it was more of a qualitative statement than a quantitative one.
> 
> The story behind this is that I'm looking into an issue on the MediaTek
> MT8188 Chromebook, where in some scenarios the system may emit 30+
> video decoders concurrently (e.g. generating thumbnails for excess
> amount of video files etc.), and such behavior can easily hang the
> system if it has a smaller amount of memory (<4GB).
> 
> In addition to seeking mitigation in the user space software side,
> we're also looking for ways to optimize how the video decoders use
> memory, so Doug suggested this improvement.
> My preliminary experiment showed that it has some positive impact -
> the system doesn't freeze up completely with it and is still
> responsive in the UART serial console. However, just like mentioned, I
> didn’t have any rigorous numbers to support it.
> 
> To test the patch better, today I set up a local WebRTC demo to
> simulate a video conference with 49 people where the mocked input
> stream is captured from the device's own front camera.
> With that, the original system easily hung in less than one minute
> with less than 40MB available memory at the time; but with the change,
> the system ran for several minutes and had an average of over 100MB
> memory. It's not a huge improvement, but it's something.
> 
> I know this isn't the most scientific experiment, but I hope it’s a
> good enough representation of one of the multi video decoder use
> cases, and gives you some confidence that the patch is worth merging.
> 
> With the test above I think I can give this:
> Tested-by: Fei Shao <fshao@chromium.org>
> 
> And, since this patch LGTM and I support it, here's my humble
> Reviewed-by: Fei Shao <fshao@chromium.org>

The arguments hew and my own research has finish convincing me we want to do
this (unless we had limited TLB space at the device level, or performance metric
that show that bigger contiguous chunk helps).

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> Regards,
> Fei
> 
> > 
> > I wasn't 100% sure if it was worth sending the patch up at this point,
> > but logically, I think it makes sense. There aren't great reasons to
> > hog all the large chunks of memory for video decoding.
> > 
> > -Doug
>
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
index 9ce34a3b5ee6..3fb1d48c3e15 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
@@ -64,7 +64,8 @@  int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
 		id = dec_ctx->id;
 	}
 
-	mem->va = dma_alloc_coherent(&plat_dev->dev, size, &mem->dma_addr, GFP_KERNEL);
+	mem->va = dma_alloc_attrs(&plat_dev->dev, size, &mem->dma_addr,
+				  GFP_KERNEL, DMA_ATTR_ALLOC_SINGLE_PAGES);
 	if (!mem->va) {
 		mtk_v4l2_err(plat_dev, "%s dma_alloc size=%ld failed!",
 			     dev_name(&plat_dev->dev), size);