[PATCH V2] ALSA: hda - power setting error check

[PATCH V2] ALSA: hda - power setting error check

[Wang Xingchao]

codec may reject power state transition requests(reporting PS-ERROR set),
in that case we re-issue a power state setting and check error bit again.

Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
---
 sound/pci/hda/hda_codec.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index d0ca370..a93c7ca 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3526,6 +3526,7 @@ static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec, hda_nid_t fg
 static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 				unsigned int power_state)
 {
+	int state, count = 0;
 	if (codec->patch_ops.set_power_state) {
 		codec->patch_ops.set_power_state(codec, fg, power_state);
 		return;
@@ -3537,9 +3538,19 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 		bool epss = snd_hda_codec_get_supported_ps(codec, fg, AC_PWRST_EPSS);
 		msleep(epss? 10:100);
 	}
-	snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
-			    power_state);
-	snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
+
+	/* repeat power states setting at most 10 times*/
+	while (count++ > 10) {
+		snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
+				    power_state);
+		snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
+		state = snd_hda_codec_read(codec, fg, 0,
+					   AC_VERB_GET_POWER_STATE, 0);
+		if (!(state & AC_PWRST_ERROR))
+			break;
+	}
+	if (count > 10)
+		snd_printk(KERN_WARNING "Power states transition reject!\n");
 }
 
 /* modem wake on ring: transition from D2 to D0 in less than 2ms. For modems,
-- 
1.7.9.5

Re: [PATCH V2] ALSA: hda - power setting error check

[Fengguang Wu]

Xingchao,

On Thu, Jun 07, 2012 at 06:21:30PM +0800, Wang Xingchao wrote:
> codec may reject power state transition requests(reporting PS-ERROR set),
> in that case we re-issue a power state setting and check error bit again.

Is this patch based on real errors observed? And tested to fix the issue?

> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> ---
>  sound/pci/hda/hda_codec.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index d0ca370..a93c7ca 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -3526,6 +3526,7 @@ static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec, hda_nid_t fg
>  static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
>  				unsigned int power_state)
>  {
> +	int state, count = 0;

nit pick: break that into two lines?

state should be unsigned int.

>  	if (codec->patch_ops.set_power_state) {
>  		codec->patch_ops.set_power_state(codec, fg, power_state);
>  		return;
> @@ -3537,9 +3538,19 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
>  		bool epss = snd_hda_codec_get_supported_ps(codec, fg, AC_PWRST_EPSS);
>  		msleep(epss? 10:100);
>  	}
> -	snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
> -			    power_state);
> -	snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
> +
> +	/* repeat power states setting at most 10 times*/
> +	while (count++ > 10) {

< 10?

for(;;) looks slightly better, by putting the initialization together
with test and increase.

> +		snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
> +				    power_state);
> +		snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
> +		state = snd_hda_codec_read(codec, fg, 0,
> +					   AC_VERB_GET_POWER_STATE, 0);
> +		if (!(state & AC_PWRST_ERROR))
> +			break;

Do you have any clue the condition of success? For example, is it time
based (the codec takes some time to respond) or retry count based?

> +	}
> +	if (count > 10)
> +		snd_printk(KERN_WARNING "Power states transition reject!\n");

Power state transition rejected

Hmm, if some codec always rejects it, the user may get lots of these
warnings..

Thanks,
Fengguang

Fwd: [PATCH V2] ALSA: hda - power setting error check

[Wang Xingchao]

Hi fengguang,

2012/6/7 Fengguang Wu <fengguang.wu@intel.com>:
> Xingchao,
>
> On Thu, Jun 07, 2012 at 06:21:30PM +0800, Wang Xingchao wrote:
>> codec may reject power state transition requests(reporting PS-ERROR set),
>> in that case we re-issue a power state setting and check error bit again.
>
> Is this patch based on real errors observed? And tested to fix the issue?
>

Not exactly, i tested the patch on my netbook with codec ALC269 and
didnot meet regression. It's not based on real error but from the HDA
spec description about PowerStates setting part. The spec said
PS-ERROR bit indicates the power states transition rejection so we'd
better check the status when try to set  codec power state.

>> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
>> ---
>>  sound/pci/hda/hda_codec.c |   17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index d0ca370..a93c7ca 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -3526,6 +3526,7 @@ static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec, hda_nid_t fg
>>  static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
>>                               unsigned int power_state)
>>  {
>> +     int state, count = 0;
>
> nit pick: break that into two lines?
>
> state should be unsigned int.
>
>>       if (codec->patch_ops.set_power_state) {
>>               codec->patch_ops.set_power_state(codec, fg, power_state);
>>               return;
>> @@ -3537,9 +3538,19 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
>>               bool epss = snd_hda_codec_get_supported_ps(codec, fg, AC_PWRST_EPSS);
>>               msleep(epss? 10:100);
>>       }
>> -     snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
>> -                         power_state);
>> -     snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
>> +
>> +     /* repeat power states setting at most 10 times*/
>> +     while (count++ > 10) {
>
> < 10?
>
> for(;;) looks slightly better, by putting the initialization together
> with test and increase.
>

thanks your review above, i will fix these errors.

>> +             snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
>> +                                 power_state);
>> +             snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
>> +             state = snd_hda_codec_read(codec, fg, 0,
>> +                                        AC_VERB_GET_POWER_STATE, 0);
>> +             if (!(state & AC_PWRST_ERROR))
>> +                     break;
>
> Do you have any clue the condition of success? For example, is it time
> based (the codec takes some time to respond) or retry count based?
>

Good question. snd_hda_codec_read() will send the command and get the
response, but power setting verb id (705h) acturally has response "0"
all the time. As the spec said, if the node remain in its previous
power states, it sets PS-ERROR bit immediately. If the node doesnot
set PS-Error bit after it received the Set Power State Verb, that
means the transition is successful. That's why i didnot add some time
delay to wait for the response here.

>> +     }
>> +     if (count > 10)
>> +             snd_printk(KERN_WARNING "Power states transition reject!\n");
>
> Power state transition rejected
>
> Hmm, if some codec always rejects it, the user may get lots of these
> warnings..

Will remove the warning message here.

--thanks
xingchao