[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<SE1P216MB130382E16D0CCD27341D535DEDA62@SE1P216MB1303.KORP216.PROD.OUTLOOK.COM>
Date: Fri, 12 Jul 2024 06:10:25 +0000
From: jackson.lee <jackson.lee@...psnmedia.com>
To: Nicolas Dufresne <nicolas.dufresne@...labora.com>, Devarsh Thakkar
<devarsht@...com>, "mchehab@...nel.org" <mchehab@...nel.org>,
"sebastian.fricke@...labora.com" <sebastian.fricke@...labora.com>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"hverkuil@...all.nl" <hverkuil@...all.nl>, Nas Chung
<nas.chung@...psnmedia.com>, lafley.kim <lafley.kim@...psnmedia.com>,
"b-brnich@...com" <b-brnich@...com>, "Luthra, Jai" <j-luthra@...com>, Vibhore
<vibhore@...com>, Dhruva Gole <d-gole@...com>, Aradhya <a-bhatia1@...com>,
"Raghavendra, Vignesh" <vigneshr@...com>
Subject: RE: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
suspend/resume
Hi Nicolas
> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Sent: Friday, June 21, 2024 2:33 AM
> To: Devarsh Thakkar <devarsht@...com>; jackson.lee
> <jackson.lee@...psnmedia.com>; mchehab@...nel.org;
> sebastian.fricke@...labora.com
> Cc: linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
> hverkuil@...all.nl; Nas Chung <nas.chung@...psnmedia.com>; lafley.kim
> <lafley.kim@...psnmedia.com>; b-brnich@...com; Luthra, Jai <j-luthra@...com>;
> Vibhore <vibhore@...com>; Dhruva Gole <d-gole@...com>; Aradhya <a-
> bhatia1@...com>; Raghavendra, Vignesh <vigneshr@...com>
> 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
Powered by blists - more mailing lists