[alsa-devel] [PATCH 1/2] hda - fix ELD memory leak
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);
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);
- 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));}- }
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);
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
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); }
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?
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
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.
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
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.
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
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 = 1OK, 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
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
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);
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);
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);
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
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
participants (3)
-
Stephen Warren -
Takashi Iwai -
Wu Fengguang