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

[Paul Kocialkowski]

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@xxxxxxxxxxxx>
> >> ---
> >>
> >> 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

Attachment: signature.asc
Description: PGP signature