ALSA: hda - Fix Skylake codec timeouts
diff mbox

Message ID s5hegk94zq0.wl-tiwai@suse.de
State New
Headers show

Commit Message

Takashi Iwai July 15, 2015, 8 a.m. UTC
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@canonical.com>
> Tested-by: Woodrow Shen <woodrow.shen@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? 


Takashi

Comments

David Henningsson July 15, 2015, 8:07 a.m. UTC | #1
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@canonical.com>
>> Tested-by: Woodrow Shen <woodrow.shen@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?

>
> --- 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
>
Takashi Iwai July 15, 2015, 8:17 a.m. UTC | #2
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@canonical.com>
> >> Tested-by: Woodrow Shen <woodrow.shen@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
>

Patch
diff mbox

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