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

[Jernej Åkrabec]

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

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.

Best regards,
Jernej

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