Hi Nicolas Thanks for your reply. > -----Original Message----- > From: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx> > Sent: Tuesday, July 16, 2024 2:06 AM > To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; Devarsh Thakkar > <devarsht@xxxxxx>; 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 > > 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 > >
