Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime suspend/resume

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> Hi Nicolas,
> 
> On 20/06/24 19:35, Nicolas Dufresne wrote:
> > Hi Devarsh,
> > 
> > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > 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.
> > 
> > It sounds like you did proper scientific testing of the suspend results calls,
> > mind sharing the actual data ?
> 
> Nopes, I did not do that but yes I agree it is good to profile and evaluate
> the trade-off but I am not expecting 250ms kind of latency. I would suggest
> Jackson to do the profiling for the resume latencies.

I'd clearly like to see numbers before we proceed.

> 
> But perhaps a separate issue, I did notice that intention of the patchset was
> to suspend without waiting for the timeout if there is no application having a
> handle to the wave5 device but even if I close the last instance I still see
> the IP stays on for 5seconds as seen in this logs [1] and this perhaps could
> be because extra pm counter references being hold.

Not sure where this comes from, I'm not aware of drivers doing that with M2M
instances. Only 

> 
> [2024-06-20 12:32:50] Freeing pipeline ...
> 
> and after 5 seconds..
> 
> [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
> [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> 
> [1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90

Appart from the 5s being too long, that is expected. If it fails after that,
this is a bug, we we should hold on merging this until the problem has been
resolved.

Imagine that userspace is going gapless playback, if you have a lets say 30ms on
forced suspend cycle due to close/open of the decoder instance, it won't
actually endup gapless. The delay will ensure that we only suspend when needed.

There is other changes I have asked in this series, since we always have the
case where userspace just pause on streaming, and we want that prolonged paused
lead to suspend. Hopefully this has been strongly tested and is not just added
for "completeness".

Its important to note that has a reviewer only, my time is limited, and I
completely rely on the author judgment of delay tuning and actual testing.

Nicolas

> 
> Regards
> Devarsh