[alsa-devel] [PATCH] ALSA: hda - Enable runtime pm for Haswell

[Wang, Xingchao]

Hi Rafael,

> -----Original Message-----
> From: Wysocki, Rafael J
> Sent: Monday, May 27, 2013 9:02 PM
> To: Wang, Xingchao
> Cc: Takashi Iwai; Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
> alsa-devel at alsa-project.org; Wang Xingchao
> Subject: Re: [PATCH] ALSA: hda - Enable runtime pm for Haswell
> 
> On 5/23/2013 4:59 AM, Wang, Xingchao wrote:
> > Hi Rafael,
> >
> >
> >> -----Original Message-----
> >> From: Wysocki, Rafael J
> >> Sent: Wednesday, May 22, 2013 7:09 PM
> >> To: Takashi Iwai
> >> Cc: Wang, Xingchao; Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
> >> alsa-devel at alsa-project.org; Wang Xingchao
> >> Subject: Re: [PATCH] ALSA: hda - Enable runtime pm for Haswell
> >>
> >> On 5/22/2013 7:51 AM, Takashi Iwai wrote:
> >>> At Wed, 22 May 2013 03:56:00 +0000,
> >>> Wang, Xingchao wrote:
> >>>> Hi,
> >>>>
> >>>> Add Rafael in loop.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Takashi Iwai [mailto:tiwai at suse.de]
> >>>>> Sent: Thursday, May 16, 2013 4:49 PM
> >>>>> To: Wang Xingchao
> >>>>> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
> >>>>> alsa-devel at alsa-project.org; Wang, Xingchao
> >>>>> Subject: Re: [PATCH] ALSA: hda - Enable runtime pm for Haswell
> >>>>>
> >>>>> At Thu, 16 May 2013 16:29:05 +0800, Wang Xingchao wrote:
> >>>>>> Haswell doesnot support runtime pm by default.
> >>>>>> This patch let haswell Display HD-A controller enter runtime
> >>>>>> suspend, and bring more power saving whith power-well.
> >>>>>>
> >>>>>> Signed-off-by: Wang Xingchao <xingchao.wang at linux.intel.com>
> >>>>> I don't think it's good to fiddle such a thing in the driver side.
> >>>>> If Haswell can support runtime PM really, it should be set commonly.
> >>>> I wonder whether it's HD-A driver's policy to only support runtime
> >>>> PM if the
> >> device can support signal wakup?
> >>>> According to Rafael, the device can support runtime PM regardless,
> >>>> no
> >> matter it supports PME or not.
> >>>> If so, we should remove the "if" condition check here.
> >>> Well, if the decision is purely a driver issue, then we can get rid
> >>> of PME check.
> >> Yes, it is.
> >>
> >>>     But in that case, it should be simply like:
> >>>
> >>> azx_probe() {
> >>> 	...
> >>> 	if (chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
> >>> 		pm_runtime_put_noidle(&pci->dev);
> >>> 	...
> >>> }
> >>>
> >>> azx_remove() {
> >>> 	...
> >>> 	if (chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
> >>> 		pm_runtime_get_noresume(&pci->dev);
> >>> 	...
> >>> }
> >>>
> >>>
> >>> AFAIU, calling pm_runtime_allow() enables the runtime PM *forcibly*.
> >>> Usually this isn't a good thing.
> >>>
> >> That's correct.  Moreover, calling both pm_runtime_allow() *and*
> >> pm_runtime_put_noidle() together would be a bug IMO.
> > It's interesting something maybe wrong in my test:
> > 1. withtout calling pm_runtime_allow(), azx_runteim_idle/suspend() will not
> be called.
> 
> That's because you need to echo "auto" into the device's
> /sys/devices/.../power/control file from user space for it to work (most likely
> that's the reason).  If that file already contains "auto" when this happens, I'll
> need to look at the code to tell you what may be wrong.

Yes, it works after echo "auto" into hda pci device, it's "on" by default. :)

Thanks
---xingchao

> 
> > 2. another trick is on my Haswell ULT C stepping board, the runtime PM
> > only work after exit from resume:
> >
> > echo mem > /sys/power/state
> >
> > if you donot let system enter suspend manually, the runtime pm will not be
> triggered.
> >
> > Is there any dependency between runtime pm suspend and normal suspend?
> 
> There shouldn't be any like this, and if there's one, it is a bug most likely.
> 
> Thanks,
> Rafael