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 >
