Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM
To:     =?UTF-8?Q?Jernej_=c5=a0krabec?= <jernej.skrabec@gmail.com>,
        Maxime Ripard <mripard@kernel.org>,
        Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-sunxi@googlegroups.com
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org
References: <20200408010232.48432-1-samuel@sholland.org>
 <9673642.nUPlyArG6x@jernej-laptop>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <244922ec-ed3a-eca7-6640-49de7ad9c605@sholland.org>
Date:   Sun, 19 Apr 2020 15:28:30 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <9673642.nUPlyArG6x@jernej-laptop>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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().

> 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