Re: [PATCH] ALSA: HDA - Check return value to reduce useless delay

[Takashi Iwai <tiwai@suse.de>]

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