Message-ID: <d73d7ade60de2bf4c4e9048756dc59062af64bc4.camel@collabora.com>
Date: Wed, 01 May 2024 14:31:43 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Fei Shao <fshao@...omium.org>, Doug Anderson <dianders@...omium.org>
Cc: Tiffany Lin <tiffany.lin@...iatek.com>, Andrew-CT Chen
 <andrew-ct.chen@...iatek.com>, Yunfei Dong <yunfei.dong@...iatek.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@...labora.com>, Wei-Shun Chang
 <weishunc@...gle.com>,  Hans Verkuil <hverkuil-cisco@...all.nl>,
 Nícolas "F. R. A. Prado" <nfraprado@...labora.com>, Rob
 Herring <robh@...nel.org>,  linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org,  linux-media@...r.kernel.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] media: mediatek: vcodec: Alloc DMA memory with
 DMA_ATTR_ALLOC_SINGLE_PAGES

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@...omium.org> wrote:
> > 
> > Hi,
> > 
> > On Mon, Apr 22, 2024 at 11:27 AM Nicolas Dufresne
> > <nicolas.dufresne@...labora.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@...omium.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@...omium.org>
> 
> And, since this patch LGTM and I support it, here's my humble
> Reviewed-by: Fei Shao <fshao@...omium.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@...labora.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
> 

Powered by blists - more mailing lists