Date: Thu, 20 Jun 2024 15:05:23 +0530
From: Devarsh Thakkar <devarsht@...com>
To: "jackson.lee" <jackson.lee@...psnmedia.com>,
        "mchehab@...nel.org"
	<mchehab@...nel.org>,
        "nicolas@...fresne.ca" <nicolas@...fresne.ca>,
        "sebastian.fricke@...labora.com" <sebastian.fricke@...labora.com>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hverkuil@...all.nl" <hverkuil@...all.nl>,
        Nas Chung
	<nas.chung@...psnmedia.com>,
        "lafley.kim" <lafley.kim@...psnmedia.com>,
        "b-brnich@...com" <b-brnich@...com>,
        Nicolas Dufresne
	<nicolas.dufresne@...labora.com>,
        "Luthra, Jai" <j-luthra@...com>, Vibhore
	<vibhore@...com>,
        Dhruva Gole <d-gole@...com>, Aradhya <a-bhatia1@...com>,
        "Raghavendra, Vignesh" <vigneshr@...com>
Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
 suspend/resume

Hi Jackson,

On 20/06/24 05:41, jackson.lee wrote:
> 
> 
>> -----Original Message-----
>> From: jackson.lee
>> Sent: Thursday, June 20, 2024 8:57 AM
>> To: Devarsh Thakkar <devarsht@...com>; mchehab@...nel.org;
>> nicolas@...fresne.ca; sebastian.fricke@...labora.com
>> Cc: linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
>> hverkuil@...all.nl; Nas Chung <nas.chung@...psnmedia.com>; lafley.kim
>> <lafley.kim@...psnmedia.com>; b-brnich@...com; Nicolas Dufresne
>> <nicolas.dufresne@...labora.com>
>> Subject: RE: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
>> suspend/resume
>>
>> Hi Devarsh
>>
>> If there is no feeding bitstreams during encoding and decoding frames, then
>> driver's status is switched to suspended automatically by autosuspend.

I think the pm_runtime_*_autosuspend helpers are to schedule a delayed suspend
i.e. after the pm counter goes to 0, suspend the device after timeout period
which is set to 5s in this case.

Even without using the pm_runtime_*_autosuspend helpers, i.e if you use
pm_runtime_resume_and_get on start streaming and pm_runtime_put_sync on stop
streaming the device gets suspended automatically if not in use albeit
immediately after the pm counter goes to 0. And this is what many codec
devices drivers do today [1]. Ain't that suffice what we want ?

In my view the delayed suspend functionality is generally helpful for devices
where resume latencies are higher for e.g. this light sensor driver [2] uses
it because it takes 250ms to stabilize after resumption and I don't see this
being used in codec drivers generally since there is no such large resume
latency. Please let me know if I am missing something or there is a strong
reason to have delayed suspend for wave5.

>> And if we don’t use autosuspend, it is very difficult for us to catch if
>> there is feeding or not while working a pipeline.
>> So it is very efficient for managing power status.

As mentioned above, if you mean by autosuspend that device should
automatically suspend if not used then you don't require to use
pm_runtime_*_autosuspend helpers (as those are for delayed suspend actually)
and instead use the generic pm helpers pm_runtime_resume_and_get and
pm_runtime_put_sync and PM core will automatically suspend the device when pm
counter drops to 0 and resume it back when pm counter is incremented.

>>
>> If the delay is very great value, we can adjust it.
>>

As mentioned above, I feel we don't require to use pm_runtime_*_autosuspend
helpers at first place.

>> Thanks
>> Jackson
>>
> 
> One more thing, 
> 
> When an instance is closed or started, we are currently putting a power status to suspend or resumed immediately.
So I tested this series and see below issues :

1) I see it seems to break VPU operation on AM62A using upstream linux-next
colliding with the polling functionality there since that device does not have
an irq and relies on polling as I see below logs on bootup :

[2024-06-20 13:01:12] root@...2axx-evm:~# dmesg | tail
[2024-06-20 13:01:16] [   23.744372] x8 : ffff000804248a50 x7 :
ffff00087f6ba0c0 x6 : 0000000000000000
[2024-06-20 13:01:16] [   23.744381] x5 : 0000000000f42400 x4 :
0000000000000000 x3 : 0000000000000001
[2024-06-20 13:01:16] [   23.744390] x2 : ffff0008041ad808 x1 :
0000000000000044 x0 : ffff800082150044
[2024-06-20 13:01:16] [   23.744400] Call trace:
[2024-06-20 13:01:16] [   23.744404]  wave5_vdi_read_register+0x8/0x20 [wave5]
[2024-06-20 13:01:16] [   23.744420]  kthread_worker_fn+0xcc/0x184
[2024-06-20 13:01:16] [   23.744432]  kthread+0x118/0x11c
[2024-06-20 13:01:16] [   23.744440]  ret_from_fork+0x10/0x20
[2024-06-20 13:01:16] [   23.744452] Code: b9000022 d65f03c0 f940b000 8b214000
(b9400000)
[2024-06-20 13:01:16] [   23.744458] ---[ end trace 0000000000000000 ]---

I think care needs to be taken to make sure timer is started after device is
powered on and stopped before device gets powered off.


> The autospend feature is being only used when there is no feeding while working a pipeline.

2) I think above doesn't seem to work, Brandon had a hack patch on vendor tree
[3] for AM62A timer, with that I no longer see above crash issue but I observe
that there is a 5 second wait to power off device even after last instance is
closed as seen here [4], seems like power counter is not getting set to 0 on
instance close, you may try to reproduce the same on j721s2 evm too.


[1]:
https://gitlab.com/linux-kernel/linux-next/-/blob/master/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c?ref_type=heads#L1637
[2]:
https://gitlab.com/linux-kernel/linux-next/-/blob/next-20240619/drivers/iio/light/bh1780.c?ref_type=tags#L179
[3]:
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.6.y-cicd&id=0be8de03825c2834a39af603b088cbf31e19d55d
[4]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90

Regards
Devarsh

Powered by blists - more mailing lists