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

Wang Xingchao wangxingchao2011 at gmail.com
Thu Jun 7 15:33:35 CEST 2012


Hi fengguang,

2012/6/7 Fengguang Wu <fengguang.wu at 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 at 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


More information about the Alsa-devel mailing list