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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200420151010.GL125838@aptenodytes>
Date:   Mon, 20 Apr 2020 17:10:10 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Jernej Škrabec <jernej.skrabec@...il.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-sunxi@...glegroups.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

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.

> > Anyway, I made a runtime PM support long time ago, but never do anything 
> > besides running few tests:
> > https://github.com/jernejsk/linux-1/commit/
> > d245b7fa2a26e519ff675a255c45230575a4a848
> > 
> > It takes a bit different approach. Power is enabled in start streaming and 
> > disabled in stop streaming. This is simpler approach and doesn't need 
> > autosuspend functionality. I also moved call to a function which sets format 
> > in HW registers to start streaming handler, so it's guaranteed to be set at 
> > the beginning.
> 
> Assuming cedrus_device_run() only gets called between streamon and streamoff
> (which appears to be the case), this looks like a better design.

Yes, this is defintiely ensured by the V4L2 framework. I agree that streamon/off
are the correct sync points.

> > Note that some registers are only set in start streaming handler. With your 
> > approach, if first frame is submitted too late, asserting and de-asserting 
> > reset line could reset those registers.
> 
> I only see registers being set in cedrus_start_streaming() after your patch,
> where you add a call to cedrus_dst_format_set(). I can't find any registers
> being written in any of the ->start() callbacks.
> 
> I'll send a v2 which combines the two patches: yours which handles the runtime
> part better, and mine which handles the probe/remove part better with !CONFIG_PM.

Thanks, that should do it!

Cheers,

Paul

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

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