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
