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 ? > > > > 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
