[alsa-devel] [PATCH 0/4 v5] i915 to call hda driver on HDMI plug/unplug
Hopefully last version? :-)
* Added commit text about duplicate events (patch 4/4) * Added locks in bind/unbind on i915 side (patch 2/4) * Fixed docbook comments in i915 struct (patch 1/4) * Removed port_mst_streaming parameter (patch 1/4) * Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it?
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 ++++++++++++++++ include/sound/hda_i915.h | 7 +++++++ sound/hda/hdac_i915.c | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 6 files changed, 79 insertions(+), 4 deletions(-)
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).
Reviewed-by: Jani Nikula jani.nikula@intel.com Signed-off-by: David Henningsson david.henningsson@canonical.com --- 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_ */
On Fri, Aug 28, 2015 at 07:02:27PM +0200, David Henningsson wrote:
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).
Reviewed-by: Jani Nikula jani.nikula@intel.com Signed-off-by: David Henningsson david.henningsson@canonical.com
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);
Kerneldoc looks good now, but it won't be of much use with an include directive into the drm.tmpl docbook. Can you please coordinate with Libin on that?
Thanks, Daniel
- } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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).
Reviewed-by: Jani Nikula jani.nikula@intel.com Signed-off-by: David Henningsson david.henningsson@canonical.com --- 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 fd1de45..1fc327d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1809,6 +1809,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 3da9b84..678a34f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -399,6 +399,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) @@ -419,6 +422,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); }
/** @@ -428,13 +434,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); }
/** @@ -525,12 +538,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_modeset_lock_all(dev_priv->dev); acomp->ops = &i915_audio_component_ops; acomp->dev = i915_dev; + dev_priv->audio_component = acomp; + drm_modeset_unlock_all(dev_priv->dev);
return 0; } @@ -539,9 +556,13 @@ static void i915_audio_component_unbind(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);
+ drm_modeset_lock_all(dev_priv->dev); acomp->ops = NULL; acomp->dev = NULL; + dev_priv->audio_component = NULL; + drm_modeset_unlock_all(dev_priv->dev); }
static const struct component_ops i915_audio_component_bind_ops = {
This lets the interested codec be notified when an i915 pin/ELD event happens.
Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: David Henningsson david.henningsson@canonical.com --- include/sound/hda_i915.h | 7 +++++++ sound/hda/hdac_i915.c | 10 ++++++++++ 2 files changed, 17 insertions(+)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index adb5ba5..a0a1d67 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -4,12 +4,15 @@ #ifndef __SOUND_HDA_I915_H #define __SOUND_HDA_I915_H
+#include <drm/i915_component.h> + #ifdef CONFIG_SND_HDA_I915 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable); int snd_hdac_display_power(struct hdac_bus *bus, bool enable); int snd_hdac_get_display_clk(struct hdac_bus *bus); int snd_hdac_i915_init(struct hdac_bus *bus); int snd_hdac_i915_exit(struct hdac_bus *bus); +int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *); #else static int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { @@ -31,6 +34,10 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus) { return 0; } +static inline int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *) +{ + return -ENODEV; +} #endif
#endif /* __SOUND_HDA_I915_H */ diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 5676b84..55c3df4 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -134,6 +134,16 @@ static int hdac_component_master_match(struct device *dev, void *data) return !strcmp(dev->driver->name, "i915"); }
+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); + int snd_hdac_i915_init(struct hdac_bus *bus) { struct component_match *match = NULL;
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.
Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: David Henningsson david.henningsson@canonical.com --- sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..acbfbe0 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,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; };
@@ -2191,6 +2196,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);
@@ -2316,6 +2324,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; @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- 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;
Recently I always see the following error message during S4 or S3 resume with drm-intel-nightly. [ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 97.804297] PM: Marking nosave pages: [mem 0x00000000-0x00000fff] [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] [ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated 321557 pages) [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s) [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 98.151998] Suspending console(s) (use no_console_suspend to debug) [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 [ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 [ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 [ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt is disabled at this moment, how could hdmi_present_sense() get the response from codec ?
thanks
-----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson Sent: Saturday, August 29, 2015 1:03 AM To: tiwai@suse.de; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Yang, Libin; Vetter, Daniel Cc: David Henningsson Subject: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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.
Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: David Henningsson david.henningsson@canonical.com
sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..acbfbe0 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,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;
};
@@ -2191,6 +2196,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);
@@ -2316,6 +2324,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; @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- 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
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote:
Recently I always see the following error message during S4 or S3 resume with drm-intel-nightly. [ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 97.804297] PM: Marking nosave pages: [mem 0x00000000-0x00000fff] [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] [ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated 321557 pages) [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s) [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 98.151998] Suspending console(s) (use no_console_suspend to debug) [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 [ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 [ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 [ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt is disabled at this moment, how could hdmi_present_sense() get the response from codec ?
Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM.
Could you check whether it works?
thanks,
Takashi
--- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,9 @@ 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); + /* don't call notifier during PM; will be checked in resume callback */ + if (!atomic_read(&codec->core.in_pm)) + check_presence_and_report(codec, pin_nid); }
static int patch_generic_hdmi(struct hda_codec *codec)
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote:
Recently I always see the following error message during S4 or S3 resume
with drm-intel-nightly.
[ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
done.
[ 97.804297] PM: Marking nosave pages: [mem 0x00000000-0x00000fff] [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] [ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated 321557
pages)
[ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
MB/s)
[ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 98.151998] Suspending console(s) (use no_console_suspend to debug) [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2:
HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0,
force 128
[ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to polling mode: last cmd=0x206f2f00
[ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec,
disabling MSI: last cmd=0x206f2f00
[ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to single_cmd mode: last cmd=0x206f2f00
[ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of
snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt is disabled at this moment, how could hdmi_present_sense() get the response from codec ?
Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM.
Could you check whether it works?
thanks,
Takashi
Strange, your patch couldn't get away the error message.
thanks
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,9 @@ 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);
- /* don't call notifier during PM; will be checked in resume callback */
- if (!atomic_read(&codec->core.in_pm))
check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
On Wed, 25 Nov 2015 11:57:13 +0100, Zhang, Xiong Y wrote:
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote:
Recently I always see the following error message during S4 or S3 resume
with drm-intel-nightly.
[ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
done.
[ 97.804297] PM: Marking nosave pages: [mem 0x00000000-0x00000fff] [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] [ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated 321557
pages)
[ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
MB/s)
[ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 98.151998] Suspending console(s) (use no_console_suspend to debug) [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2:
HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last
cmd=0x206f2e08
[ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0,
force 128
[ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to polling mode: last cmd=0x206f2f00
[ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec,
disabling MSI: last cmd=0x206f2f00
[ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to single_cmd mode: last cmd=0x206f2f00
[ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of
snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt is disabled at this moment, how could hdmi_present_sense() get the response from codec ?
Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM.
Could you check whether it works?
thanks,
Takashi
Strange, your patch couldn't get away the error message.
OK, then it's not about the race *during* the hd-audio driver resuming or such. I guess it's the other way round: the i915 driver notifies it unnecessarily while it's getting off. The audio part of HDMI controller relies on the i915 power state, so it's screwed up.
Check with drm.debug option to see in which code path it's triggered. If it's a call in i915 suspend, the call should be removed not to wake up the audio part unnecessarily.
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Takashi
On Wed, 25 Nov 2015 11:57:13 +0100, Zhang, Xiong Y wrote:
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote:
Recently I always see the following error message during S4 or S3 resume
with drm-intel-nightly.
[ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
done.
[ 97.804297] PM: Marking nosave pages: [mem
0x00000000-0x00000fff]
[ 97.804302] PM: Marking nosave pages: [mem
0x00058000-0x00058fff]
[ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem
0x84c11000-0x84c12fff]
[ 97.804312] PM: Marking nosave pages: [mem
0x876fc000-0x87746fff]
[ 97.804317] PM: Marking nosave pages: [mem
0x8785e000-0x87fe9fff]
[ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated
321557
pages)
[ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
MB/s)
[ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001
seconds)
done.
[ 98.151998] Suspending console(s) (use no_console_suspend to
debug)
[ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi
hdaudioC0D2:
HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size
is 0,
force 128
[ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to polling mode: last cmd=0x206f2f00
[ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec,
disabling MSI: last cmd=0x206f2f00
[ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to single_cmd mode: last cmd=0x206f2f00
[ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of
snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt
is
disabled at this moment, how could hdmi_present_sense() get the
response
from codec ?
Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM.
Could you check whether it works?
thanks,
Takashi
Strange, your patch couldn't get away the error message.
OK, then it's not about the race *during* the hd-audio driver resuming or such. I guess it's the other way round: the i915 driver notifies it unnecessarily while it's getting off. The audio part of HDMI controller relies on the i915 power state, so it's screwed up.
Check with drm.debug option to see in which code path it's triggered. If it's a call in i915 suspend, the call should be removed not to wake up the audio part unnecessarily.
[Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 suspend. --- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,9 @@ 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); + /* don't call notifier during PM; will be checked in resume callback */ + if (atomic_read(&codec->core.in_pm)) + check_presence_and_report(codec, pin_nid); }
static int patch_generic_hdmi(struct hda_codec *codec)
This could remove the error message. Go through the audio driver, I found in_pm isn't zero only during call codec_suspend() and codec_resume(). atomic_inc_not_zero(&codec->in_pm) in snd_hdac_power_up_pm() isn't inc in_pm since in_pm is zero. Is this right ?
thanks
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Takashi
On Thu, 26 Nov 2015 07:06:56 +0100, Zhang, Xiong Y wrote:
On Wed, 25 Nov 2015 11:57:13 +0100, Zhang, Xiong Y wrote:
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote:
Recently I always see the following error message during S4 or S3 resume
with drm-intel-nightly.
[ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
done.
[ 97.804297] PM: Marking nosave pages: [mem
0x00000000-0x00000fff]
[ 97.804302] PM: Marking nosave pages: [mem
0x00058000-0x00058fff]
[ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000fffff] [ 97.804310] PM: Marking nosave pages: [mem
0x84c11000-0x84c12fff]
[ 97.804312] PM: Marking nosave pages: [mem
0x876fc000-0x87746fff]
[ 97.804317] PM: Marking nosave pages: [mem
0x8785e000-0x87fe9fff]
[ 97.804387] PM: Marking nosave pages: [mem 0x88000000-0xffffffff] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated
321557
pages)
[ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
MB/s)
[ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001
seconds)
done.
[ 98.151998] Suspending console(s) (use no_console_suspend to
debug)
[ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi
hdaudioC0D2:
HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 99.178150] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178151] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178152] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178153] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178154] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178155] snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2,
last
cmd=0x206f2e08
[ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size
is 0,
force 128
[ 101.189709] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to polling mode: last cmd=0x206f2f00
[ 102.195492] snd_hda_intel 0000:00:1f.3: No response from codec,
disabling MSI: last cmd=0x206f2f00
[ 103.201275] snd_hda_intel 0000:00:1f.3: azx_get_response timeout,
switching to single_cmd mode: last cmd=0x206f2f00
[ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed
The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of
snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt
is
disabled at this moment, how could hdmi_present_sense() get the
response
from codec ?
Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM.
Could you check whether it works?
thanks,
Takashi
Strange, your patch couldn't get away the error message.
OK, then it's not about the race *during* the hd-audio driver resuming or such. I guess it's the other way round: the i915 driver notifies it unnecessarily while it's getting off. The audio part of HDMI controller relies on the i915 power state, so it's screwed up.
Check with drm.debug option to see in which code path it's triggered. If it's a call in i915 suspend, the call should be removed not to wake up the audio part unnecessarily.
[Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 suspend.
So the call of eld_notifier should be suppressed at suspend.
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,9 @@ 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);
- /* don't call notifier during PM; will be checked in resume callback */
- if (atomic_read(&codec->core.in_pm))
check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
This could remove the error message. Go through the audio driver, I found in_pm isn't zero only during call codec_suspend() and codec_resume(). atomic_inc_not_zero(&codec->in_pm) in snd_hdac_power_up_pm() isn't inc in_pm since in_pm is zero. Is this right ?
It's the flag that is set only during the sound driver's PM, not i915 driver's PM. I gave the patch from the wrong assumption. It's not the race between i915 and audio PM, thus the patch doesn't help.
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
Takashi
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
Takashi
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
--- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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); + if (!atomic_read(&codec->core.in_pm) && + !pm_runtime_suspended(hda_codec_dev(codec))) + check_presence_and_report(codec, pin_nid); }
static int patch_generic_hdmi(struct hda_codec *codec)
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
- if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
thanks
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
- if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
Takashi
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
> BTW, I have a patchset to avoid the audio h/w wakeup by a new > component ops to get ELD and connection states. It was posted to > alsa-devel shortly ago just as a reference, but this should work well > in such a case, too. The test patches are found in test/hdmi-jack > branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >> component ops to get ELD and connection states. It was posted to >> alsa-devel shortly ago just as a reference, but this should work well >> in such a case, too. The test patches are found in test/hdmi-jack >> branch of my sound git tree.
Did you try this branch (merge onto intel-test-nightly)?
[Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code.
Takashi
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>> component ops to get ELD and connection states. It was posted to >>> alsa-devel shortly ago just as a reference, but this should work well >>> in such a case, too. The test patches are found in test/hdmi-jack >>> branch of my sound git tree. > > Did you try this branch (merge onto intel-test-nightly)? > [Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code.
Takashi
On to, 2015-11-26 at 16:29 +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote: > > > > > BTW, I have a patchset to avoid the audio h/w > > > > wakeup by a new > > > > component ops to get ELD and connection states. It > > > > was posted to > > > > alsa-devel shortly ago just as a reference, but > > > > this should work well > > > > in such a case, too. The test patches are found in > > > > test/hdmi-jack > > > > branch of my sound git tree. > > > > Did you try this branch (merge onto intel-test- > > nightly)? > > > [Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
- if (!atomic_read(&codec->core.in_pm) &&
- !pm_runtime_suspended(hda_codec_dev(codec)))
check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
As I understand this happens during the i915 system suspend/resume hooks are running. There is no reason why the getting a power domain reference would fail there. In fact we are keeping all power wells for the whole duration of these callbacks, see the call to intel_display_set_init_power(true) in i915_drm_suspend() and i915_drm_resume_early()->intel_power_domains_init_hw(). So not sure how the audio power well could be off at that time.
So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code.
Takashi
On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote: > >>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>>> component ops to get ELD and connection states. It was posted to >>>> alsa-devel shortly ago just as a reference, but this should work well >>>> in such a case, too. The test patches are found in test/hdmi-jack >>>> branch of my sound git tree. >> >> Did you try this branch (merge onto intel-test-nightly)? >> > [Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
I don't understand why you need to do anything special. When the eld notify happens during suspend, the hardware is still fully powered up, so it should just work(tm) as long as the eld_notify is a synchronous call and it drops the power reference at the end.
I guess for any get_power after i915 has suspended we'd need to just reject the get_power call. Or does something force hda to suspend before and resume after i915 always?
On Thu, 26 Nov 2015 16:43:23 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
> On Thu, 26 Nov 2015 08:57:30 +0100, > Zhang, Xiong Y wrote: >> >>>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>>>> component ops to get ELD and connection states. It was posted to >>>>> alsa-devel shortly ago just as a reference, but this should work well >>>>> in such a case, too. The test patches are found in test/hdmi-jack >>>>> branch of my sound git tree. >>> >>> Did you try this branch (merge onto intel-test-nightly)? >>> >> [Zhang, Xiong Y] Yes, this branch could fix my issue. > > OK, good to hear. But this isn't for 4.4 or backport, as it's more > radical changes. > > Could you check the patch below instead? This suppresses the notifier > handling during suspend. It's bad, but the hotplug should be still > handled via normal unsol event, so it should keep working, good enough > as a stop gap. > > > Takashi > > --- > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index bdb6f226d006..f7c70e2ae65c 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -33,6 +33,7 @@ > #include <linux/delay.h> > #include <linux/slab.h> > #include <linux/module.h> > +#include <linux/pm_runtime.h> > #include <sound/core.h> > #include <sound/jack.h> > #include <sound/asoundef.h> > @@ -2352,7 +2353,9 @@ 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); > + if (!atomic_read(&codec->core.in_pm) && > + !pm_runtime_suspended(hda_codec_dev(codec))) > + check_presence_and_report(codec, pin_nid); > } > > static int patch_generic_hdmi(struct hda_codec *codec) [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
I don't understand why you need to do anything special. When the eld notify happens during suspend, the hardware is still fully powered up, so it should just work(tm) as long as the eld_notify is a synchronous call and it drops the power reference at the end.
Hm, that's the question. It's never clear so far as we haven't got any detailed logs.
The symptom implies that the graphics side is off while HDA tries to execute some verbs. So the power well is the first suspect. We reall need to track down the code path triggering the issue.
I guess for any get_power after i915 has suspended we'd need to just reject the get_power call. Or does something force hda to suspend before and resume after i915 always?
The HDA code itself calls get_power at the beginning of the resume. But not sure whether this suffices for the execution ordering.
Takashi
On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:43:23 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote: > > >> On Thu, 26 Nov 2015 08:57:30 +0100, >> Zhang, Xiong Y wrote: >>> >>>>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>>>>> component ops to get ELD and connection states. It was posted to >>>>>> alsa-devel shortly ago just as a reference, but this should work well >>>>>> in such a case, too. The test patches are found in test/hdmi-jack >>>>>> branch of my sound git tree. >>>> >>>> Did you try this branch (merge onto intel-test-nightly)? >>>> >>> [Zhang, Xiong Y] Yes, this branch could fix my issue. >> >> OK, good to hear. But this isn't for 4.4 or backport, as it's more >> radical changes. >> >> Could you check the patch below instead? This suppresses the notifier >> handling during suspend. It's bad, but the hotplug should be still >> handled via normal unsol event, so it should keep working, good enough >> as a stop gap. >> >> >> Takashi >> >> --- >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >> index bdb6f226d006..f7c70e2ae65c 100644 >> --- a/sound/pci/hda/patch_hdmi.c >> +++ b/sound/pci/hda/patch_hdmi.c >> @@ -33,6 +33,7 @@ >> #include <linux/delay.h> >> #include <linux/slab.h> >> #include <linux/module.h> >> +#include <linux/pm_runtime.h> >> #include <sound/core.h> >> #include <sound/jack.h> >> #include <sound/asoundef.h> >> @@ -2352,7 +2353,9 @@ 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); >> + if (!atomic_read(&codec->core.in_pm) && >> + !pm_runtime_suspended(hda_codec_dev(codec))) >> + check_presence_and_report(codec, pin_nid); >> } >> >> static int patch_generic_hdmi(struct hda_codec *codec) > [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
I don't understand why you need to do anything special. When the eld notify happens during suspend, the hardware is still fully powered up, so it should just work(tm) as long as the eld_notify is a synchronous call and it drops the power reference at the end.
Hm, that's the question. It's never clear so far as we haven't got any detailed logs.
The symptom implies that the graphics side is off while HDA tries to execute some verbs. So the power well is the first suspect. We reall need to track down the code path triggering the issue.
I guess for any get_power after i915 has suspended we'd need to just reject the get_power call. Or does something force hda to suspend before and resume after i915 always?
The HDA code itself calls get_power at the beginning of the resume. But not sure whether this suffices for the execution ordering.
Just chatted with Imre a bit, and he clarified the suspend order thing:
So what happens is that the normal suspend hooks for hda and i915 can in theory happen in any order. But i915 reamains powered on until the late suspend hook.
So we can get 1. i915 suspend 2. hda suspend 3. i915 late suspend
or 1. hda suspend 2. i915 suspend 3. i915 late suspend
So in this latter case i915 can call the eld notify hook after hda has already suspended. So that at least you would need to handle in your side.
On Thu, 26 Nov 2015 16:58:09 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:43:23 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote: > On Thu, 26 Nov 2015 10:16:17 +0100, > Zhang, Xiong Y wrote: >> >> >>> On Thu, 26 Nov 2015 08:57:30 +0100, >>> Zhang, Xiong Y wrote: >>>> >>>>>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>>>>>> component ops to get ELD and connection states. It was posted to >>>>>>> alsa-devel shortly ago just as a reference, but this should work well >>>>>>> in such a case, too. The test patches are found in test/hdmi-jack >>>>>>> branch of my sound git tree. >>>>> >>>>> Did you try this branch (merge onto intel-test-nightly)? >>>>> >>>> [Zhang, Xiong Y] Yes, this branch could fix my issue. >>> >>> OK, good to hear. But this isn't for 4.4 or backport, as it's more >>> radical changes. >>> >>> Could you check the patch below instead? This suppresses the notifier >>> handling during suspend. It's bad, but the hotplug should be still >>> handled via normal unsol event, so it should keep working, good enough >>> as a stop gap. >>> >>> >>> Takashi >>> >>> --- >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >>> index bdb6f226d006..f7c70e2ae65c 100644 >>> --- a/sound/pci/hda/patch_hdmi.c >>> +++ b/sound/pci/hda/patch_hdmi.c >>> @@ -33,6 +33,7 @@ >>> #include <linux/delay.h> >>> #include <linux/slab.h> >>> #include <linux/module.h> >>> +#include <linux/pm_runtime.h> >>> #include <sound/core.h> >>> #include <sound/jack.h> >>> #include <sound/asoundef.h> >>> @@ -2352,7 +2353,9 @@ 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); >>> + if (!atomic_read(&codec->core.in_pm) && >>> + !pm_runtime_suspended(hda_codec_dev(codec))) >>> + check_presence_and_report(codec, pin_nid); >>> } >>> >>> static int patch_generic_hdmi(struct hda_codec *codec) >> [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend(). > > OK, then the problem isn't about the HD-audio driver but rather i915 > driver. As already mentioned, i915 driver shouldn't issue eld_notify > callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
I don't understand why you need to do anything special. When the eld notify happens during suspend, the hardware is still fully powered up, so it should just work(tm) as long as the eld_notify is a synchronous call and it drops the power reference at the end.
Hm, that's the question. It's never clear so far as we haven't got any detailed logs.
The symptom implies that the graphics side is off while HDA tries to execute some verbs. So the power well is the first suspect. We reall need to track down the code path triggering the issue.
I guess for any get_power after i915 has suspended we'd need to just reject the get_power call. Or does something force hda to suspend before and resume after i915 always?
The HDA code itself calls get_power at the beginning of the resume. But not sure whether this suffices for the execution ordering.
Just chatted with Imre a bit, and he clarified the suspend order thing:
So what happens is that the normal suspend hooks for hda and i915 can in theory happen in any order. But i915 reamains powered on until the late suspend hook.
So we can get
- i915 suspend
- hda suspend
- i915 late suspend
or
- hda suspend
- i915 suspend
- i915 late suspend
So in this latter case i915 can call the eld notify hook after hda has already suspended. So that at least you would need to handle in your side.
OK, for the latter case, we may test a band-aid patch like below. This will skip the notifier certainly while being suspended. Xiong, could you check it?
Takashi
--- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
+ /* skip notification during suspend */ + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) + return; + check_presence_and_report(codec, pin_nid); }
On Thu, 26 Nov 2015 16:58:09 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:43:23 +0100, Ville Syrjälä wrote:
On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote: > > > > On 2015-11-26 10:24, Takashi Iwai wrote: >> On Thu, 26 Nov 2015 10:16:17 +0100, >> Zhang, Xiong Y wrote: >>> >>> >>>> On Thu, 26 Nov 2015 08:57:30 +0100, >>>> Zhang, Xiong Y wrote: >>>>> >>>>>>>> BTW, I have a patchset to avoid the audio h/w wakeup by a
new
>>>>>>>> component ops to get ELD and connection states. It was
posted to
>>>>>>>> alsa-devel shortly ago just as a reference, but this should
work well
>>>>>>>> in such a case, too. The test patches are found in
test/hdmi-jack
>>>>>>>> branch of my sound git tree. >>>>>> >>>>>> Did you try this branch (merge onto intel-test-nightly)? >>>>>> >>>>> [Zhang, Xiong Y] Yes, this branch could fix my issue. >>>> >>>> OK, good to hear. But this isn't for 4.4 or backport, as it's more >>>> radical changes. >>>> >>>> Could you check the patch below instead? This suppresses the
notifier
>>>> handling during suspend. It's bad, but the hotplug should be still >>>> handled via normal unsol event, so it should keep working, good
enough
>>>> as a stop gap. >>>> >>>> >>>> Takashi >>>> >>>> --- >>>> diff --git a/sound/pci/hda/patch_hdmi.c
b/sound/pci/hda/patch_hdmi.c
>>>> index bdb6f226d006..f7c70e2ae65c 100644 >>>> --- a/sound/pci/hda/patch_hdmi.c >>>> +++ b/sound/pci/hda/patch_hdmi.c >>>> @@ -33,6 +33,7 @@ >>>> #include <linux/delay.h> >>>> #include <linux/slab.h> >>>> #include <linux/module.h> >>>> +#include <linux/pm_runtime.h> >>>> #include <sound/core.h> >>>> #include <sound/jack.h> >>>> #include <sound/asoundef.h> >>>> @@ -2352,7 +2353,9 @@ 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); >>>> + if (!atomic_read(&codec->core.in_pm) && >>>> + !pm_runtime_suspended(hda_codec_dev(codec))) >>>> + check_presence_and_report(codec, pin_nid); >>>> } >>>> >>>> static int patch_generic_hdmi(struct hda_codec *codec) >>> [Zhang, Xiong Y] this patch couldn't remove the error message.
So audio controller isn't in Runtime D3 after azx_suspend().
>> >> OK, then the problem isn't about the HD-audio driver but rather
i915
>> driver. As already mentioned, i915 driver shouldn't issue
eld_notify
>> callback in the suspend code path. > > We don't have a complete drm log so I'm only speculating here; but
the
> HDA log seems to indicate the power well is off when we try to talk
to
> the HDA controller. That means either the i915 shut it down while we > were holding a lock on it, or HDA tried to lock it and that didn't make > it through; in which case the HDA side should handle an error from > get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
I don't understand why you need to do anything special. When the eld notify happens during suspend, the hardware is still fully powered up, so it should just work(tm) as long as the eld_notify is a synchronous call and it drops the power reference at the end.
Hm, that's the question. It's never clear so far as we haven't got any detailed logs.
The symptom implies that the graphics side is off while HDA tries to execute some verbs. So the power well is the first suspect. We reall need to track down the code path triggering the issue.
I guess for any get_power after i915 has suspended we'd need to just reject the get_power call. Or does something force hda to suspend before and resume after i915 always?
The HDA code itself calls get_power at the beginning of the resume. But not sure whether this suffices for the execution ordering.
Just chatted with Imre a bit, and he clarified the suspend order thing:
So what happens is that the normal suspend hooks for hda and i915 can in theory happen in any order. But i915 reamains powered on until the late suspend hook.
So we can get
- i915 suspend
- hda suspend
- i915 late suspend
or
- hda suspend
- i915 suspend
- i915 late suspend
So in this latter case i915 can call the eld notify hook after hda has already suspended. So that at least you would need to handle in your side.
OK, for the latter case, we may test a band-aid patch like below. This will skip the notifier certainly while being suspended. Xiong, could you check it?
[Zhang, Xiong Y] see the attachment file, it is s4 log. And it is latter case.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during suspend */
- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid);
}
[Zhang, Xiong Y] yes, this patch could remove the error message.
On Fri, 27 Nov 2015 03:55:28 +0100, Zhang, Xiong Y wrote:
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during suspend */
- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid);
}
[Zhang, Xiong Y] yes, this patch could remove the error message.
Alright, then it's indeed a failure in the sound driver side. Below is the official patch I'm going to merge.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend
The recent addition of ELD notifier for Intel HDMI/DP codec may lead the bad codec connection found as kernel messages like below: Suspending console(s) (use no_console_suspend to debug) hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 .... snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 azx_single_wait_for_response: 42 callbacks suppressed
This seems appearing when the sound driver went to suspend before i915 driver. Then i915 driver disables HDMI/DP audio bit and calls the registered notifier, and the HDA codec tries to handle it as a hot(un)plug. But since the driver is already in the suspended state, it fails miserably.
As this is a sort of spurious wakeup, it can be ignored safely, as long as it's delivered during the system suspend. OTOH, if a notification comes during the runtime suspend, the situation is different: we need to wake up. But during the system suspend, such a notification can't be the reason for a wakeup.
This patch addresses it by a simple check of the current sound card status. The skipped notification doesn't matter because the HDA driver will check the plugged status forcibly at the resume in return.
Then, why the card status, not a runtime PM status or else? The HDA controller driver is supposed to set the card status to D3 at the system suspend but not at the runtime suspend. So we can see it as a flag that is set only for the system suspend. Admittedly, it's a bit ugly, but it should work well for now.
Reported-and-tested-by: "Zhang, Xiong Y" xiong.y.zhang@intel.com Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events') Cc: stable@vger.kernel.org # v4.3+ Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..4b6fb668c91c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,12 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
+ /* skip notification during system suspend (but not in runtime PM); + * the state will be updated at resume + */ + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) + return; + check_presence_and_report(codec, pin_nid); }
On 2015-11-27 14:38, Takashi Iwai wrote:
On Fri, 27 Nov 2015 03:55:28 +0100, Zhang, Xiong Y wrote:
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during suspend */
- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid); }
[Zhang, Xiong Y] yes, this patch could remove the error message.
Alright, then it's indeed a failure in the sound driver side. Below is the official patch I'm going to merge.
Just a quick question; have you checked that this does not bring back the original bug the entire patch set was supposed to fix?
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend
The recent addition of ELD notifier for Intel HDMI/DP codec may lead the bad codec connection found as kernel messages like below: Suspending console(s) (use no_console_suspend to debug) hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 .... snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 azx_single_wait_for_response: 42 callbacks suppressed
This seems appearing when the sound driver went to suspend before i915 driver. Then i915 driver disables HDMI/DP audio bit and calls the registered notifier, and the HDA codec tries to handle it as a hot(un)plug. But since the driver is already in the suspended state, it fails miserably.
As this is a sort of spurious wakeup, it can be ignored safely, as long as it's delivered during the system suspend. OTOH, if a notification comes during the runtime suspend, the situation is different: we need to wake up. But during the system suspend, such a notification can't be the reason for a wakeup.
This patch addresses it by a simple check of the current sound card status. The skipped notification doesn't matter because the HDA driver will check the plugged status forcibly at the resume in return.
Then, why the card status, not a runtime PM status or else? The HDA controller driver is supposed to set the card status to D3 at the system suspend but not at the runtime suspend. So we can see it as a flag that is set only for the system suspend. Admittedly, it's a bit ugly, but it should work well for now.
Reported-and-tested-by: "Zhang, Xiong Y" xiong.y.zhang@intel.com Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events') Cc: stable@vger.kernel.org # v4.3+ Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/patch_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..4b6fb668c91c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,12 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during system suspend (but not in runtime PM);
* the state will be updated at resume*/- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid); }
On Fri, 27 Nov 2015 14:45:31 +0100, David Henningsson wrote:
On 2015-11-27 14:38, Takashi Iwai wrote:
On Fri, 27 Nov 2015 03:55:28 +0100, Zhang, Xiong Y wrote:
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during suspend */
- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid); }
[Zhang, Xiong Y] yes, this patch could remove the error message.
Alright, then it's indeed a failure in the sound driver side. Below is the official patch I'm going to merge.
Just a quick question; have you checked that this does not bring back the original bug the entire patch set was supposed to fix?
Do you mean the hotplug handling runtime PM? I tested it locally, but wider ranged tests would be appreciated. In theory, it should work as mentioned in the changelog.
Takashi
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend
The recent addition of ELD notifier for Intel HDMI/DP codec may lead the bad codec connection found as kernel messages like below: Suspending console(s) (use no_console_suspend to debug) hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 .... snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 azx_single_wait_for_response: 42 callbacks suppressed
This seems appearing when the sound driver went to suspend before i915 driver. Then i915 driver disables HDMI/DP audio bit and calls the registered notifier, and the HDA codec tries to handle it as a hot(un)plug. But since the driver is already in the suspended state, it fails miserably.
As this is a sort of spurious wakeup, it can be ignored safely, as long as it's delivered during the system suspend. OTOH, if a notification comes during the runtime suspend, the situation is different: we need to wake up. But during the system suspend, such a notification can't be the reason for a wakeup.
This patch addresses it by a simple check of the current sound card status. The skipped notification doesn't matter because the HDA driver will check the plugged status forcibly at the resume in return.
Then, why the card status, not a runtime PM status or else? The HDA controller driver is supposed to set the card status to D3 at the system suspend but not at the runtime suspend. So we can see it as a flag that is set only for the system suspend. Admittedly, it's a bit ugly, but it should work well for now.
Reported-and-tested-by: "Zhang, Xiong Y" xiong.y.zhang@intel.com Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events') Cc: stable@vger.kernel.org # v4.3+ Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/patch_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..4b6fb668c91c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,12 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04;
- /* skip notification during system suspend (but not in runtime PM);
* the state will be updated at resume*/- if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
return;- check_presence_and_report(codec, pin_nid); }
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On Thu, 26 Nov 2015 16:29:12 +0100, David Henningsson wrote:
On 2015-11-26 16:23, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote: > >>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>>> component ops to get ELD and connection states. It was posted to >>>> alsa-devel shortly ago just as a reference, but this should work well >>>> in such a case, too. The test patches are found in test/hdmi-jack >>>> branch of my sound git tree. >> >> Did you try this branch (merge onto intel-test-nightly)? >> > [Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...?
Would be nice, but it's hard, because as mentioned, many paths evaluating the return value from get_power can't stop the things easily.
Also, one thing that came to my mind now: do we have a dependency in suspend order between i915 and HDA? Wouldn't it happen that i915 driver goes to suspend while HDA still active? Then a return check from get_power doesn't necessarily help because it might hold it.
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback?
Yeah, that's I supposed in below.
So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code.
And I think we need a quicker solution for now. My patchset to use get_eld callback removes the whole power up/down at notification, so we won't have this issue. Thus we need a fix only for 4.3/4.4.
Takashi
On Thu, Nov 26, 2015 at 04:23:16PM +0100, Takashi Iwai wrote:
On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote:
On 2015-11-26 10:24, Takashi Iwai wrote:
On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote:
On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote:
>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new >>> component ops to get ELD and connection states. It was posted to >>> alsa-devel shortly ago just as a reference, but this should work well >>> in such a case, too. The test patches are found in test/hdmi-jack >>> branch of my sound git tree. > > Did you try this branch (merge onto intel-test-nightly)? > [Zhang, Xiong Y] Yes, this branch could fix my issue.
OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes.
Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/asoundef.h> @@ -2352,7 +2353,9 @@ 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);
if (!atomic_read(&codec->core.in_pm) &&
!pm_runtime_suspended(hda_codec_dev(codec)))check_presence_and_report(codec, pin_nid);}
static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend().
OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path.
We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...?
My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off.
Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard.
So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code.
Not calling eld_notify doesn't really help since when we suspend we do a normal modeset. And on platforms where the eld notify interrupt stuff works that will happen. This needs to be solved somewhere else I think. -Daniel
On Fri, 28 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it?
I think there's a slight conflict with Libin's series [1], so both should probably go through the same tree. I'm fine with either.
BR, Jani.
[1] http://mid.gmane.org/1439880714-40931-1-git-send-email-libin.yang@intel.com
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 ++++++++++++++++ include/sound/hda_i915.h | 7 +++++++ sound/hda/hdac_i915.c | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 6 files changed, 79 insertions(+), 4 deletions(-)
-- 1.9.1
On Fri, Aug 28, 2015 at 08:14:48PM +0300, Jani Nikula wrote:
On Fri, 28 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it?
I think there's a slight conflict with Libin's series [1], so both should probably go through the same tree. I'm fine with either.
Yeah makes sense. Takashi can you please pull in this entire series into your tree too?
Thanks, Daniel
On Wed, 02 Sep 2015 13:45:03 +0200, Daniel Vetter wrote:
On Fri, Aug 28, 2015 at 08:14:48PM +0300, Jani Nikula wrote:
On Fri, 28 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it?
I think there's a slight conflict with Libin's series [1], so both should probably go through the same tree. I'm fine with either.
Yeah makes sense. Takashi can you please pull in this entire series into your tree too?
Do you mean Libin's series or David's? I already applied David's patches (including drm/i915 changes) to for-next of sound git tree now. Shall I merge Libin's patches to my branch, too?
The branch is supposed to be stable, so feel free to pull into your testing branch.
Takashi
On Wed, Sep 02, 2015 at 01:59:30PM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 13:45:03 +0200, Daniel Vetter wrote:
On Fri, Aug 28, 2015 at 08:14:48PM +0300, Jani Nikula wrote:
On Fri, 28 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it?
I think there's a slight conflict with Libin's series [1], so both should probably go through the same tree. I'm fine with either.
Yeah makes sense. Takashi can you please pull in this entire series into your tree too?
Do you mean Libin's series or David's? I already applied David's patches (including drm/i915 changes) to for-next of sound git tree now. Shall I merge Libin's patches to my branch, too?
I was a few days behind on mails and made a bit a mess between these two patch series. I thought both David's and Libin's series is ready, but let's ask Jani to confirm ;-)
The branch is supposed to be stable, so feel free to pull into your testing branch.
Ok. I'll ping you if I do pull something which isn't in upstream just in. -Daniel
On 2015-08-28 19:02, David Henningsson wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Hi Takashi,
Thanks for finally merging it. Just want to make you aware that it seems like you merged v4 instead of v5, so the changes mentioned above did not make it in. Anything either you or I should do about that?
On Thu, 03 Sep 2015 09:52:00 +0200, David Henningsson wrote:
On 2015-08-28 19:02, David Henningsson wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Hi Takashi,
Thanks for finally merging it. Just want to make you aware that it seems like you merged v4 instead of v5, so the changes mentioned above did not make it in. Anything either you or I should do about that?
Argh, that's bad. Could you post incremental patches to correct to v5?
At the next time please put revision number in the patch itself, not only in cover letter. You can do it with --subject-prefix option of git-format-patch.
thanks,
Takashi
On 2015-09-03 10:31, Takashi Iwai wrote:
On Thu, 03 Sep 2015 09:52:00 +0200, David Henningsson wrote:
On 2015-08-28 19:02, David Henningsson wrote:
Hopefully last version? :-)
- Added commit text about duplicate events (patch 4/4)
- Added locks in bind/unbind on i915 side (patch 2/4)
- Fixed docbook comments in i915 struct (patch 1/4)
- Removed port_mst_streaming parameter (patch 1/4)
- Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing.
Hi Takashi,
Thanks for finally merging it. Just want to make you aware that it seems like you merged v4 instead of v5, so the changes mentioned above did not make it in. Anything either you or I should do about that?
Argh, that's bad. Could you post incremental patches to correct to v5?
Posted now. To avoid spamming it was to you and the alsa-devel ML only.
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-September/097216.h...
At the next time please put revision number in the patch itself, not only in cover letter. You can do it with --subject-prefix option of git-format-patch.
Ok, will try to remember :-)
On Thu, 03 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
Argh, that's bad. Could you post incremental patches to correct to v5?
At the next time please put revision number in the patch itself, not only in cover letter. You can do it with --subject-prefix option of git-format-patch.
FWIW you can also use -vN (i.e. -v5 in this case) parameter.
BR, Jani.
participants (7)
-
Daniel Vetter -
David Henningsson -
Imre Deak -
Jani Nikula -
Takashi Iwai -
Ville Syrjälä -
Zhang, Xiong Y