Re: [PATCH] ALSA: hda - Fix Skylake codec timeouts

[David Henningsson <david.henningsson@canonical.com>]

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
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic