* [PATCH] ALSA: hda - Fix Skylake codec timeouts
@ 2015-07-15 7:39 David Henningsson
2015-07-15 8:00 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: David Henningsson @ 2015-07-15 7:39 UTC (permalink / raw)
To: tiwai, alsa-devel; +Cc: hui.wang, mengdong.lin, David Henningsson
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);
+ }
+ return 0;
}
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Fix Skylake codec timeouts
2015-07-15 7:39 [PATCH] ALSA: hda - Fix Skylake codec timeouts David Henningsson
@ 2015-07-15 8:00 ` Takashi Iwai
2015-07-15 8:07 ` David Henningsson
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Iwai @ 2015-07-15 8:00 UTC (permalink / raw)
To: David Henningsson; +Cc: hui.wang, mengdong.lin, alsa-devel
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?
--- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Fix Skylake codec timeouts
2015-07-15 8:00 ` Takashi Iwai
@ 2015-07-15 8:07 ` David Henningsson
2015-07-15 8:17 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: David Henningsson @ 2015-07-15 8:07 UTC (permalink / raw)
To: Takashi Iwai; +Cc: hui.wang, mengdong.lin, alsa-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Fix Skylake codec timeouts
2015-07-15 8:07 ` David Henningsson
@ 2015-07-15 8:17 ` Takashi Iwai
0 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2015-07-15 8:17 UTC (permalink / raw)
To: David Henningsson; +Cc: hui.wang, mengdong.lin, alsa-devel
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-15 8:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-15 7:39 [PATCH] ALSA: hda - Fix Skylake codec timeouts David Henningsson
2015-07-15 8:00 ` Takashi Iwai
2015-07-15 8:07 ` David Henningsson
2015-07-15 8:17 ` 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.