[PATCH 1/2] hda - fix ELD memory leak

[PATCH 1/2] hda - fix ELD memory leak

[Wu Fengguang]

memset(eld) clears eld->proc_entry which will leak the struct
snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 sound/pci/hda/hda_eld.c    |    5 +----
 sound/pci/hda/patch_hdmi.c |   11 +++++------
 2 files changed, 6 insertions(+), 10 deletions(-)

--- linux.orig/sound/pci/hda/hda_eld.c	2011-11-15 21:02:25.000000000 +0800
+++ linux/sound/pci/hda/hda_eld.c	2011-11-15 21:13:46.000000000 +0800
@@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
 					buf + ELD_FIXED_BYTES + mnl + 3 * i);
 	}
 
+	e->eld_valid = true;
 	return 0;
 
 out_fail:
-	e->eld_ver = 0;
 	return -EINVAL;
 }
 
@@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
 	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
 	 */
 
-	if (!eld->eld_valid)
-		return -ENOENT;
-
 	size = snd_hdmi_get_eld_size(codec, nid);
 	if (size == 0) {
 		/* wfg: workaround for ASUS P5E-VM HDMI board */
--- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:02:25.000000000 +0800
+++ linux/sound/pci/hda/patch_hdmi.c	2011-11-15 21:13:42.000000000 +0800
@@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
+	bool eld_valid = false;
 
-	memset(eld, 0, sizeof(*eld));
+	eld->eld_valid = false;
 
 	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
 	if (eld->monitor_present)
-		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
-	else
-		eld->eld_valid	= 0;
+		eld_valid	= !!(present & AC_PINSENSE_ELDV);
 
 	printk(KERN_INFO
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld->eld_valid)
+	if (eld_valid)
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);

[PATCH 2/2] hda - delayed ELD repoll

[Wu Fengguang]

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 sound/pci/hda/hda_local.h  |    2 +
 sound/pci/hda/patch_hdmi.c |   49 ++++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 7 deletions(-)

--- linux.orig/sound/pci/hda/hda_local.h	2011-11-15 21:29:53.000000000 +0800
+++ linux/sound/pci/hda/hda_local.h	2011-11-15 21:29:54.000000000 +0800
@@ -655,6 +655,8 @@ struct hdmi_eld {
 #ifdef CONFIG_PROC_FS
 	struct snd_info_entry *proc_entry;
 #endif
+	struct hda_codec *codec;
+	struct delayed_work work;
 };
 
 int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid);
--- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:29:53.000000000 +0800
+++ linux/sound/pci/hda/patch_hdmi.c	2011-11-15 21:31:48.000000000 +0800
@@ -746,7 +746,7 @@ static void hdmi_setup_audio_infoframe(s
  */
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld);
+			       struct hdmi_eld *eld, bool retry);
 
 static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 {
@@ -766,7 +766,7 @@ static void hdmi_intrinsic_event(struct 
 		return;
 	eld = &spec->pins[pin_idx].sink_eld;
 
-	hdmi_present_sense(codec, pin_nid, eld);
+	hdmi_present_sense(codec, pin_nid, eld, true);
 
 	/*
 	 * HDMI sink's ELD info cannot always be retrieved for now, e.g.
@@ -969,7 +969,7 @@ static int hdmi_read_pin_conn(struct hda
 }
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld)
+			       struct hdmi_eld *eld, bool retry)
 {
 	/*
 	 * Always execute a GetPinSense verb here, even when called from
@@ -992,13 +992,31 @@ static void hdmi_present_sense(struct hd
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
 		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld_valid)
+	if (eld_valid) {
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
+		else {
+			queue_delayed_work(codec->bus->workq,
+					   &eld->work,
+					   msecs_to_jiffies(300));
+		}
+	}
 
 	snd_hda_input_jack_report(codec, pin_nid);
 }
 
+static void hda_eld_work(struct work_struct *work)
+{
+	struct hdmi_eld *eld = container_of(
+				container_of(work, struct delayed_work, work),
+				struct hdmi_eld, work);
+	struct hdmi_spec_per_pin *per_pin =
+			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
+	struct hda_codec *codec = eld->codec;
+
+	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
+}
+
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
 	if (err < 0)
 		return err;
 
-	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
+	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld, false);
 	return 0;
 }
 
@@ -1263,6 +1281,23 @@ static int generic_hdmi_build_controls(s
 	return 0;
 }
 
+static void snd_hda_eld_init(struct hda_codec *codec, struct hdmi_eld *eld,
+			     int pin_idx)
+{
+	eld->codec = codec;
+	INIT_DELAYED_WORK(&eld->work, hda_eld_work);
+
+	snd_hda_eld_proc_new(codec, eld, pin_idx);
+}
+
+static void snd_hda_eld_free(struct hda_codec *codec, struct hdmi_eld *eld)
+{
+	cancel_delayed_work(&eld->work);
+	flush_workqueue(codec->bus->workq);
+
+	snd_hda_eld_proc_free(codec, eld);
+}
+
 static int generic_hdmi_init(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1278,7 +1313,7 @@ static int generic_hdmi_init(struct hda_
 				    AC_VERB_SET_UNSOLICITED_ENABLE,
 				    AC_USRSP_EN | pin_nid);
 
-		snd_hda_eld_proc_new(codec, eld, pin_idx);
+		snd_hda_eld_init(codec, eld, pin_idx);
 	}
 	return 0;
 }
@@ -1292,7 +1327,7 @@ static void generic_hdmi_free(struct hda
 		struct hdmi_spec_per_pin *per_pin = &spec->pins[pin_idx];
 		struct hdmi_eld *eld = &per_pin->sink_eld;
 
-		snd_hda_eld_proc_free(codec, eld);
+		snd_hda_eld_free(codec, eld);
 	}
 	snd_hda_input_jack_free(codec);

Re: [PATCH 2/2] hda - delayed ELD repoll

[Wu Fengguang]

> -	if (eld_valid)
> +	if (eld_valid) {
>  		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
>  			snd_hdmi_show_eld(eld);
> +		else {

Oops, forgot testing @retry here! Updated patch follows.

> +			queue_delayed_work(codec->bus->workq,
> +					   &eld->work,
> +					   msecs_to_jiffies(300));
> +		}
> +	}

[PATCH 2/2 v2] hda - delayed ELD repoll

[Wu Fengguang]

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 sound/pci/hda/hda_local.h  |    2 +
 sound/pci/hda/patch_hdmi.c |   49 ++++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 7 deletions(-)

--- linux.orig/sound/pci/hda/hda_local.h	2011-11-15 21:29:53.000000000 +0800
+++ linux/sound/pci/hda/hda_local.h	2011-11-15 21:29:54.000000000 +0800
@@ -655,6 +655,8 @@ struct hdmi_eld {
 #ifdef CONFIG_PROC_FS
 	struct snd_info_entry *proc_entry;
 #endif
+	struct hda_codec *codec;
+	struct delayed_work work;
 };
 
 int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid);
--- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:29:53.000000000 +0800
+++ linux/sound/pci/hda/patch_hdmi.c	2011-11-16 00:50:50.000000000 +0800
@@ -746,7 +746,7 @@ static void hdmi_setup_audio_infoframe(s
  */
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld);
+			       struct hdmi_eld *eld, bool retry);
 
 static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 {
@@ -766,7 +766,7 @@ static void hdmi_intrinsic_event(struct 
 		return;
 	eld = &spec->pins[pin_idx].sink_eld;
 
-	hdmi_present_sense(codec, pin_nid, eld);
+	hdmi_present_sense(codec, pin_nid, eld, true);
 
 	/*
 	 * HDMI sink's ELD info cannot always be retrieved for now, e.g.
@@ -969,7 +969,7 @@ static int hdmi_read_pin_conn(struct hda
 }
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld)
+			       struct hdmi_eld *eld, bool retry)
 {
 	/*
 	 * Always execute a GetPinSense verb here, even when called from
@@ -992,13 +992,31 @@ static void hdmi_present_sense(struct hd
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
 		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld_valid)
+	if (eld_valid) {
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
+		else if (retry) {
+			queue_delayed_work(codec->bus->workq,
+					   &eld->work,
+					   msecs_to_jiffies(300));
+		}
+	}
 
 	snd_hda_input_jack_report(codec, pin_nid);
 }
 
+static void hda_eld_work(struct work_struct *work)
+{
+	struct hdmi_eld *eld = container_of(
+				container_of(work, struct delayed_work, work),
+				struct hdmi_eld, work);
+	struct hdmi_spec_per_pin *per_pin =
+			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
+	struct hda_codec *codec = eld->codec;
+
+	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
+}
+
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
 	if (err < 0)
 		return err;
 
-	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
+	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld, false);
 	return 0;
 }
 
@@ -1263,6 +1281,23 @@ static int generic_hdmi_build_controls(s
 	return 0;
 }
 
+static void snd_hda_eld_init(struct hda_codec *codec, struct hdmi_eld *eld,
+			     int pin_idx)
+{
+	eld->codec = codec;
+	INIT_DELAYED_WORK(&eld->work, hda_eld_work);
+
+	snd_hda_eld_proc_new(codec, eld, pin_idx);
+}
+
+static void snd_hda_eld_free(struct hda_codec *codec, struct hdmi_eld *eld)
+{
+	cancel_delayed_work(&eld->work);
+	flush_workqueue(codec->bus->workq);
+
+	snd_hda_eld_proc_free(codec, eld);
+}
+
 static int generic_hdmi_init(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1278,7 +1313,7 @@ static int generic_hdmi_init(struct hda_
 				    AC_VERB_SET_UNSOLICITED_ENABLE,
 				    AC_USRSP_EN | pin_nid);
 
-		snd_hda_eld_proc_new(codec, eld, pin_idx);
+		snd_hda_eld_init(codec, eld, pin_idx);
 	}
 	return 0;
 }
@@ -1292,7 +1327,7 @@ static void generic_hdmi_free(struct hda
 		struct hdmi_spec_per_pin *per_pin = &spec->pins[pin_idx];
 		struct hdmi_eld *eld = &per_pin->sink_eld;
 
-		snd_hda_eld_proc_free(codec, eld);
+		snd_hda_eld_free(codec, eld);
 	}
 	snd_hda_input_jack_free(codec);

Re: [PATCH 2/2 v2] hda - delayed ELD repoll

[Takashi Iwai]

At Wed, 16 Nov 2011 00:57:08 +0800,
Wu Fengguang wrote:
> +static void hda_eld_work(struct work_struct *work)
> +{
> +	struct hdmi_eld *eld = container_of(
> +				container_of(work, struct delayed_work, work),
> +				struct hdmi_eld, work);

Ugh, hardly to read.  Slightly better to use to_delayed_work()?

	struct hdmi_eld *eld = container_of(to_delayed_work(work),
					    struct hdmi_eld, work);

> +	struct hdmi_spec_per_pin *per_pin =
> +			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
> +	struct hda_codec *codec = eld->codec;
> +
> +	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
> +}
> +
>  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  {
>  	struct hdmi_spec *spec = codec->spec;
> @@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
>  	if (err < 0)
>  		return err;
>  
> -	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
> +	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld, false);
...

Looking through the code, I wonder how about keeping work and codec
pointer in struct hdmi_spec_per_pin instead of struct hdmi_eld.  Then
we don't need to expose it in hda_local.h, and changes would be
slightly more compact.

hdmi_present_sense() can be modified to receive hdmi_sepc_per_pin*
directly instead of individual nid and eld pointer.


thanks,

Takashi

Re: [PATCH 2/2 v2] hda - delayed ELD repoll

[Wu Fengguang]

On Wed, Nov 16, 2011 at 01:10:37AM +0800, Takashi Iwai wrote:
> At Wed, 16 Nov 2011 00:57:08 +0800,
> Wu Fengguang wrote:
> > +static void hda_eld_work(struct work_struct *work)
> > +{
> > +	struct hdmi_eld *eld = container_of(
> > +				container_of(work, struct delayed_work, work),
> > +				struct hdmi_eld, work);
> 
> Ugh, hardly to read.  Slightly better to use to_delayed_work()?
> 
> 	struct hdmi_eld *eld = container_of(to_delayed_work(work),
> 					    struct hdmi_eld, work);

Yeah that looks much better! Changed to:

+static void hdmi_repoll_eld(struct work_struct *work)
+{     
+       struct hdmi_spec_per_pin *per_pin =
+       container_of(to_delayed_work(work), struct hdmi_spec_per_pin, work);
+       
+       hdmi_present_sense(per_pin, false);
+}

> > +	struct hdmi_spec_per_pin *per_pin =
> > +			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
> > +	struct hda_codec *codec = eld->codec;
> > +
> > +	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
> > +}
> > +
> >  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> >  {
> >  	struct hdmi_spec *spec = codec->spec;
> > @@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
> >  	if (err < 0)
> >  		return err;
> >  
> > -	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
> > +	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld, false);
> ...
> 
> Looking through the code, I wonder how about keeping work and codec
> pointer in struct hdmi_spec_per_pin instead of struct hdmi_eld.  Then
> we don't need to expose it in hda_local.h, and changes would be
> slightly more compact.
> 
> hdmi_present_sense() can be modified to receive hdmi_sepc_per_pin*
> directly instead of individual nid and eld pointer.

Good idea! With the changes I get a much smaller patch :-)

I'll repost the patch series after some more testing.

Thanks,
Fengguang
---

Subject: hda - delayed ELD repoll
Date: Mon Nov 14 11:31:00 CST 2011

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 sound/pci/hda/patch_hdmi.c |   36 ++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

--- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-16 09:53:01.000000000 +0800
+++ linux/sound/pci/hda/patch_hdmi.c	2011-11-16 10:33:06.000000000 +0800
@@ -65,7 +65,10 @@ struct hdmi_spec_per_pin {
 	hda_nid_t pin_nid;
 	int num_mux_nids;
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
+
+	struct hda_codec *codec;
 	struct hdmi_eld sink_eld;
+	struct delayed_work work;
 };
 
 struct hdmi_spec {
@@ -745,8 +748,7 @@ static void hdmi_setup_audio_infoframe(s
  * Unsolicited events
  */
 
-static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld);
+static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, bool retry);
 
 static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 {
@@ -766,7 +768,7 @@ static void hdmi_intrinsic_event(struct 
 		return;
 	eld = &spec->pins[pin_idx].sink_eld;
 
-	hdmi_present_sense(codec, pin_nid, eld);
+	hdmi_present_sense(&spec->pins[pin_idx], true);
 
 	/*
 	 * HDMI sink's ELD info cannot always be retrieved for now, e.g.
@@ -968,9 +970,11 @@ static int hdmi_read_pin_conn(struct hda
 	return 0;
 }
 
-static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld)
+static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, bool retry)
 {
+	struct hda_codec *codec = per_pin->codec;
+	struct hdmi_eld *eld = &per_pin->sink_eld;
+	hda_nid_t pin_nid = per_pin->pin_nid;
 	/*
 	 * Always execute a GetPinSense verb here, even when called from
 	 * hdmi_intrinsic_event; for some NVIDIA HW, the unsolicited
@@ -992,13 +996,27 @@ static void hdmi_present_sense(struct hd
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
 		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld_valid)
+	if (eld_valid) {
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
+		else if (retry) {
+			queue_delayed_work(codec->bus->workq,
+					   &per_pin->work,
+					   msecs_to_jiffies(300));
+		}
+	}
 
 	snd_hda_input_jack_report(codec, pin_nid);
 }
 
+static void hdmi_repoll_eld(struct work_struct *work)
+{
+	struct hdmi_spec_per_pin *per_pin =
+	container_of(to_delayed_work(work), struct hdmi_spec_per_pin, work);
+
+	hdmi_present_sense(per_pin, false);
+}
+
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
 	if (err < 0)
 		return err;
 
-	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
+	hdmi_present_sense(per_pin, false);
 	return 0;
 }
 
@@ -1278,6 +1296,8 @@ static int generic_hdmi_init(struct hda_
 				    AC_VERB_SET_UNSOLICITED_ENABLE,
 				    AC_USRSP_EN | pin_nid);
 
+		per_pin->codec = codec;
+		INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld);
 		snd_hda_eld_proc_new(codec, eld, pin_idx);
 	}
 	return 0;
@@ -1292,10 +1312,12 @@ static void generic_hdmi_free(struct hda
 		struct hdmi_spec_per_pin *per_pin = &spec->pins[pin_idx];
 		struct hdmi_eld *eld = &per_pin->sink_eld;
 
+		cancel_delayed_work(&per_pin->work);
 		snd_hda_eld_proc_free(codec, eld);
 	}
 	snd_hda_input_jack_free(codec);
 
+	flush_workqueue(codec->bus->workq);
 	kfree(spec);
 }

Re: [PATCH 2/2 v2] hda - delayed ELD repoll

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 3092 bytes --]

Dear Fengguang,


Am Mittwoch, den 16.11.2011, 00:57 +0800 schrieb Wu Fengguang:
> The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
> between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
> actually readable. During the time the ELD buffer is mysteriously all 0.
> 
> Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.
> 
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

other then my comment at the end, please note that it is very confusing
for European (and other folks) what your first name is.

If it is Fengguang then you should use

Fengguang Wu or Wu, Fengguang

where I prefer the first one.

> ---
>  sound/pci/hda/hda_local.h  |    2 +
>  sound/pci/hda/patch_hdmi.c |   49 ++++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 7 deletions(-)
> 
> --- linux.orig/sound/pci/hda/hda_local.h	2011-11-15 21:29:53.000000000 +0800
> +++ linux/sound/pci/hda/hda_local.h	2011-11-15 21:29:54.000000000 +0800
> @@ -655,6 +655,8 @@ struct hdmi_eld {
>  #ifdef CONFIG_PROC_FS
>  	struct snd_info_entry *proc_entry;
>  #endif
> +	struct hda_codec *codec;
> +	struct delayed_work work;
>  };
>  
>  int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid);
> --- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:29:53.000000000 +0800
> +++ linux/sound/pci/hda/patch_hdmi.c	2011-11-16 00:50:50.000000000 +0800
> @@ -746,7 +746,7 @@ static void hdmi_setup_audio_infoframe(s
>   */
>  
>  static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
> -			       struct hdmi_eld *eld);
> +			       struct hdmi_eld *eld, bool retry);
>  
>  static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>  {
> @@ -766,7 +766,7 @@ static void hdmi_intrinsic_event(struct 
>  		return;
>  	eld = &spec->pins[pin_idx].sink_eld;
>  
> -	hdmi_present_sense(codec, pin_nid, eld);
> +	hdmi_present_sense(codec, pin_nid, eld, true);
>  
>  	/*
>  	 * HDMI sink's ELD info cannot always be retrieved for now, e.g.
> @@ -969,7 +969,7 @@ static int hdmi_read_pin_conn(struct hda
>  }
>  
>  static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
> -			       struct hdmi_eld *eld)
> +			       struct hdmi_eld *eld, bool retry)
>  {
>  	/*
>  	 * Always execute a GetPinSense verb here, even when called from
> @@ -992,13 +992,31 @@ static void hdmi_present_sense(struct hd
>  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
>  		codec->addr, pin_nid, eld->monitor_present, eld_valid);
>  
> -	if (eld_valid)
> +	if (eld_valid) {
>  		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
>  			snd_hdmi_show_eld(eld);
> +		else if (retry) {
> +			queue_delayed_work(codec->bus->workq,
> +					   &eld->work,
> +					   msecs_to_jiffies(300));
> +		}

Would adding a comment referring to the discussion thread be beneficial
for code readers asking why a delay is needed?

> +	}
>  
>  	snd_hda_input_jack_report(codec, pin_nid);
>  }

[…]


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] hda - delayed ELD repoll

[Stephen Warren]

Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:
> The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
> between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
> actually readable. During the time the ELD buffer is mysteriously all 0.
> 
> Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Any idea why; is the graphics driver writing the ELD data to the audio HW
after triggering the unsolicited even rather than before, or something
like that? 250mS almost sounds like it's setting ELDV in the audio HW,
then going and reading the EDID, then writing the EDID to the audio HW;
perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's
meant to be setting just PRESENT, and later setting ELDV?

-- 
nvpublic

Re: [alsa-devel] [PATCH 2/2] hda - delayed ELD repoll

[Wu Fengguang]

On Wed, Nov 16, 2011 at 02:25:00AM +0800, Stephen Warren wrote:
> Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:
> > The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
> > between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
> > actually readable. During the time the ELD buffer is mysteriously all 0.
> > 
> > Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.
> 
> Any idea why; is the graphics driver writing the ELD data to the audio HW
> after triggering the unsolicited even rather than before, or something
> like that?

Nope. The graphics driver is doing

        eld_valid = 0
        write to eld buffer
        eld_valid = 1

> 250mS almost sounds like it's setting ELDV in the audio HW,
> then going and reading the EDID, then writing the EDID to the audio HW;
> perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's
> meant to be setting just PRESENT, and later setting ELDV?

>From the debug dmesg, I'm pretty sure that the ELDV events are
triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register
writes. Since the ELD data is already prepared, there is no EDID read
in between.

Below is the dmesg representing a video mode set.

ELD writes from the graphics driver

[  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
[  424.257670] [drm:ironlake_write_eld], ELD on pipe B
[  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
[  424.262156] [drm:ironlake_write_eld], ELD size 13

ELD events received by audio driver (eld reads 0)

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.272415] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
[  424.274566] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.277027] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.283157] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0

graphics driver go on with its job

[  424.314331] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.367183] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.368960] [drm:ironlake_update_wm], FIFO watermarks For pipe A - plane 5, cursor: 6
[  424.370700] [drm:ironlake_update_wm], FIFO watermarks For pipe B - plane 42, cursor: 6
[  424.424056] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.476906] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.479169] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x100
[  424.481084] [drm:ironlake_fdi_link_train], FDI train 1 done.
[  424.483452] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x600
[  424.485444] [drm:ironlake_fdi_link_train], FDI train 2 done.
[  424.486765] [drm:ironlake_fdi_link_train], FDI train done
[  424.490820] [drm:intel_update_fbc], 
[  424.491841] [drm:drm_crtc_helper_set_config], Setting connector DPMS state to on
[  424.494449] [drm:drm_crtc_helper_set_config],        [CONNECTOR:12:HDMI-A-2] set DPMS on
[  424.505904] [drm:intel_prepare_page_flip], preparing flip with no unpin work?

audio driver repoll the ELD after 300ms (eld data readable now)

[  424.574763] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.580867] HDMI: detected monitor RX-V1800 at connection type HDMI
[  424.582413] HDMI: available speakers: FL/FR LFE FC RL/RR RC RLC/RRC
[  424.584740] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000 96000 176400 192000 384000, bits = 16 20 24
[  424.587429] HDMI: supports coding type LPCM: channels = 8, rates = 32000 44100 48000 96000 176400 192000 384000, bits = 16 20 24
[  424.590968] HDMI: supports coding type AC-3: channels = 6, rates = 32000 44100 48000, max bitrate = 640000
[  424.594164] HDMI: supports coding type DTS: channels = 7, rates = 32000 44100 48000 96000 176400, max bitrate = 1536000
[  424.597563] HDMI: supports coding type DSD (One Bit Audio): channels = 6, rates = 44100
[  424.600284] HDMI: supports coding type E-AC-3/DD+ (Dolby Digital Plus): channels = 8, rates = 44100 48000
[  424.602420] HDMI: supports coding type MLP (Dolby TrueHD): channels = 8, rates = 48000 176400 384000
[  424.604478] HDMI: supports coding type DTS-HD: channels = 8, rates = 48000 176400 384000

Thanks,
Fengguang

Re: [PATCH 2/2] hda - delayed ELD repoll

[Stephen Warren]

Wu Fengguang wrote at Tuesday, November 15, 2011 7:48 PM:
> On Wed, Nov 16, 2011 at 02:25:00AM +0800, Stephen Warren wrote:
> > Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:
> > > The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
> > > between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
> > > actually readable. During the time the ELD buffer is mysteriously all 0.
> > >
> > > Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.
> >
> > Any idea why; is the graphics driver writing the ELD data to the audio HW
> > after triggering the unsolicited even rather than before, or something
> > like that?
> 
> Nope. The graphics driver is doing
> 
>         eld_valid = 0
>         write to eld buffer
>         eld_valid = 1

OK, that sounds fine.

> > 250mS almost sounds like it's setting ELDV in the audio HW,
> > then going and reading the EDID, then writing the EDID to the audio HW;
> > perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's
> > meant to be setting just PRESENT, and later setting ELDV?
> 
> From the debug dmesg, I'm pretty sure that the ELDV events are
> triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register
> writes. Since the ELD data is already prepared, there is no EDID read
> in between.
> 
> Below is the dmesg representing a video mode set.
> 
> ELD writes from the graphics driver
> 
> [  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
> [  424.257670] [drm:ironlake_write_eld], ELD on pipe B
> [  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
> [  424.262156] [drm:ironlake_write_eld], ELD size 13
> 
> ELD events received by audio driver (eld reads 0)
> 
> [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

That line makes sense.

> [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
that message; it seems that the unsolicited response contains the correct
data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

> [  424.272415] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
> [  424.274566] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> [  424.277027] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> [  424.283157] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
> 
> graphics driver go on with its job
> 
> [  424.314331] [drm:intel_wait_for_vblank], vblank wait timed out
> [  424.367183] [drm:intel_wait_for_vblank], vblank wait timed out
> [  424.368960] [drm:ironlake_update_wm], FIFO watermarks For pipe A - plane 5, cursor: 6
> [  424.370700] [drm:ironlake_update_wm], FIFO watermarks For pipe B - plane 42, cursor: 6
> [  424.424056] [drm:intel_wait_for_vblank], vblank wait timed out
> [  424.476906] [drm:intel_wait_for_vblank], vblank wait timed out
> [  424.479169] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x100
> [  424.481084] [drm:ironlake_fdi_link_train], FDI train 1 done.
> [  424.483452] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x600
> [  424.485444] [drm:ironlake_fdi_link_train], FDI train 2 done.
> [  424.486765] [drm:ironlake_fdi_link_train], FDI train done
> [  424.490820] [drm:intel_update_fbc],
> [  424.491841] [drm:drm_crtc_helper_set_config], Setting connector DPMS state to on
> [  424.494449] [drm:drm_crtc_helper_set_config],        [CONNECTOR:12:HDMI-A-2] set DPMS on
> [  424.505904] [drm:intel_prepare_page_flip], preparing flip with no unpin work?
> 
> audio driver repoll the ELD after 300ms (eld data readable now)
> 
> [  424.574763] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> [  424.580867] HDMI: detected monitor RX-V1800 at connection type HDMI
> [  424.582413] HDMI: available speakers: FL/FR LFE FC RL/RR RC RLC/RRC
...

OK, well I guess this make it work, and I don't think the patch will cause
any issues on HW without this issue, so the patch is fine. I'd still love
to understand why this is happening though.

-- 
nvpublic

Re: [PATCH 2/2] hda - delayed ELD repoll

[Takashi Iwai]

At Wed, 16 Nov 2011 07:51:28 -0800,
Stephen Warren wrote:
> 
> > > 250mS almost sounds like it's setting ELDV in the audio HW,
> > > then going and reading the EDID, then writing the EDID to the audio HW;
> > > perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's
> > > meant to be setting just PRESENT, and later setting ELDV?
> > 
> > From the debug dmesg, I'm pretty sure that the ELDV events are
> > triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register
> > writes. Since the ELD data is already prepared, there is no EDID read
> > in between.
> > 
> > Below is the dmesg representing a video mode set.
> > 
> > ELD writes from the graphics driver
> > 
> > [  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
> > [  424.257670] [drm:ironlake_write_eld], ELD on pipe B
> > [  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
> > [  424.262156] [drm:ironlake_write_eld], ELD size 13
> > 
> > ELD events received by audio driver (eld reads 0)
> > 
> > [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
> 
> That line makes sense.
> 
> > [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> 
> I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
> that message; it seems that the unsolicited response contains the correct
> data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

Note that this bit isn't from GET_PIN_SENSE verb but the bit in the
unsol event argument.


Takashi

Re: [PATCH 2/2] hda - delayed ELD repoll

[Stephen Warren]

Takashi Iwai wrote at Wednesday, November 16, 2011 8:58 AM:
> At Wed, 16 Nov 2011 07:51:28 -0800, Stephen Warren wrote:
...
> > > [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
> >
> > That line makes sense.
> >
> > > [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> >
> > I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
> > that message; it seems that the unsolicited response contains the correct
> > data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.
> 
> Note that this bit isn't from GET_PIN_SENSE verb but the bit in the
> unsol event argument.

The first message above prints data from the unsolicited event. The second
message above is derived from the GET_PIN_SENSE verb.

-- 
nvpublic

Re: [alsa-devel] [PATCH 2/2] hda - delayed ELD repoll

[Takashi Iwai]

At Wed, 16 Nov 2011 08:12:04 -0800,
Stephen Warren wrote:
> 
> Takashi Iwai wrote at Wednesday, November 16, 2011 8:58 AM:
> > At Wed, 16 Nov 2011 07:51:28 -0800, Stephen Warren wrote:
> ...
> > > > [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
> > >
> > > That line makes sense.
> > >
> > > > [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> > >
> > > I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
> > > that message; it seems that the unsolicited response contains the correct
> > > data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.
> > 
> > Note that this bit isn't from GET_PIN_SENSE verb but the bit in the
> > unsol event argument.
> 
> The first message above prints data from the unsolicited event. The second
> message above is derived from the GET_PIN_SENSE verb.

Ah, right.  Too similar lines...


thanks,

Takashi

Re: [PATCH 2/2] hda - delayed ELD repoll

[Wu Fengguang]

On Wed, Nov 16, 2011 at 11:51:28PM +0800, Stephen Warren wrote:
> Wu Fengguang wrote at Tuesday, November 15, 2011 7:48 PM:
> > On Wed, Nov 16, 2011 at 02:25:00AM +0800, Stephen Warren wrote:
> > > Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:
> > > > The Intel HDMI chips (ironlake at least) are found to have ~250ms delay
> > > > between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes
> > > > actually readable. During the time the ELD buffer is mysteriously all 0.
> > > >
> > > > Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.
> > >
> > > Any idea why; is the graphics driver writing the ELD data to the audio HW
> > > after triggering the unsolicited even rather than before, or something
> > > like that?
> > 
> > Nope. The graphics driver is doing
> > 
> >         eld_valid = 0
> >         write to eld buffer
> >         eld_valid = 1
> 
> OK, that sounds fine.
> 
> > > 250mS almost sounds like it's setting ELDV in the audio HW,
> > > then going and reading the EDID, then writing the EDID to the audio HW;
> > > perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's
> > > meant to be setting just PRESENT, and later setting ELDV?
> > 
> > From the debug dmesg, I'm pretty sure that the ELDV events are
> > triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register
> > writes. Since the ELD data is already prepared, there is no EDID read
> > in between.
> > 
> > Below is the dmesg representing a video mode set.
> > 
> > ELD writes from the graphics driver
> > 
> > [  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
> > [  424.257670] [drm:ironlake_write_eld], ELD on pipe B
> > [  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
> > [  424.262156] [drm:ironlake_write_eld], ELD size 13
> > 
> > ELD events received by audio driver (eld reads 0)
> > 
> > [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
> 
> That line makes sense.
> 
> > [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> 
> I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
> that message; it seems that the unsolicited response contains the correct
> data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

It depends on timing. When audio driver receives the unsolicited event, 
graphics driver has finished with "eld_valid = 1", hence
AC_VERB_GET_PIN_SENSE returns ELDV=1.

It's not happening /all the time/ though. For example here is another
dmesg showing a different timing on the same test box.

[  467.561516] [drm:intel_dp_mode_set], Enabling DP audio on pipe B
[  467.567503] [drm:intel_write_eld], ELD on [CONNECTOR:26:DP-3], [ENCODER:27:TMDS-27]
[  467.574207] [drm:ironlake_write_eld], ELD on pipe B
[  467.579346] [drm:ironlake_write_eld], Audio directed to unknown port
[  467.584724] [drm:ironlake_write_eld], ELD: DisplayPort detected
[  467.586540] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
[  467.599608] [drm:ironlake_write_eld],                    
[  467.599922] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
[  467.605834] ELD size 9                                           
[  467.610434] [drm:sandybridge_update_wm], FIFO watermarks For pipe A - plane 7, cursor: 6
[  467.612365] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1
[  467.620654] [drm:sandybridge_update_wm],                 
[  467.620765] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1

> > [  424.272415] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
> > [  424.274566] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> > [  424.277027] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> > [  424.283157] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
> > 
> > graphics driver go on with its job
> > 
> > [  424.314331] [drm:intel_wait_for_vblank], vblank wait timed out
> > [  424.367183] [drm:intel_wait_for_vblank], vblank wait timed out
> > [  424.368960] [drm:ironlake_update_wm], FIFO watermarks For pipe A - plane 5, cursor: 6
> > [  424.370700] [drm:ironlake_update_wm], FIFO watermarks For pipe B - plane 42, cursor: 6
> > [  424.424056] [drm:intel_wait_for_vblank], vblank wait timed out
> > [  424.476906] [drm:intel_wait_for_vblank], vblank wait timed out
> > [  424.479169] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x100
> > [  424.481084] [drm:ironlake_fdi_link_train], FDI train 1 done.
> > [  424.483452] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x600
> > [  424.485444] [drm:ironlake_fdi_link_train], FDI train 2 done.
> > [  424.486765] [drm:ironlake_fdi_link_train], FDI train done
> > [  424.490820] [drm:intel_update_fbc],
> > [  424.491841] [drm:drm_crtc_helper_set_config], Setting connector DPMS state to on
> > [  424.494449] [drm:drm_crtc_helper_set_config],        [CONNECTOR:12:HDMI-A-2] set DPMS on
> > [  424.505904] [drm:intel_prepare_page_flip], preparing flip with no unpin work?
> > 
> > audio driver repoll the ELD after 300ms (eld data readable now)
> > 
> > [  424.574763] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> > [  424.580867] HDMI: detected monitor RX-V1800 at connection type HDMI
> > [  424.582413] HDMI: available speakers: FL/FR LFE FC RL/RR RC RLC/RRC
> ...
> 
> OK, well I guess this make it work, and I don't think the patch will cause
> any issues on HW without this issue, so the patch is fine. I'd still love
> to understand why this is happening though.

I'd like to know the root cause, too... However enabling drm debugging
does not show any co-related event that turns the ELD into readable state,
and I cannot find clues in the spec, too...

Thanks,
Fengguang

Re: [PATCH 2/2] hda - delayed ELD repoll

[Wu Fengguang]

> > > Below is the dmesg representing a video mode set.
> > > 
> > > ELD writes from the graphics driver
> > > 
> > > [  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
> > > [  424.257670] [drm:ironlake_write_eld], ELD on pipe B
> > > [  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
> > > [  424.262156] [drm:ironlake_write_eld], ELD size 13
> > > 
> > > ELD events received by audio driver (eld reads 0)
> > > 
> > > [  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0
> > 
> > That line makes sense.
> > 
> > > [  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
> > 
> > I don't /think/ it's related to this issue, but I wonder why ELDV==1 in
> > that message; it seems that the unsolicited response contains the correct
> > data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.
> 
> It depends on timing. When audio driver receives the unsolicited event, 
> graphics driver has finished with "eld_valid = 1", hence
> AC_VERB_GET_PIN_SENSE returns ELDV=1.
> 
> It's not happening /all the time/ though. For example here is another
> dmesg showing a different timing on the same test box.

Sorry this is actually from another test box. But I did see similar
ones from the same test box.

> [  467.561516] [drm:intel_dp_mode_set], Enabling DP audio on pipe B
> [  467.567503] [drm:intel_write_eld], ELD on [CONNECTOR:26:DP-3], [ENCODER:27:TMDS-27]
> [  467.574207] [drm:ironlake_write_eld], ELD on pipe B
> [  467.579346] [drm:ironlake_write_eld], Audio directed to unknown port
> [  467.584724] [drm:ironlake_write_eld], ELD: DisplayPort detected
> [  467.586540] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
> [  467.599608] [drm:ironlake_write_eld],                    
> [  467.599922] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
> [  467.605834] ELD size 9                                           
> [  467.610434] [drm:sandybridge_update_wm], FIFO watermarks For pipe A - plane 7, cursor: 6
> [  467.612365] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1
> [  467.620654] [drm:sandybridge_update_wm],                 
> [  467.620765] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1

Re: [PATCH 1/2] hda - fix ELD memory leak

[Takashi Iwai]

At Tue, 15 Nov 2011 22:31:55 +0800,
Wu Fengguang wrote:
> 
> memset(eld) clears eld->proc_entry which will leak the struct
> snd_info_entry when unloading the module.
> 
> Fix it by
> - remove memset(eld)
> - set eld->eld_valid to true _after_ all eld fields have been filled
> - don't access the other eld fields when (eld->eld_valid == false)
> 
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

This should be send to stable kernel, too, right?
It appears in 3.1, at least.


Takashi

> ---
>  sound/pci/hda/hda_eld.c    |    5 +----
>  sound/pci/hda/patch_hdmi.c |   11 +++++------
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> --- linux.orig/sound/pci/hda/hda_eld.c	2011-11-15 21:02:25.000000000 +0800
> +++ linux/sound/pci/hda/hda_eld.c	2011-11-15 21:13:46.000000000 +0800
> @@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
>  					buf + ELD_FIXED_BYTES + mnl + 3 * i);
>  	}
>  
> +	e->eld_valid = true;
>  	return 0;
>  
>  out_fail:
> -	e->eld_ver = 0;
>  	return -EINVAL;
>  }
>  
> @@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
>  	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
>  	 */
>  
> -	if (!eld->eld_valid)
> -		return -ENOENT;
> -
>  	size = snd_hdmi_get_eld_size(codec, nid);
>  	if (size == 0) {
>  		/* wfg: workaround for ASUS P5E-VM HDMI board */
> --- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:02:25.000000000 +0800
> +++ linux/sound/pci/hda/patch_hdmi.c	2011-11-15 21:13:42.000000000 +0800
> @@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
>  	 * the unsolicited response to avoid custom WARs.
>  	 */
>  	int present = snd_hda_pin_sense(codec, pin_nid);
> +	bool eld_valid = false;
>  
> -	memset(eld, 0, sizeof(*eld));
> +	eld->eld_valid = false;
>  
>  	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
>  	if (eld->monitor_present)
> -		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
> -	else
> -		eld->eld_valid	= 0;
> +		eld_valid	= !!(present & AC_PINSENSE_ELDV);
>  
>  	printk(KERN_INFO
>  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
> -		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
> +		codec->addr, pin_nid, eld->monitor_present, eld_valid);
>  
> -	if (eld->eld_valid)
> +	if (eld_valid)
>  		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
>  			snd_hdmi_show_eld(eld);
>  
> 

Re: [PATCH 1/2] hda - fix ELD memory leak

[Wu Fengguang]

On Tue, Nov 15, 2011 at 10:35:41PM +0800, Takashi Iwai wrote:
> At Tue, 15 Nov 2011 22:31:55 +0800,
> Wu Fengguang wrote:
> > 
> > memset(eld) clears eld->proc_entry which will leak the struct
> > snd_info_entry when unloading the module.
> > 
> > Fix it by
> > - remove memset(eld)
> > - set eld->eld_valid to true _after_ all eld fields have been filled
> > - don't access the other eld fields when (eld->eld_valid == false)
> > 
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> 
> This should be send to stable kernel, too, right?
> It appears in 3.1, at least.

Yeah. Good point!  I'll resend it when everything goes fine.

Thanks,
Fengguang

> > ---
> >  sound/pci/hda/hda_eld.c    |    5 +----
> >  sound/pci/hda/patch_hdmi.c |   11 +++++------
> >  2 files changed, 6 insertions(+), 10 deletions(-)
> > 
> > --- linux.orig/sound/pci/hda/hda_eld.c	2011-11-15 21:02:25.000000000 +0800
> > +++ linux/sound/pci/hda/hda_eld.c	2011-11-15 21:13:46.000000000 +0800
> > @@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
> >  					buf + ELD_FIXED_BYTES + mnl + 3 * i);
> >  	}
> >  
> > +	e->eld_valid = true;
> >  	return 0;
> >  
> >  out_fail:
> > -	e->eld_ver = 0;
> >  	return -EINVAL;
> >  }
> >  
> > @@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
> >  	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
> >  	 */
> >  
> > -	if (!eld->eld_valid)
> > -		return -ENOENT;
> > -
> >  	size = snd_hdmi_get_eld_size(codec, nid);
> >  	if (size == 0) {
> >  		/* wfg: workaround for ASUS P5E-VM HDMI board */
> > --- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:02:25.000000000 +0800
> > +++ linux/sound/pci/hda/patch_hdmi.c	2011-11-15 21:13:42.000000000 +0800
> > @@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
> >  	 * the unsolicited response to avoid custom WARs.
> >  	 */
> >  	int present = snd_hda_pin_sense(codec, pin_nid);
> > +	bool eld_valid = false;
> >  
> > -	memset(eld, 0, sizeof(*eld));
> > +	eld->eld_valid = false;
> >  
> >  	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
> >  	if (eld->monitor_present)
> > -		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
> > -	else
> > -		eld->eld_valid	= 0;
> > +		eld_valid	= !!(present & AC_PINSENSE_ELDV);
> >  
> >  	printk(KERN_INFO
> >  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
> > -		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
> > +		codec->addr, pin_nid, eld->monitor_present, eld_valid);
> >  
> > -	if (eld->eld_valid)
> > +	if (eld_valid)
> >  		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
> >  			snd_hdmi_show_eld(eld);
> >  
> > 

Re: [PATCH 1/2] hda - fix ELD memory leak

[Takashi Iwai]

At Tue, 15 Nov 2011 22:41:16 +0800,
Wu Fengguang wrote:
> 
> On Tue, Nov 15, 2011 at 10:35:41PM +0800, Takashi Iwai wrote:
> > At Tue, 15 Nov 2011 22:31:55 +0800,
> > Wu Fengguang wrote:
> > > 
> > > memset(eld) clears eld->proc_entry which will leak the struct
> > > snd_info_entry when unloading the module.
> > > 
> > > Fix it by
> > > - remove memset(eld)
> > > - set eld->eld_valid to true _after_ all eld fields have been filled
> > > - don't access the other eld fields when (eld->eld_valid == false)
> > > 
> > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > 
> > This should be send to stable kernel, too, right?
> > It appears in 3.1, at least.
> 
> Yeah. Good point!  I'll resend it when everything goes fine.

Ah, no, you don't need to resend.  I'd just need to put Cc to stable
in the patch commit log.  Then Greg will pick it up automatically
when the tree is merged to the upstream.

But of course I need to know beforehand whether the patch is intended
to be sent to stable or not.


Takashi

> 
> Thanks,
> Fengguang
> 
> > > ---
> > >  sound/pci/hda/hda_eld.c    |    5 +----
> > >  sound/pci/hda/patch_hdmi.c |   11 +++++------
> > >  2 files changed, 6 insertions(+), 10 deletions(-)
> > > 
> > > --- linux.orig/sound/pci/hda/hda_eld.c	2011-11-15 21:02:25.000000000 +0800
> > > +++ linux/sound/pci/hda/hda_eld.c	2011-11-15 21:13:46.000000000 +0800
> > > @@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
> > >  					buf + ELD_FIXED_BYTES + mnl + 3 * i);
> > >  	}
> > >  
> > > +	e->eld_valid = true;
> > >  	return 0;
> > >  
> > >  out_fail:
> > > -	e->eld_ver = 0;
> > >  	return -EINVAL;
> > >  }
> > >  
> > > @@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
> > >  	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
> > >  	 */
> > >  
> > > -	if (!eld->eld_valid)
> > > -		return -ENOENT;
> > > -
> > >  	size = snd_hdmi_get_eld_size(codec, nid);
> > >  	if (size == 0) {
> > >  		/* wfg: workaround for ASUS P5E-VM HDMI board */
> > > --- linux.orig/sound/pci/hda/patch_hdmi.c	2011-11-15 21:02:25.000000000 +0800
> > > +++ linux/sound/pci/hda/patch_hdmi.c	2011-11-15 21:13:42.000000000 +0800
> > > @@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
> > >  	 * the unsolicited response to avoid custom WARs.
> > >  	 */
> > >  	int present = snd_hda_pin_sense(codec, pin_nid);
> > > +	bool eld_valid = false;
> > >  
> > > -	memset(eld, 0, sizeof(*eld));
> > > +	eld->eld_valid = false;
> > >  
> > >  	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
> > >  	if (eld->monitor_present)
> > > -		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
> > > -	else
> > > -		eld->eld_valid	= 0;
> > > +		eld_valid	= !!(present & AC_PINSENSE_ELDV);
> > >  
> > >  	printk(KERN_INFO
> > >  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
> > > -		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
> > > +		codec->addr, pin_nid, eld->monitor_present, eld_valid);
> > >  
> > > -	if (eld->eld_valid)
> > > +	if (eld_valid)
> > >  		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
> > >  			snd_hdmi_show_eld(eld);
> > >  
> > > 
> 

Re: [PATCH 1/2] hda - fix ELD memory leak

[Wu Fengguang]

On Tue, Nov 15, 2011 at 10:45:15PM +0800, Takashi Iwai wrote:
> At Tue, 15 Nov 2011 22:41:16 +0800,
> Wu Fengguang wrote:
> > 
> > On Tue, Nov 15, 2011 at 10:35:41PM +0800, Takashi Iwai wrote:
> > > At Tue, 15 Nov 2011 22:31:55 +0800,
> > > Wu Fengguang wrote:
> > > > 
> > > > memset(eld) clears eld->proc_entry which will leak the struct
> > > > snd_info_entry when unloading the module.
> > > > 
> > > > Fix it by
> > > > - remove memset(eld)
> > > > - set eld->eld_valid to true _after_ all eld fields have been filled
> > > > - don't access the other eld fields when (eld->eld_valid == false)
> > > > 
> > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > > 
> > > This should be send to stable kernel, too, right?
> > > It appears in 3.1, at least.
> > 
> > Yeah. Good point!  I'll resend it when everything goes fine.
> 
> Ah, no, you don't need to resend.  I'd just need to put Cc to stable
> in the patch commit log.  Then Greg will pick it up automatically
> when the tree is merged to the upstream.

Ah OK, that would be convenient.

> But of course I need to know beforehand whether the patch is intended
> to be sent to stable or not.

Ack. The patch applies cleanly to 3.1.

Thanks,
Fengguang

Re: [PATCH 1/2] hda - fix ELD memory leak

[Stephen Warren]

Wu Fengguang wrote at Tuesday, November 15, 2011 7:32 AM:
> memset(eld) clears eld->proc_entry which will leak the struct
> snd_info_entry when unloading the module.
>
> Fix it by
> - remove memset(eld)
> - set eld->eld_valid to true _after_ all eld fields have been filled
> - don't access the other eld fields when (eld->eld_valid == false)
>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic