Skylake/Broadwell/Haswell: No HDMI audio jack detection in D3

This is a patch set fixing a long-standing bug for some Haswell, Broadwell and most Skylake machines.

When the HDA audio controller is in D3, a hotplug event (i e, HDMI or DisplayPort being plugged in) fails to wake up the audio side so it never registers that something has happened.

By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.

I've been trying to get this through upstream, but it has been slower than usual due to it touching both the i915 and HDA subsystems, and ideally I wanted it in there first, but I don't think OEM wants to miss another SRU cycle. So this is on its way in upstream, but not there yet. The patches here are based on v5 of my patch set, and v4 were both reviewed by Jani Nikula and Takashi Iwai.

Now, I've tested the upstream version, but I've just backported this to 3.19 without any testing. I could use Timo's assistance here so we coordinate testing this with other i915 changes for the next SRU cycle. And I could use Timo's help to make sure this applies equally to i915 and i915_bpo.

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1490895

David Henningsson (4):
  drm/i915: Add audio pin sense / ELD callback
  drm/i915: Call audio pin/ELD notify function
  ALSA: hda - allow codecs to access the i915 pin/ELD callback
  ALSA: hda - Wake the codec up on pin/ELD notify events

 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/intel_audio.c | 27 ++++++++++++++++++++++++---
 include/drm/i915_component.h | 16 ++++++++++++++++
 sound/pci/hda/hda_i915.c | 10 ++++++++++
 sound/pci/hda/hda_intel.h | 5 +++++
 sound/pci/hda/patch_hdmi.c | 21 ++++++++++++++++++++-
 6 files changed, 76 insertions(+), 4 deletions(-)

--
1.9.1

This callback will be called by the i915 driver to notify the hda
driver that its HDMI information needs to be refreshed, i e,
that audio output is now available (or unavailable) - usually as a
result of a monitor being plugged in (or unplugged).

Signed-off-by: David Henningsson <email address hidden>
---
 include/drm/i915_component.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..c0f4d45 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -34,6 +34,22 @@ struct i915_audio_component {
   void (*codec_wake_override)(struct device *, bool enable);
   int (*get_cdclk_freq)(struct device *);
  } *ops;
+
+ const struct i915_audio_component_audio_ops {
+ /**
+ * @audio_ptr:
+ *
+ * Pointer to pass when calling pin_eld_notify.
+ */
+ void *audio_ptr;
+ /**
+ * @pin_eld_notify:
+ *
+ * Called from i915 driver, notifying the HDA driver that
+ * pin sense and/or ELD information has changed.
+ */
+ void (*pin_eld_notify)(void *audio_ptr, int port);
+ } *audio_ops;
 };

 #endif /* _I915_COMPONENT_H_ */
--
1.9.1

When the audio codec is enabled or disabled, notify the audio driver.
This will enable the audio driver to get the notification at all times
(even when audio is in different powersave states).

