* [PATCH] ALSA: HDA - Check return value to reduce useless delay
@ 2012-10-24 6:53 Wang Xingchao
2012-10-24 22:00 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Wang Xingchao @ 2012-10-24 6:53 UTC (permalink / raw)
To: tiwai, alsa-devel; +Cc: Wang Xingchao
For verb 705h, it's useless to read response, so use *write api would be
better. If there's error after sending cmd, just try again without continue
after wrong operation.Otherwise there's long time delay.
Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
---
sound/pci/hda/hda_codec.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 960800b..0946eca 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3569,6 +3569,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
int count;
unsigned int state;
+ int err;
/* this delay seems necessary to avoid click noise at power-down */
if (power_state == AC_PWRST_D3) {
@@ -3582,9 +3583,11 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
codec->patch_ops.set_power_state(codec, fg,
power_state);
else {
- snd_hda_codec_read(codec, fg, 0,
+ err = snd_hda_codec_write(codec, fg, 0,
AC_VERB_SET_POWER_STATE,
power_state);
+ if (err < 0)
+ continue;
snd_hda_codec_set_power_to_all(codec, fg, power_state,
true);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
2012-10-24 6:53 [PATCH] ALSA: HDA - Check return value to reduce useless delay Wang Xingchao
@ 2012-10-24 22:00 ` Takashi Iwai
2012-10-25 10:30 ` Wang, Xingchao
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2012-10-24 22:00 UTC (permalink / raw)
To: Wang Xingchao; +Cc: alsa-devel
At Wed, 24 Oct 2012 14:53:23 +0800,
Wang Xingchao wrote:
>
> For verb 705h, it's useless to read response, so use *write api would be
> better. If there's error after sending cmd, just try again without continue
> after wrong operation.Otherwise there's long time delay.
Well, this is a bit sensitive part. Did you do the good test coverage
for different hardware controllers and codecs, including the non-Intel
ones?
Takashi
> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> ---
> sound/pci/hda/hda_codec.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 960800b..0946eca 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -3569,6 +3569,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
> hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
> int count;
> unsigned int state;
> + int err;
>
> /* this delay seems necessary to avoid click noise at power-down */
> if (power_state == AC_PWRST_D3) {
> @@ -3582,9 +3583,11 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
> codec->patch_ops.set_power_state(codec, fg,
> power_state);
> else {
> - snd_hda_codec_read(codec, fg, 0,
> + err = snd_hda_codec_write(codec, fg, 0,
> AC_VERB_SET_POWER_STATE,
> power_state);
> + if (err < 0)
> + continue;
> snd_hda_codec_set_power_to_all(codec, fg, power_state,
> true);
> }
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
2012-10-24 22:00 ` Takashi Iwai
@ 2012-10-25 10:30 ` Wang, Xingchao
2012-10-25 11:08 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Wang, Xingchao @ 2012-10-25 10:30 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, October 25, 2012 6:01 AM
> To: Wang, Xingchao
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
>
> At Wed, 24 Oct 2012 14:53:23 +0800,
> Wang Xingchao wrote:
> >
> > For verb 705h, it's useless to read response, so use *write api would
> > be better. If there's error after sending cmd, just try again without
> > continue after wrong operation.Otherwise there's long time delay.
>
> Well, this is a bit sensitive part. Did you do the good test coverage for
> different hardware controllers and codecs, including the non-Intel ones?
>
Well I only tested the patch on Haswell/Ivybridge platform. For Haswell I meet an issue about codec suspend/resume.
There's no response from codec when there's power state transition from D3 to D0, and the patch could reduced delay time.
For ivybridge the codec works well, so this patch has no active behavior.
I assume the verb 705h will not work for subnodes if it failed at first for "function group".
Is there such case when set power state for the "Audio Function Group" fail but the sub-nodes could keep on setting power?
Thanks
--xingchao
>
> Takashi
>
> > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > ---
> > sound/pci/hda/hda_codec.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 960800b..0946eca 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -3569,6 +3569,7 @@ static unsigned int hda_set_power_state(struct
> hda_codec *codec,
> > hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
> > int count;
> > unsigned int state;
> > + int err;
> >
> > /* this delay seems necessary to avoid click noise at power-down */
> > if (power_state == AC_PWRST_D3) {
> > @@ -3582,9 +3583,11 @@ static unsigned int hda_set_power_state(struct
> hda_codec *codec,
> > codec->patch_ops.set_power_state(codec, fg,
> > power_state);
> > else {
> > - snd_hda_codec_read(codec, fg, 0,
> > + err = snd_hda_codec_write(codec, fg, 0,
> > AC_VERB_SET_POWER_STATE,
> > power_state);
> > + if (err < 0)
> > + continue;
> > snd_hda_codec_set_power_to_all(codec, fg, power_state,
> > true);
> > }
> > --
> > 1.7.9.5
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
2012-10-25 10:30 ` Wang, Xingchao
@ 2012-10-25 11:08 ` Takashi Iwai
2012-10-26 10:45 ` Wang, Xingchao
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2012-10-25 11:08 UTC (permalink / raw)
To: Wang, Xingchao; +Cc: alsa-devel@alsa-project.org
At Thu, 25 Oct 2012 10:30:35 +0000,
Wang, Xingchao wrote:
>
>
>
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, October 25, 2012 6:01 AM
> > To: Wang, Xingchao
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
> >
> > At Wed, 24 Oct 2012 14:53:23 +0800,
> > Wang Xingchao wrote:
> > >
> > > For verb 705h, it's useless to read response, so use *write api would
> > > be better. If there's error after sending cmd, just try again without
> > > continue after wrong operation.Otherwise there's long time delay.
> >
> > Well, this is a bit sensitive part. Did you do the good test coverage for
> > different hardware controllers and codecs, including the non-Intel ones?
> >
>
> Well I only tested the patch on Haswell/Ivybridge platform.
Both are too new :)
> For Haswell I meet an issue about codec suspend/resume.
> There's no response from codec when there's power state transition from D3 to D0, and the patch could reduced delay time.
But does it fix the issue itself?
> For ivybridge the codec works well, so this patch has no active behavior.
> I assume the verb 705h will not work for subnodes if it failed at first for "function group".
> Is there such case when set power state for the "Audio Function Group" fail but the sub-nodes could keep on setting power?
Not sure. But we need more testing obviously before breaking
anything.
thanks,
Takashi
>
> Thanks
> --xingchao
>
> >
> > Takashi
> >
> > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > > ---
> > > sound/pci/hda/hda_codec.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > index 960800b..0946eca 100644
> > > --- a/sound/pci/hda/hda_codec.c
> > > +++ b/sound/pci/hda/hda_codec.c
> > > @@ -3569,6 +3569,7 @@ static unsigned int hda_set_power_state(struct
> > hda_codec *codec,
> > > hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
> > > int count;
> > > unsigned int state;
> > > + int err;
> > >
> > > /* this delay seems necessary to avoid click noise at power-down */
> > > if (power_state == AC_PWRST_D3) {
> > > @@ -3582,9 +3583,11 @@ static unsigned int hda_set_power_state(struct
> > hda_codec *codec,
> > > codec->patch_ops.set_power_state(codec, fg,
> > > power_state);
> > > else {
> > > - snd_hda_codec_read(codec, fg, 0,
> > > + err = snd_hda_codec_write(codec, fg, 0,
> > > AC_VERB_SET_POWER_STATE,
> > > power_state);
> > > + if (err < 0)
> > > + continue;
> > > snd_hda_codec_set_power_to_all(codec, fg, power_state,
> > > true);
> > > }
> > > --
> > > 1.7.9.5
> > >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
2012-10-25 11:08 ` Takashi Iwai
@ 2012-10-26 10:45 ` Wang, Xingchao
2012-10-26 10:59 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Wang, Xingchao @ 2012-10-26 10:45 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, October 25, 2012 7:08 PM
> To: Wang, Xingchao
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
>
> At Thu, 25 Oct 2012 10:30:35 +0000,
> Wang, Xingchao wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Thursday, October 25, 2012 6:01 AM
> > > To: Wang, Xingchao
> > > Cc: alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce
> > > useless delay
> > >
> > > At Wed, 24 Oct 2012 14:53:23 +0800,
> > > Wang Xingchao wrote:
> > > >
> > > > For verb 705h, it's useless to read response, so use *write api
> > > > would be better. If there's error after sending cmd, just try
> > > > again without continue after wrong operation.Otherwise there's long
> time delay.
> > >
> > > Well, this is a bit sensitive part. Did you do the good test
> > > coverage for different hardware controllers and codecs, including the
> non-Intel ones?
> > >
> >
> > Well I only tested the patch on Haswell/Ivybridge platform.
>
> Both are too new :)
>
> > For Haswell I meet an issue about codec suspend/resume.
> > There's no response from codec when there's power state transition from D3
> to D0, and the patch could reduced delay time.
>
> But does it fix the issue itself?
Actually not. :)
>
> > For ivybridge the codec works well, so this patch has no active behavior.
> > I assume the verb 705h will not work for subnodes if it failed at first for
> "function group".
> > Is there such case when set power state for the "Audio Function Group" fail
> but the sub-nodes could keep on setting power?
>
> Not sure. But we need more testing obviously before breaking anything.
Okay, I will try to test it on other platforms. :)
--xingchao
>
>
> thanks,
>
> Takashi
>
>
> >
> > Thanks
> > --xingchao
> >
> > >
> > > Takashi
> > >
> > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > > > ---
> > > > sound/pci/hda/hda_codec.c | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > > index 960800b..0946eca 100644
> > > > --- a/sound/pci/hda/hda_codec.c
> > > > +++ b/sound/pci/hda/hda_codec.c
> > > > @@ -3569,6 +3569,7 @@ static unsigned int
> > > > hda_set_power_state(struct
> > > hda_codec *codec,
> > > > hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
> > > > int count;
> > > > unsigned int state;
> > > > + int err;
> > > >
> > > > /* this delay seems necessary to avoid click noise at power-down */
> > > > if (power_state == AC_PWRST_D3) { @@ -3582,9 +3583,11 @@
> static
> > > > unsigned int hda_set_power_state(struct
> > > hda_codec *codec,
> > > > codec->patch_ops.set_power_state(codec, fg,
> > > > power_state);
> > > > else {
> > > > - snd_hda_codec_read(codec, fg, 0,
> > > > + err = snd_hda_codec_write(codec, fg, 0,
> > > > AC_VERB_SET_POWER_STATE,
> > > > power_state);
> > > > + if (err < 0)
> > > > + continue;
> > > > snd_hda_codec_set_power_to_all(codec, fg, power_state,
> > > > true);
> > > > }
> > > > --
> > > > 1.7.9.5
> > > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
2012-10-26 10:45 ` Wang, Xingchao
@ 2012-10-26 10:59 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2012-10-26 10:59 UTC (permalink / raw)
To: Wang, Xingchao; +Cc: alsa-devel@alsa-project.org
At Fri, 26 Oct 2012 10:45:33 +0000,
Wang, Xingchao wrote:
>
>
>
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, October 25, 2012 7:08 PM
> > To: Wang, Xingchao
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay
> >
> > At Thu, 25 Oct 2012 10:30:35 +0000,
> > Wang, Xingchao wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Thursday, October 25, 2012 6:01 AM
> > > > To: Wang, Xingchao
> > > > Cc: alsa-devel@alsa-project.org
> > > > Subject: Re: [PATCH] ALSA: HDA - Check return value to reduce
> > > > useless delay
> > > >
> > > > At Wed, 24 Oct 2012 14:53:23 +0800,
> > > > Wang Xingchao wrote:
> > > > >
> > > > > For verb 705h, it's useless to read response, so use *write api
> > > > > would be better. If there's error after sending cmd, just try
> > > > > again without continue after wrong operation.Otherwise there's long
> > time delay.
> > > >
> > > > Well, this is a bit sensitive part. Did you do the good test
> > > > coverage for different hardware controllers and codecs, including the
> > non-Intel ones?
> > > >
> > >
> > > Well I only tested the patch on Haswell/Ivybridge platform.
> >
> > Both are too new :)
> >
> > > For Haswell I meet an issue about codec suspend/resume.
> > > There's no response from codec when there's power state transition from D3
> > to D0, and the patch could reduced delay time.
> >
> > But does it fix the issue itself?
>
> Actually not. :)
> >
> > > For ivybridge the codec works well, so this patch has no active behavior.
> > > I assume the verb 705h will not work for subnodes if it failed at first for
> > "function group".
> > > Is there such case when set power state for the "Audio Function Group" fail
> > but the sub-nodes could keep on setting power?
> >
> > Not sure. But we need more testing obviously before breaking anything.
>
> Okay, I will try to test it on other platforms. :)
Thanks.
But thinking this twice, I think the only case it matters is that when
the codec returns a bogus error at the power up. As the original
code, it continues to power up all sub nodes. But with your patch,
the sub node power-up will be skipped completely.
So, a compromise option would be:
- Keep using snd_hda_codec_read() to sync with the power up of FG
node; snd_hda_codec_write() is asynchronous, so this can be too fast
for power up of sub nodes, and...
- Check the return value from the fg node, skip the rest and continue,
except for the last try. (Maybe we should add some delay for the
repeated try here, though.) And in the last loop count, forcibly
continue to the power up of the rest.
In this way, most of time consumed for the broken power up case you
faced should be avoided.
Takashi
> --xingchao
> >
> >
> > thanks,
> >
> > Takashi
> >
> >
> > >
> > > Thanks
> > > --xingchao
> > >
> > > >
> > > > Takashi
> > > >
> > > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > > > > ---
> > > > > sound/pci/hda/hda_codec.c | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > > > index 960800b..0946eca 100644
> > > > > --- a/sound/pci/hda/hda_codec.c
> > > > > +++ b/sound/pci/hda/hda_codec.c
> > > > > @@ -3569,6 +3569,7 @@ static unsigned int
> > > > > hda_set_power_state(struct
> > > > hda_codec *codec,
> > > > > hda_nid_t fg = codec->afg ? codec->afg : codec->mfg;
> > > > > int count;
> > > > > unsigned int state;
> > > > > + int err;
> > > > >
> > > > > /* this delay seems necessary to avoid click noise at power-down */
> > > > > if (power_state == AC_PWRST_D3) { @@ -3582,9 +3583,11 @@
> > static
> > > > > unsigned int hda_set_power_state(struct
> > > > hda_codec *codec,
> > > > > codec->patch_ops.set_power_state(codec, fg,
> > > > > power_state);
> > > > > else {
> > > > > - snd_hda_codec_read(codec, fg, 0,
> > > > > + err = snd_hda_codec_write(codec, fg, 0,
> > > > > AC_VERB_SET_POWER_STATE,
> > > > > power_state);
> > > > > + if (err < 0)
> > > > > + continue;
> > > > > snd_hda_codec_set_power_to_all(codec, fg, power_state,
> > > > > true);
> > > > > }
> > > > > --
> > > > > 1.7.9.5
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-26 10:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-24 6:53 [PATCH] ALSA: HDA - Check return value to reduce useless delay Wang Xingchao
2012-10-24 22:00 ` Takashi Iwai
2012-10-25 10:30 ` Wang, Xingchao
2012-10-25 11:08 ` Takashi Iwai
2012-10-26 10:45 ` Wang, Xingchao
2012-10-26 10:59 ` Takashi Iwai
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.