[alsa-devel] [PATCH] ALSA: hda - Fix Skylake codec timeouts

[Takashi Iwai]

On Wed, 15 Jul 2015 10:07:35 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-07-15 10:00, Takashi Iwai wrote:
> > On Wed, 15 Jul 2015 09:39:35 +0200,
> > David Henningsson wrote:
> >>
> >> When the controller is powered up but the HDMI codec is powered down
> >> on Skylake, the power well is turned off. When the codec is then
> >> powered up again, we need to poke the codec a little extra to make
> >> sure it wakes up. Otherwise we'll get sad "no response from codec"
> >> messages and broken audio.
> >>
> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> >> Tested-by: Woodrow Shen <woodrow.shen at canonical.com>
> >> ---
> >>
> >>   * It would good to have an ack from Intel on this patch, since they
> >>     have better hardware knowledge than I.
> >>
> >>   * Also I haven't really kept track of all recent reorganisation of the
> >>     hda driver so I'm not totally sure how many kernels back (if any)
> >>     that this applies to
> >>
> >>   sound/pci/hda/hda_intel.c | 10 +++++++++-
> >>   1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >> index ca151b4..872e9a7 100644
> >> --- a/sound/pci/hda/hda_intel.c
> >> +++ b/sound/pci/hda/hda_intel.c
> >> @@ -567,9 +567,17 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
> >>   /* Enable/disable i915 display power for the link */
> >>   static int azx_intel_link_power(struct azx *chip, bool enable)
> >>   {
> >> +	int err;
> >>   	struct hdac_bus *bus = azx_bus(chip);
> >>
> >> -	return snd_hdac_display_power(bus, enable);
> >> +	err = snd_hdac_display_power(bus, enable);
> >> +	if (err < 0)
> >> +		return err;
> >> +	if (enable && (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)) {
> >> +		snd_hdac_set_codec_wakeup(bus, true);
> >> +		snd_hdac_set_codec_wakeup(bus, false);
> >> +	}
> >
> > Wouldn't it be better to put in snd_hadc_display_power() itself?
> 
> I don't mind either way. Mengdong, do you have an opinion?

Oh, BTW, a clear difference is that my patch calls the wakeup only at
the first power up.  link_power() can be called multiple times, so
there might behave differently, and needs the check whether it really
works.


Takashi

> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -56,8 +56,11 @@ int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
> >   		enable ? "enable" : "disable");
> >
> >   	if (enable) {
> > -		if (!bus->i915_power_refcount++)
> > +		if (!bus->i915_power_refcount++) {
> >   			acomp->ops->get_power(acomp->dev);
> > +			snd_hdac_set_codec_wakeup(bus, true);
> > +			snd_hdac_set_codec_wakeup(bus, false);
> > +		}
> >   	} else {
> >   		WARN_ON(!bus->i915_power_refcount);
> >   		if (!--bus->i915_power_refcount)
> >
> > Takashi
> >
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
>