Signed-off-by: David Henningsson <email address hidden>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/intel_audio.c | 27 ++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e437bb0..146ef60 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1700,6 +1700,7 @@ struct drm_i915_private {
  struct drm_property *force_audio_property;

  /* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
  bool audio_component_registered;

  uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 530f0d6..3348e60 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -404,6 +404,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
  struct drm_connector *connector;
  struct drm_device *dev = encoder->dev;
  struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_audio_component *acomp = dev_priv->audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port->port;

  connector = drm_select_eld(encoder, mode);
  if (!connector)
@@ -424,6 +427,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)

  if (dev_priv->display.audio_codec_enable)
   dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
+
+ if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+ acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }

 /**
@@ -433,13 +439,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
  * The disable sequences must be performed before disabling the transcoder or
  * port.
  */
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 {
- struct drm_device *dev = encoder->base.dev;
+ struct drm_encoder *encoder = &intel_encoder->base;
+ struct drm_device *dev = encoder->dev;
  struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_audio_component *acomp = dev_priv->audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port->port;

  if (dev_priv->display.audio_codec_disable)
- dev_priv->display.audio_codec_disable(encoder);
+ dev_priv->display.audio_codec_disable(intel_encoder);
+
+ if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+ acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }

 /**
@@ -529,12 +542,16 @@ static int i915_audio_component_bind(struct device *i915_dev,
          struct device *hda_dev, void *data)
 {
  struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);

  if (WARN_ON(acomp->ops || acomp->dev))
   return -EEXIST;

+ drm_mod...

This lets the interested codec be notified when an i915 pin/ELD
event happens.

Signed-off-by: David Henningsson <email address hidden>
---
 sound/pci/hda/hda_i915.c | 10 ++++++++++
 sound/pci/hda/hda_intel.h | 5 +++++
 2 files changed, 15 insertions(+)

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index aec8a0e..b6eadf1 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -108,6 +108,16 @@ void haswell_set_bclk(struct hda_intel *hda)
  azx_writew(&hda->chip, EM5, bclk_n);
 }

+int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *aops)
+{
+ if (WARN_ON(!hdac_acomp))
+ return -ENODEV;
+
+ hdac_acomp->audio_ops = aops;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
+
 static int hda_component_master_bind(struct device *dev)
 {
  struct snd_card *card = dev_get_drvdata(dev);
diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h
index 51cd664..51b940c 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -54,6 +54,7 @@ void haswell_set_bclk(struct hda_intel *hda);
 int hda_i915_init(struct hda_intel *hda);
 int hda_i915_init_bpo(struct hda_intel *hda);
 int hda_i915_exit(struct hda_intel *hda);
+int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *);
 #else
 static inline int hda_set_codec_wakeup(struct hda_intel *hda, bool enable)
 {
@@ -80,6 +81,10 @@ static inline int hda_i915_exit_bpo(void)
 {
        return 0;
 }
+static inline int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *)
+{
+ return -ENODEV;
+}
 #endif

 #endif
--
1.9.1

Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.

This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.

Right now, this might mean we get two callbacks from the same event,
one from the unsol event and one from the i915 driver, but this is
not harmful and can be optimised away in a later patch.

Signed-off-by: David Henningsson <email address hidden>
---
 sound/pci/hda/patch_hdmi.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index b422e40..8fe466b 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -40,6 +40,7 @@
 #include "hda_codec.h"
 #include "hda_local.h"
 #include "hda_jack.h"
+#include "hda_intel.h"

 static bool static_hdmi_pcm;
 module_param(static_hdmi_pcm, bool, 0644);
@@ -145,6 +146,9 @@ struct hdmi_spec {
   */
  struct hda_multi_out multiout;
  struct hda_pcm_stream pcm_playback;
+
+ /* i915/powerwell (Haswell+/Valleyview+) specific */
+ struct i915_audio_component_audio_ops i915_audio_ops;
 };

@@ -2201,6 +2205,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
  struct hdmi_spec *spec = codec->spec;
  int pin_idx;

+ if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ snd_hdac_i915_register_notifier(NULL);
+
  for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
   struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);

@@ -2327,6 +2334,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
  snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }

+static void intel_pin_eld_notify(void *audio_ptr, int port)
+{
+ struct hda_codec *codec = audio_ptr;
+ int pin_nid = port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
  struct hdmi_spec *spec;
@@ -2344,8 +2359,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
   intel_haswell_fixup_enable_dp12(codec);
  }

- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
   codec->depop_delay = 0;
+ spec->i915_audio_ops.audio_ptr = codec;
+ spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+ snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
+ }

  if (hdmi_parse_codec(codec) < 0) {
   codec->spec = NULL;
--
1.9.1

On Tue, Sep 01, 2015 at 12:04:05PM +0200, David Henningsson wrote:
> This is a patch set fixing a long-standing bug for some Haswell, Broadwell and most Skylake machines.
>
> When the HDA audio controller is in D3, a hotplug event (i e, HDMI or DisplayPort being plugged in) fails to wake up the audio side so it never registers that something has happened.
>
> By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
>
> I've been trying to get this through upstream, but it has been slower than usual due to it touching both the i915 and HDA subsystems, and ideally I wanted it in there first, but I don't think OEM wants to miss another SRU cycle. So this is on its way in upstream, but not there yet. The patches here are based on v5 of my patch set, and v4 were both reviewed by Jani Nikula and Takashi Iwai.
>
> Now, I've tested the upstream version, but I've just backported this to 3.19 without any testing. I could use Timo's assistance here so we coordinate testing this with other i915 changes for the next SRU cycle. And I could use Timo's help to make sure this applies equally to i915 and i915_bpo.
>
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1490895
>
> David Henningsson (4):
> drm/i915: Add audio pin sense / ELD callback
> drm/i915: Call audio pin/ELD notify function
> ALSA: hda - allow codecs to access the i915 pin/ELD callback
> ALSA: hda - Wake the codec up on pin/ELD notify events
>
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_audio.c | 27 ++++++++++++++++++++++++---
> include/drm/i915_component.h | 16 ++++++++++++++++
> sound/pci/hda/hda_i915.c | 10 ++++++++++
> sound/pci/hda/hda_intel.h | 5 +++++
> sound/pci/hda/patch_hdmi.c | 21 ++++++++++++++++++++-
> 6 files changed, 76 insertions(+), 4 deletions(-)
>
> --
> 1.9.1
>
>
> --
> kernel-team mailing list
> <email address hidden>
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

I want to see some positive test results in the bug before I'm going to
take this.

Brad
--
Brad Figg <email address hidden> http://www.canonical.com

On 2015-09-01 21:38, Brad Figg wrote:
> On Tue, Sep 01, 2015 at 12:04:05PM +0200, David Henningsson wrote:
>> This is a patch set fixing a long-standing bug for some Haswell, Broadwell and most Skylake machines.
>>
>> When the HDA audio controller is in D3, a hotplug event (i e, HDMI or DisplayPort being plugged in) fails to wake up the audio side so it never registers that something has happened.
>>
>> By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
>>
>> I've been trying to get this through upstream, but it has been slower than usual due to it touching both the i915 and HDA subsystems, and ideally I wanted it in there first, but I don't think OEM wants to miss another SRU cycle. So this is on its way in upstream, but not there yet. The patches here are based on v5 of my patch set, and v4 were both reviewed by Jani Nikula and Takashi Iwai.
>>
>> Now, I've tested the upstream version, but I've just backported this to 3.19 without any testing. I could use Timo's assistance here so we coordinate testing this with other i915 changes for the next SRU cycle. And I could use Timo's help to make sure this applies equally to i915 and i915_bpo.
>>
>> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1490895
> I want to see some positive test results in the bug before I'm going to
> take this.

Me too. :-)

