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(). > 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. > 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. > Best regards, > Jernej Thanks for the review! Samuel
