linux-kernel - Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200421090139.GC610776@aptenodytes>
Date:   Tue, 21 Apr 2020 11:01:39 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     jernej.skrabec@...il.com, Samuel Holland <samuel@...lland.org>,
        linux-sunxi@...glegroups.com, Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-media <linux-media@...r.kernel.org>
Subject: Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

Hi Ezequiel,

On Tue 21 Apr 20, 00:51, Ezequiel Garcia wrote:
> Hi Jernej, Paul:
> 
> On Mon, 20 Apr 2020 at 13:41, Jernej Škrabec <jernej.skrabec@...il.com> wrote:
> >
> > Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> > napisal(a):
> > > Hi,
> > >
> > > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > > Hi Samuel!
> > > > >
> > > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > > > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > > > >>
> > > > >> Since the device is stateless, each frame gets a separate runtime PM
> > > > >> reference. Enable autosuspend so the PM callbacks are not run before
> > > > >> and
> > > > >> after every frame.
> > > > >>
> > > > >> Signed-off-by: Samuel Holland <samuel@...lland.org>
> > > > >> ---
> > > > >>
> > > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > > >> anything more complicated at the moment.
> > > > >>
> > > > >> ---
> > > > >>
> > > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > > >
> > > > [snip]
> > > >
> > > > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > > > a
> > > > > function is called, which sets few registers in HW. Of course, there is
> > > > > no
> > > > > guarantee that someone will start decoding immediately after capture
> > > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > > reset will clear those registers and decoded video will be in different
> > > > > format than expected. It could be even argued that registers should not
> > > > > be set in that function and that this is design issue or bug in driver.
> > > >
> > > > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > > > cedrus_engine_enable/disable().
> > >
> > > This might indeed be an issue with multiple decoding contexts in parallel,
> > > with potentially different formats. For that reason, it looks like the
> > > right thing to do would be to set the format at each decoding run based on
> > > the format set in the context by s_fmt.
> >
> > So you are suggesting that cedrus_dst_format_set() should be moved to
> > cedrus_device_run(), right? This way all registers are set at each run, which
> > is then truly stateless.
> >
> 
> BTW, this is how the Hantro and Rockchip's Rkvdec
> drivers are implemented. One of the main reasons is
> to have simultaneous decoding support.

That's a wise decision :)

I had actually tested that cedrus can decode multiple streams in parallel, but
I must have used the same format settings for both and didn't notice the issue
then.

Good that we are fixing it now!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)