Once Timo has a finished build (and I've provided him with a version
that actually compiles, *ahem*), could Anthony help assign people to
help us test it? It would be very nice if we could have this tested on
more than one of (Skylake / Broadwell / Haswell).

@Anthony, if you look at the duplicates for bug 1490895 (e g, 1383997) I
think you'll quickly understand what all of this is about.

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

Kernel to test can be found under http://koti.kapsi.fi/~tjaalton/skl/, debs with version 3.19.0-27.29+bpo2.2_amd64.deb.

According to the mailing list:

* We've got positive testing results from Jerry Kao (tested HSW and BDW)
   and myself [David Henningsson] (tested SKL)

* Upstream, Takashi Iwai has now merged the corresponding patch set into
   his for-next tree and it's been scheduled to reach 4.3-rc1.

On Skylake platform I encounter shutdown hang without HDMI plug issue
the system stop at "init: wait-for-state (plymouth-shutdown lightdm)"
Log attached

 > On Skylake platform I encounter shutdown hang without HDMI plug issue
 > the system stop at "init: wait-for-state (plymouth-shutdown lightdm)"
 > Log attached

Thanks for testing. Can you confirm that this is an actual regression with the patch set, i e, that it does not happen with the previous 3.19 kernel ( 3.19.0-28.30 )?

I'm not sure it's regression issue or not, but it's also happen on 3.19 kernel ( 3.19.0-27 ) "with" HDMI plug, if I unplug the HDMI and test it again then the system can be shutdown without hang.

so the different between original kernel and bpo 2.2 is : bpo 2.2 encounter shutdown hang "without" HDMI plug
and both encounter this issue "with" HDMI plug.

I tried the fail SKL system with KH2 image. The issue Ray discovered can be reproduced without diwic's patches. So it seems not a regression issue for this fix.

Before install patches, no matter HDMI plug or non-plug, the error in comment#12 could happen.
With HDMI plugging: failure rate 2/2
Without HDMI plugging: failure rate 2/3

The hang is not a regression, it happens if hdmi is left plugged in when rebooting/shutting down. It doesn't hang if hdmi is plugged in before booting up.

This is filed now upstream: https://bugs.freedesktop.org/show_bug.cgi?id=91905

Patch verification result is positive. Please check the detail result in http://bit.ly/1ECAeth

After install patches in comment #8, HDMI hot-plugging is detectable and functions works well.
test platforms: TUSL4-E3-C2(SKL), MASL5-E2-C1(SKL), HOU4-E3-C1B(BDW), Inspiron 3442 (HSW)
test kernel: 3.19.0-27
install patches:linux-headers-3.19.0-27-generic_3.19.0-27.29+bpo2.2_amd64.deb, linux-image-3.19.0-27-generic_3.19.0-27.29+bpo2.2_amd64.deb, linux-image-3.19.0-27-generic_3.19.0-27.29+bpo2.2_amd64.deb

One additional information for comment#17

When verified fix patch, audio icon disappeared once after reboot system with HDMI connected. Please refer attached screenshot and log. Can not recover this issue by reboot/power off system.

After recover the system and check the clean system without patch, can not reproduce this issue anymore.

Without patch
reboot for 5 times: can not reproduce
power off for times: can not reproduce

With patch
reboot for 5 times: can not reproduce
power off for times: can not reproduce

Conclusion
There is no evidence that the audio icon disappear issue is a regression of this fix patch.

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-vivid' to 'verification-done-vivid'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

Test on TUSL4-E3-C3 (SKL) is done, the proposed kernel shows positive result.

From a private bug:

I have verified kernel 3.19.0-29.31~14.04.1 on <Skylake SKU>, this issue is fixed. The HDMI audio device is available and works fine.

Verified Passed with KH 0914-0 Build ( 3.19.0-29.31~14.04.1 ) on Haswell & Broadwell SKU.

- Haswell:
SKU: RDU-E4-C1
HDMI audio passed in both AC & Battery mode

- Broadwell:
SKU: HOU4-E3-C1B
HDMI audio passed in both AC & Battery mode

This bug was fixed in the package linux - 3.19.0-30.33

---------------
linux (3.19.0-30.33) vivid; urgency=low

  [ Brad Figg ]

  * Release Tracking Bug
    - LP: #1498065
  * Revert "[Config]
    MFD_INTEL_LPSS/MFD_INTEL_LPSS_ACPI/MFD_INTEL_LPSS_PCI=m"
    - LP: #1498137
  * [Config] Disable the MFD_INTEL_LPSS* driver

linux (3.19.0-30.32) vivid; urgency=low

  [ Brad Figg ]

  * Release Tracking Bug
    - LP: #1498065

  [ Upstream Kernel Changes ]

  * net: Fix skb_set_peeked use-after-free bug
    - LP: #1497184

linux (3.19.0-29.31) vivid; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1493902

  [ Ander Conselvan de Oliveira ]

  * SAUCE: i915_bpo: Set ddi_pll_sel in DP MST path
    - LP: #1483320

  [ Chris J Arges ]

  * [Config] DEFAULT_IOSCHED="deadline" for ppc64el
    - LP: #1469829

  [ Chris Wilson ]

  * SAUCE: i915_bpo: drm/i915: Flag the execlists context object as dirty
    after every use
    - LP: #1489501

  [ Daniel Vetter ]

  * SAUCE: i915_bpo: drm/i915: Only dither on 6bpc panels
    - LP: #1489501

  [ David Henningsson ]

  * SAUCE: drm/i915: Add audio pin sense / ELD callback
    - LP: #1490895
  * SAUCE: drm/i915: Call audio pin/ELD notify function
    - LP: #1490895
  * SAUCE: ubuntu/i915: Call audio pin/ELD notify function
    - LP: #1490895
  * SAUCE: ALSA: hda - Add "hdac_acomp" global variable
    - LP: #1490895
  * SAUCE: ALSA: hda - allow codecs to access the i915 pin/ELD callback
    - LP: #1490895
  * SAUCE: ALSA: hda - Wake the codec up on pin/ELD notify events
    - LP: #1490895

  [ Jani Nikula ]

  * SAUCE: i915_bpo: Revert "drm/i915: Allow parsing of variable size child
    device entries from VBT"
    - LP: #1489501

  [ Maarten Lankhorst ]

  * SAUCE: i915_bpo: drm/i915: calculate primary visibility changes instead
    of calling from set_config
    - LP: #1489501
  * SAUCE: i915_bpo: drm/i915: Commit planes on each crtc separately.
    - LP: #1489501

  [ Thulasimani,Sivakumar ]

  * SAUCE: i915_bpo: Revert "drm/i915: Add eDP intermediate frequencies for
    CHV"
    - LP: #1489501
  * SAUCE: i915_bpo: drm/i915: remove HBR2 from chv supported list
    - LP: #1489501
  * SAUCE: i915_bpo: drm/i915: Avoid TP3 on CHV
    - LP: #1489501

  [ Timo Aaltonen ]

  * Revert "SAUCE: i915_bpo: drm/i915: Allow parsing of variable size child
    device entries from VBT, addendum v2"
    - LP: #1489501
  * SAUCE: Migrate Broadwell to i915_bpo.
    - LP: #1483320

  [ Upstream Kernel Changes ]

  * tcp: fix recv with flags MSG_WAITALL | MSG_PEEK
    - LP: #1486146
  * powerpc/powernv: Fix the overflow of OPAL message notifiers head array
    - LP: #1487085
  * xhci: call BIOS workaround to enable runtime suspend on Intel Braswell
    - LP: #1489292
  * PM / QoS: Make it possible to expose device latency tolerance to
    userspace
    - LP: #1488395
  * ACPI / PM: Attach ACPI power domain only once
    - LP: #1488395
  * Driver core: wakeup the parent device before trying probe
    - LP: #1488395
  * klist: implement klist_prev()
    - LP: #1488395
  * driver core: implement device_for_each_child_reverse()
    - LP: #1488395
  * mfd: make mfd_remove_devices() iterate in reverse order
    ...