* [PATCH V2] ALSA: hda - power setting error check @ 2012-06-07 10:21 Wang Xingchao 2012-06-07 10:42 ` Fengguang Wu 0 siblings, 1 reply; 3+ messages in thread From: Wang Xingchao @ 2012-06-07 10:21 UTC (permalink / raw) To: tiwai, alsa-devel, fengguang.wu; +Cc: wangxingchao2011, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH V2] ALSA: hda - power setting error check 2012-06-07 10:21 [PATCH V2] ALSA: hda - power setting error check Wang Xingchao @ 2012-06-07 10:42 ` Fengguang Wu 2012-06-07 13:33 ` Fwd: " Wang Xingchao 0 siblings, 1 reply; 3+ messages in thread From: Fengguang Wu @ 2012-06-07 10:42 UTC (permalink / raw) To: Wang Xingchao; +Cc: tiwai, alsa-devel, wangxingchao2011 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Fwd: [PATCH V2] ALSA: hda - power setting error check 2012-06-07 10:42 ` Fengguang Wu @ 2012-06-07 13:33 ` Wang Xingchao 0 siblings, 0 replies; 3+ messages in thread From: Wang Xingchao @ 2012-06-07 13:33 UTC (permalink / raw) To: Wu Fengguang; +Cc: Takashi Iwai, Alsa-devel, 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-06-07 13:33 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-07 10:21 [PATCH V2] ALSA: hda - power setting error check Wang Xingchao 2012-06-07 10:42 ` Fengguang Wu 2012-06-07 13:33 ` Fwd: " Wang Xingchao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.