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

[Nicolas Dufresne]

Hi Jackson,

Le vendredi 12 juillet 2024 à 06:10 +0000, jackson.lee a écrit :
> Hi Nicolas
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > Sent: Friday, June 21, 2024 2:33 AM
> > To: Devarsh Thakkar <devarsht@xxxxxx>; jackson.lee
> > <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> > sebastian.fricke@xxxxxxxxxxxxx
> > Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > hverkuil@xxxxxxxxx; Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; lafley.kim
> > <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx; Luthra, Jai <j-luthra@xxxxxx>;
> > Vibhore <vibhore@xxxxxx>; Dhruva Gole <d-gole@xxxxxx>; Aradhya <a-
> > bhatia1@xxxxxx>; Raghavendra, Vignesh <vigneshr@xxxxxx>
> > Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> > suspend/resume
> > 
> > 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.
> > 
> 
> I measured latency for the resume and suspend of our hw block.
> 
> Resume : 124 microsecond
> Suspend : 355 microsecond
> 
> I think if the delay is 100ms, it is enough.
> How about this ?

Seem very fast operation indeed, so a very small delay seems logical. I believe
this is similar to what other drivers uses, so sounds good to me. 

**If** we decide to go lower or drop the delay, then I'd like see someones
benchmark that show that doing suspend during playback does improve power
efficiently without reducing throughput.

Nicolas

> 
> > > 
> > > 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.
> > 
> 
> After 5sec, the hw goes to suspend. So there is no bug in the current patch-set.
> 
> 
> Thanks
> 
> 
> > 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
>