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

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

[Wang Xingchao]

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@linux.intel.com>
---
 sound/pci/hda/hda_intel.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index bf27693..eb25888 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
 
 	if (pci_dev_run_wake(pci))
 		pm_runtime_put_noidle(&pci->dev);
+	else if (chip->driver_caps
+			& AZX_DCAPS_I915_POWERWELL) {
+		/* Haswell doesnot support runtime pm by default */
+		pm_runtime_put_noidle(&pci->dev);
+		pm_runtime_allow(&pci->dev);
+	}
 
 	dev++;
 	complete_all(&chip->probe_wait);
-- 
1.7.9.5

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

[Takashi Iwai]

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@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.


Takashi

> ---
>  sound/pci/hda/hda_intel.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index bf27693..eb25888 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
>  
>  	if (pci_dev_run_wake(pci))
>  		pm_runtime_put_noidle(&pci->dev);
> +	else if (chip->driver_caps
> +			& AZX_DCAPS_I915_POWERWELL) {
> +		/* Haswell doesnot support runtime pm by default */
> +		pm_runtime_put_noidle(&pci->dev);
> +		pm_runtime_allow(&pci->dev);
> +	}
>  
>  	dev++;
>  	complete_all(&chip->probe_wait);
> -- 
> 1.7.9.5
> 

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

[Wang, Xingchao]

Hi Takashi,


> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, May 16, 2013 4:49 PM
> To: Wang Xingchao
> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn; alsa-devel@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@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.

Haswell doesnot show its power capability to support PME, I'm not sure it's caused by BIOS setting
or need some additional configurations in PCI registers.
So pci_dev_run_wake() return false and  this make haswell lose
the chance to support runtime pm in software, right?

If a PCI device would support runtime pm in software, maybe it doesnot support PME in hardware level, it should
get a chance to try. I see some pci devices, they look much like:

pm_runtime_put_noidle(&pdev->dev);
pm_runtime_allow(&pdev->dev);

anyway it's better if we can set haswell PME capability externally,  then it fits current code.
but if there's such software requirement for runtime pm, it should be improved, right?

Thanks
--xingchao
> 
> 
> Takashi
> 
> > ---
> >  sound/pci/hda/hda_intel.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index bf27693..eb25888 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
> >
> >  	if (pci_dev_run_wake(pci))
> >  		pm_runtime_put_noidle(&pci->dev);
> > +	else if (chip->driver_caps
> > +			& AZX_DCAPS_I915_POWERWELL) {
> > +		/* Haswell doesnot support runtime pm by default */
> > +		pm_runtime_put_noidle(&pci->dev);
> > +		pm_runtime_allow(&pci->dev);
> > +	}
> >
> >  	dev++;
> >  	complete_all(&chip->probe_wait);
> > --
> > 1.7.9.5
> >

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

[Wang, Xingchao]

Hi,

Add Rafael in loop.

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, May 16, 2013 4:49 PM
> To: Wang Xingchao
> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn; alsa-devel@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@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.

Thanks
--xingchao

> 
> 
> Takashi
> 
> > ---
> >  sound/pci/hda/hda_intel.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index bf27693..eb25888 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
> >
> >  	if (pci_dev_run_wake(pci))
> >  		pm_runtime_put_noidle(&pci->dev);
> > +	else if (chip->driver_caps
> > +			& AZX_DCAPS_I915_POWERWELL) {
> > +		/* Haswell doesnot support runtime pm by default */
> > +		pm_runtime_put_noidle(&pci->dev);
> > +		pm_runtime_allow(&pci->dev);
> > +	}
> >
> >  	dev++;
> >  	complete_all(&chip->probe_wait);
> > --
> > 1.7.9.5
> >

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

[Takashi Iwai]

At Wed, 22 May 2013 03:56:00 +0000,
Wang, Xingchao wrote:
> 
> Hi,
> 
> Add Rafael in loop.
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, May 16, 2013 4:49 PM
> > To: Wang Xingchao
> > Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn; alsa-devel@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@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.  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.


thanks,

Takashi

> 
> Thanks
> --xingchao
> 
> > 
> > 
> > Takashi
> > 
> > > ---
> > >  sound/pci/hda/hda_intel.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index bf27693..eb25888 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
> > >
> > >  	if (pci_dev_run_wake(pci))
> > >  		pm_runtime_put_noidle(&pci->dev);
> > > +	else if (chip->driver_caps
> > > +			& AZX_DCAPS_I915_POWERWELL) {
> > > +		/* Haswell doesnot support runtime pm by default */
> > > +		pm_runtime_put_noidle(&pci->dev);
> > > +		pm_runtime_allow(&pci->dev);
> > > +	}
> > >
> > >  	dev++;
> > >  	complete_all(&chip->probe_wait);
> > > --
> > > 1.7.9.5
> > >
> 

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

[Wang, Xingchao]

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@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@suse.de]
> >>> Sent: Thursday, May 16, 2013 4:49 PM
> >>> To: Wang Xingchao
> >>> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
> >>> alsa-devel@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@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.

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?

Thanks
--xingchao


> 
> Thanks,
> Rafael
> 
> 
> >
> >>>> ---
> >>>>   sound/pci/hda/hda_intel.c |    6 ++++++
> >>>>   1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >>>> index bf27693..eb25888 100644
> >>>> --- a/sound/pci/hda/hda_intel.c
> >>>> +++ b/sound/pci/hda/hda_intel.c
> >>>> @@ -3755,6 +3755,12 @@ static int azx_probe(struct pci_dev *pci,
> >>>>
> >>>>   	if (pci_dev_run_wake(pci))
> >>>>   		pm_runtime_put_noidle(&pci->dev);
> >>>> +	else if (chip->driver_caps
> >>>> +			& AZX_DCAPS_I915_POWERWELL) {
> >>>> +		/* Haswell doesnot support runtime pm by default */
> >>>> +		pm_runtime_put_noidle(&pci->dev);
> >>>> +		pm_runtime_allow(&pci->dev);
> >>>> +	}
> >>>>
> >>>>   	dev++;
> >>>>   	complete_all(&chip->probe_wait);
> >>>> --
> >>>> 1.7.9.5
> >>>>

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

[Rafael J. Wysocki]

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@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@suse.de]
>>>>> Sent: Thursday, May 16, 2013 4:49 PM
>>>>> To: Wang Xingchao
>>>>> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
>>>>> alsa-devel@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@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.

> 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

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [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@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@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@suse.de]
> >>>>> Sent: Thursday, May 16, 2013 4:49 PM
> >>>>> To: Wang Xingchao
> >>>>> Cc: Girdwood, Liam R; Lin, Mengdong; Li, Jocelyn;
> >>>>> alsa-devel@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@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