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 (!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;} elseval &= 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