* [PATCH] hda - fail ELD reading early
@ 2011-11-22 8:46 Wu Fengguang
2011-11-22 10:39 ` [alsa-devel] " Takashi Iwai
0 siblings, 1 reply; 5+ messages in thread
From: Wu Fengguang @ 2011-11-22 8:46 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org, LKML
With the ELD repoll mechanism, we can (and should) fail the ELD reading
immediately when find something obviously wrong and let the caller retry
after some delay.
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
--- linux.orig/sound/pci/hda/hda_eld.c 2011-11-22 16:02:58.000000000 +0800
+++ linux/sound/pci/hda/hda_eld.c 2011-11-22 16:36:10.000000000 +0800
@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
for (i = 0; i < size; i++) {
unsigned int val = hdmi_get_eld_data(codec, nid, i);
+ /*
+ * Graphics driver might be writing to ELD buffer right now.
+ * Just abort. The caller will repoll after a while.
+ */
if (!(val & AC_ELDD_ELD_VALID)) {
- if (!i) {
- snd_printd(KERN_INFO
- "HDMI: invalid ELD data\n");
- ret = -EINVAL;
- goto error;
- }
snd_printd(KERN_INFO
"HDMI: invalid ELD data byte %d\n", i);
- val = 0;
- } else
- val &= AC_ELDD_ELD_DATA;
+ ret = -EINVAL;
+ goto error;
+ }
+ val &= AC_ELDD_ELD_DATA;
+ /*
+ * The first byte cannot be zero. This can happen on some DVI
+ * connections. Some Intel chips may also need some 250ms delay
+ * to return non-zero ELD data, even when the graphics driver
+ * correctly writes ELD content before setting ELD_valid bit.
+ */
+ if (!val && !i) {
+ snd_printdd(KERN_INFO "HDMI: 0 ELD data\n");
+ ret = -EINVAL;
+ goto error;
+ }
buf[i] = val;
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [alsa-devel] [PATCH] hda - fail ELD reading early 2011-11-22 8:46 [PATCH] hda - fail ELD reading early Wu Fengguang @ 2011-11-22 10:39 ` Takashi Iwai 2011-11-22 10:53 ` Wu Fengguang 0 siblings, 1 reply; 5+ messages in thread From: Takashi Iwai @ 2011-11-22 10:39 UTC (permalink / raw) To: Wu Fengguang; +Cc: alsa-devel@alsa-project.org, LKML At Tue, 22 Nov 2011 16:46:23 +0800, Wu Fengguang wrote: > > With the ELD repoll mechanism, we can (and should) fail the ELD reading > immediately when find something obviously wrong and let the caller retry > after some delay. > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > --- > sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > --- linux.orig/sound/pci/hda/hda_eld.c 2011-11-22 16:02:58.000000000 +0800 > +++ linux/sound/pci/hda/hda_eld.c 2011-11-22 16:36:10.000000000 +0800 > @@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *el > > for (i = 0; i < size; i++) { > unsigned int val = hdmi_get_eld_data(codec, nid, i); > + /* > + * Graphics driver might be writing to ELD buffer right now. > + * Just abort. The caller will repoll after a while. > + */ > if (!(val & AC_ELDD_ELD_VALID)) { > - if (!i) { > - snd_printd(KERN_INFO > - "HDMI: invalid ELD data\n"); > - ret = -EINVAL; > - goto error; > - } > snd_printd(KERN_INFO > "HDMI: invalid ELD data byte %d\n", i); > - val = 0; > - } else > - val &= AC_ELDD_ELD_DATA; > + ret = -EINVAL; > + goto error; > + } > + val &= AC_ELDD_ELD_DATA; > + /* > + * The first byte cannot be zero. This can happen on some DVI > + * connections. Some Intel chips may also need some 250ms delay > + * to return non-zero ELD data, even when the graphics driver > + * correctly writes ELD content before setting ELD_valid bit. > + */ > + if (!val && !i) { > + snd_printdd(KERN_INFO "HDMI: 0 ELD data\n"); > + ret = -EINVAL; > + goto error; > + } Shouldn't this zero-check be before the valid-bit check? Otherwise it'll never reach there. thanks, Takashi ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] hda - fail ELD reading early 2011-11-22 10:39 ` [alsa-devel] " Takashi Iwai @ 2011-11-22 10:53 ` Wu Fengguang 2011-11-22 11:08 ` Takashi Iwai 0 siblings, 1 reply; 5+ messages in thread From: Wu Fengguang @ 2011-11-22 10:53 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org, LKML On Tue, Nov 22, 2011 at 06:39:53PM +0800, Takashi Iwai wrote: > At Tue, 22 Nov 2011 16:46:23 +0800, > Wu Fengguang wrote: > > > > With the ELD repoll mechanism, we can (and should) fail the ELD reading > > immediately when find something obviously wrong and let the caller retry > > after some delay. > > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > --- > > sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > --- linux.orig/sound/pci/hda/hda_eld.c 2011-11-22 16:02:58.000000000 +0800 > > +++ linux/sound/pci/hda/hda_eld.c 2011-11-22 16:36:10.000000000 +0800 > > @@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *el > > > > for (i = 0; i < size; i++) { > > unsigned int val = hdmi_get_eld_data(codec, nid, i); > > + /* > > + * Graphics driver might be writing to ELD buffer right now. > > + * Just abort. The caller will repoll after a while. > > + */ > > if (!(val & AC_ELDD_ELD_VALID)) { > > - if (!i) { > > - snd_printd(KERN_INFO > > - "HDMI: invalid ELD data\n"); > > - ret = -EINVAL; > > - goto error; > > - } > > snd_printd(KERN_INFO > > "HDMI: invalid ELD data byte %d\n", i); > > - val = 0; > > - } else > > - val &= AC_ELDD_ELD_DATA; > > + ret = -EINVAL; > > + goto error; > > + } > > + val &= AC_ELDD_ELD_DATA; > > + /* > > + * The first byte cannot be zero. This can happen on some DVI > > + * connections. Some Intel chips may also need some 250ms delay > > + * to return non-zero ELD data, even when the graphics driver > > + * correctly writes ELD content before setting ELD_valid bit. > > + */ > > + if (!val && !i) { > > + snd_printdd(KERN_INFO "HDMI: 0 ELD data\n"); > > + ret = -EINVAL; > > + goto error; > > + } > > Shouldn't this zero-check be before the valid-bit check? > Otherwise it'll never reach there. It does reach there: [ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 ==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data [ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI [ 1191.335020] HDMI: available speakers: FL/FR [ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24 The funny thing is, it's reporting (invalid) 0 ELD data that has the AC_ELDD_ELD_VALID bit set. Thanks, Fengguang ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] hda - fail ELD reading early 2011-11-22 10:53 ` Wu Fengguang @ 2011-11-22 11:08 ` Takashi Iwai 2011-11-22 11:20 ` Wu Fengguang 0 siblings, 1 reply; 5+ messages in thread From: Takashi Iwai @ 2011-11-22 11:08 UTC (permalink / raw) To: Wu Fengguang; +Cc: alsa-devel@alsa-project.org, LKML At Tue, 22 Nov 2011 18:53:18 +0800, Wu Fengguang wrote: > > On Tue, Nov 22, 2011 at 06:39:53PM +0800, Takashi Iwai wrote: > > At Tue, 22 Nov 2011 16:46:23 +0800, > > Wu Fengguang wrote: > > > > > > With the ELD repoll mechanism, we can (and should) fail the ELD reading > > > immediately when find something obviously wrong and let the caller retry > > > after some delay. > > > > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > > --- > > > sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- > > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > > > --- linux.orig/sound/pci/hda/hda_eld.c 2011-11-22 16:02:58.000000000 +0800 > > > +++ linux/sound/pci/hda/hda_eld.c 2011-11-22 16:36:10.000000000 +0800 > > > @@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *el > > > > > > for (i = 0; i < size; i++) { > > > unsigned int val = hdmi_get_eld_data(codec, nid, i); > > > + /* > > > + * Graphics driver might be writing to ELD buffer right now. > > > + * Just abort. The caller will repoll after a while. > > > + */ > > > if (!(val & AC_ELDD_ELD_VALID)) { > > > - if (!i) { > > > - snd_printd(KERN_INFO > > > - "HDMI: invalid ELD data\n"); > > > - ret = -EINVAL; > > > - goto error; > > > - } > > > snd_printd(KERN_INFO > > > "HDMI: invalid ELD data byte %d\n", i); > > > - val = 0; > > > - } else > > > - val &= AC_ELDD_ELD_DATA; > > > + ret = -EINVAL; > > > + goto error; > > > + } > > > + val &= AC_ELDD_ELD_DATA; > > > + /* > > > + * The first byte cannot be zero. This can happen on some DVI > > > + * connections. Some Intel chips may also need some 250ms delay > > > + * to return non-zero ELD data, even when the graphics driver > > > + * correctly writes ELD content before setting ELD_valid bit. > > > + */ > > > + if (!val && !i) { > > > + snd_printdd(KERN_INFO "HDMI: 0 ELD data\n"); > > > + ret = -EINVAL; > > > + goto error; > > > + } > > > > Shouldn't this zero-check be before the valid-bit check? > > Otherwise it'll never reach there. > > It does reach there: > > [ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > [ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > ==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data > [ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > [ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI > [ 1191.335020] HDMI: available speakers: FL/FR > [ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24 > > The funny thing is, it's reporting (invalid) 0 ELD data that has > the AC_ELDD_ELD_VALID bit set. Ah OK. Another slight concern is that your patch gives always the error when ELD_VALID isn't set, so it makes the check more strict in practice. I guess it'd be OK, so I'll take it in. But we need to carefully hear whether anyone cries with this. thanks, Takashi ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] hda - fail ELD reading early 2011-11-22 11:08 ` Takashi Iwai @ 2011-11-22 11:20 ` Wu Fengguang 0 siblings, 0 replies; 5+ messages in thread From: Wu Fengguang @ 2011-11-22 11:20 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org, LKML On Tue, Nov 22, 2011 at 07:08:59PM +0800, Takashi Iwai wrote: > At Tue, 22 Nov 2011 18:53:18 +0800, > Wu Fengguang wrote: > > > > On Tue, Nov 22, 2011 at 06:39:53PM +0800, Takashi Iwai wrote: > > > At Tue, 22 Nov 2011 16:46:23 +0800, > > > Wu Fengguang wrote: > > > > > > > > With the ELD repoll mechanism, we can (and should) fail the ELD reading > > > > immediately when find something obviously wrong and let the caller retry > > > > after some delay. > > > > > > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > > > --- > > > > sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- > > > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > > > > > --- linux.orig/sound/pci/hda/hda_eld.c 2011-11-22 16:02:58.000000000 +0800 > > > > +++ linux/sound/pci/hda/hda_eld.c 2011-11-22 16:36:10.000000000 +0800 > > > > @@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *el > > > > > > > > for (i = 0; i < size; i++) { > > > > unsigned int val = hdmi_get_eld_data(codec, nid, i); > > > > + /* > > > > + * Graphics driver might be writing to ELD buffer right now. > > > > + * Just abort. The caller will repoll after a while. > > > > + */ > > > > if (!(val & AC_ELDD_ELD_VALID)) { > > > > - if (!i) { > > > > - snd_printd(KERN_INFO > > > > - "HDMI: invalid ELD data\n"); > > > > - ret = -EINVAL; > > > > - goto error; > > > > - } > > > > snd_printd(KERN_INFO > > > > "HDMI: invalid ELD data byte %d\n", i); > > > > - val = 0; > > > > - } else > > > > - val &= AC_ELDD_ELD_DATA; > > > > + ret = -EINVAL; > > > > + goto error; > > > > + } > > > > + val &= AC_ELDD_ELD_DATA; > > > > + /* > > > > + * The first byte cannot be zero. This can happen on some DVI > > > > + * connections. Some Intel chips may also need some 250ms delay > > > > + * to return non-zero ELD data, even when the graphics driver > > > > + * correctly writes ELD content before setting ELD_valid bit. > > > > + */ > > > > + if (!val && !i) { > > > > + snd_printdd(KERN_INFO "HDMI: 0 ELD data\n"); > > > > + ret = -EINVAL; > > > > + goto error; > > > > + } > > > > > > Shouldn't this zero-check be before the valid-bit check? > > > Otherwise it'll never reach there. > > > > It does reach there: > > > > [ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > > [ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > > ==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data > > [ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 > > [ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI > > [ 1191.335020] HDMI: available speakers: FL/FR > > [ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24 > > > > The funny thing is, it's reporting (invalid) 0 ELD data that has > > the AC_ELDD_ELD_VALID bit set. > > Ah OK. Another slight concern is that your patch gives always the > error when ELD_VALID isn't set, so it makes the check more strict in > practice. I guess it'd be OK, so I'll take it in. But we need to > carefully hear whether anyone cries with this. Agreed, the original code simply sets data to 0 when !ELD_VALID. If the hardware does the illogical thing (who knows) of conditional setting !ELD_VALID on some parts of the ELD buffer which happen to contain 0, we'll be in bad luck... Thanks, Fengguang ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-22 11:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-22 8:46 [PATCH] hda - fail ELD reading early Wu Fengguang 2011-11-22 10:39 ` [alsa-devel] " Takashi Iwai 2011-11-22 10:53 ` Wu Fengguang 2011-11-22 11:08 ` Takashi Iwai 2011-11-22 11:20 ` Wu Fengguang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox