| Message ID | s5hegk94zq0.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >
--- 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)