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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 5744 bytes --]

Hi,

On Mon 20 Apr 20, 20:09, Jernej Škrabec wrote:
> Hi!
> 
> Dne ponedeljek, 20. april 2020 ob 19:56:51 CEST je Paul Kocialkowski 
> napisal(a):
> > Hi Jernej,
> > 
> > On Mon 20 Apr 20, 18:41, Jernej Škrabec 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@sholland.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.
> > 
> > Yes, exactly! But this is outside of the scope of this series though.
> 
> I'm not sure about being out of scope. It has to be moved anyway, so why not 
> put it in best place?

I agree it should be moved to cedrus_device_run, for sure!

What I meant is that Samuel doesn't have to make that change with the runtime PM
patch series because it's a different issue. But reading the discussion again,
it's true that there is a chance of the VPU going to sleep between set fmt and
start streaming, so we should probably take care of it before merging runtime
PM.

Samuel, do you feel like making that change with your series?
I can do it otherwise.

Cheers,

Paul

> Best
> 
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > 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
> 
> 
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]