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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <SE1P216MB1303BD27042E889B9CDC07E4EDA22@SE1P216MB1303.KORP216.PROD.OUTLOOK.COM>
Date: Tue, 16 Jul 2024 01:02:31 +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

Thanks for your reply.

> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Sent: Tuesday, July 16, 2024 2:06 AM
> To: jackson.lee <jackson.lee@...psnmedia.com>; Devarsh Thakkar
> <devarsht@...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
> 
> Hi Jackson,
> 
> Le vendredi 12 juillet 2024 à 06:10 +0000, jackson.lee a écrit :
> > 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 ?
> 
> 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
> >