[PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Andy Furniss]

Christian Schmidt wrote:

Tested with all three applied and they work well on my TV all modes 
work, which was not the case previously if I added manually the modes 
listed in xorg log (I have three bogus/not working modes logged by xorg).

I do have a small problem with the interlaced modes - I assumed this was 
a radeon driver issue (the audio part obviously is - but the top line 
part is independent of audio)

https://bugs.freedesktop.org/show_bug.cgi?id=35970

I mention it just in case there is some link to the cae spec interlaced 
timings interpretation/implementation.

[PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[James Cloos]

> "CS" == Christian Schmidt  writes:

CS> The current logic misunderstands the spec about CEA 18byte descriptors.
CS> First, the spec doesn't state "detailed timing descriptors" but "18 byte
CS> descriptors", so any data record could be stored, mixed timings and
CS> other data, just as in the standard EDID.
CS> Second, the lower four bit of byte 3 of the CEA record do not contain
CS> the number of descriptors, but "the total number of DTDs defining native
CS> formats in the whole EDID [...], starting with the first DTD in the DTD
CS> list (which starts in the base EDID block)." A device can of course
CS> support non-native formats.

CS> As such the number can't be used to determine n, and the existing code
CS> will filter non-timing 18byte descriptors anyway.

CS> V2 removes an unused variable warning.

CS> Signed-off-by: Christian Schmidt 

Tested-by: James Cloos 

Works fine here on top of Linus? 7f80850d3f9f with a Radeon HD 4290 and
an hdmi-connected tv.

-JimC
-- 
James Cloos  OpenPGP: 1024D/ED7DAEA6


CS> ___
CS> dri-devel mailing list
CS> dri-devel at lists.freedesktop.org
CS> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[James Cloos]

 CS == Christian Schmidt schm...@digadd.de writes:

CS The current logic misunderstands the spec about CEA 18byte descriptors.
CS First, the spec doesn't state detailed timing descriptors but 18 byte
CS descriptors, so any data record could be stored, mixed timings and
CS other data, just as in the standard EDID.
CS Second, the lower four bit of byte 3 of the CEA record do not contain
CS the number of descriptors, but the total number of DTDs defining native
CS formats in the whole EDID [...], starting with the first DTD in the DTD
CS list (which starts in the base EDID block). A device can of course
CS support non-native formats.

CS As such the number can't be used to determine n, and the existing code
CS will filter non-timing 18byte descriptors anyway.

CS V2 removes an unused variable warning.

CS Signed-off-by: Christian Schmidt schmidt@digadd,de

Tested-by: James Cloos cl...@jhcloos.com

Works fine here on top of Linus’ 7f80850d3f9f with a Radeon HD 4290 and
an hdmi-connected tv.

-JimC
-- 
James Cloos cl...@jhcloos.com OpenPGP: 1024D/ED7DAEA6


CS ___
CS dri-devel mailing list
CS dri-devel@lists.freedesktop.org
CS http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Andy Furniss]

Christian Schmidt wrote:

Tested with all three applied and they work well on my TV all modes 
work, which was not the case previously if I added manually the modes 
listed in xorg log (I have three bogus/not working modes logged by xorg).


I do have a small problem with the interlaced modes - I assumed this was 
a radeon driver issue (the audio part obviously is - but the top line 
part is independent of audio)


https://bugs.freedesktop.org/show_bug.cgi?id=35970

I mention it just in case there is some link to the cae spec interlaced 
timings interpretation/implementation.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Adam Jackson]

On Sun, 2011-11-13 at 09:57 +0100, Christian Schmidt wrote:
> The current logic misunderstands the spec about CEA 18byte descriptors.
> First, the spec doesn't state "detailed timing descriptors" but "18 byte
> descriptors", so any data record could be stored, mixed timings and
> other data, just as in the standard EDID.

I don't think the code misinterprets this.  But I also don't think your
patch changes this interpretation, so that's fine.

> Second, the lower four bit of byte 3 of the CEA record do not contain
> the number of descriptors, but "the total number of DTDs defining native
> formats in the whole EDID [...], starting with the first DTD in the DTD
> list (which starts in the base EDID block)." A device can of course
> support non-native formats.
> 
> As such the number can't be used to determine n, and the existing code
> will filter non-timing 18byte descriptors anyway.

Good catch, thanks.

Reviewed-by: Adam Jackson 

- ajax
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: 

Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Adam Jackson]

On Sun, 2011-11-13 at 09:57 +0100, Christian Schmidt wrote:
 The current logic misunderstands the spec about CEA 18byte descriptors.
 First, the spec doesn't state detailed timing descriptors but 18 byte
 descriptors, so any data record could be stored, mixed timings and
 other data, just as in the standard EDID.

I don't think the code misinterprets this.  But I also don't think your
patch changes this interpretation, so that's fine.

 Second, the lower four bit of byte 3 of the CEA record do not contain
 the number of descriptors, but the total number of DTDs defining native
 formats in the whole EDID [...], starting with the first DTD in the DTD
 list (which starts in the base EDID block). A device can of course
 support non-native formats.
 
 As such the number can't be used to determine n, and the existing code
 will filter non-timing 18byte descriptors anyway.

Good catch, thanks.

Reviewed-by: Adam Jackson a...@redhat.com

- ajax


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Christian Schmidt]

The current logic misunderstands the spec about CEA 18byte descriptors.
First, the spec doesn't state "detailed timing descriptors" but "18 byte
descriptors", so any data record could be stored, mixed timings and
other data, just as in the standard EDID.
Second, the lower four bit of byte 3 of the CEA record do not contain
the number of descriptors, but "the total number of DTDs defining native
formats in the whole EDID [...], starting with the first DTD in the DTD
list (which starts in the base EDID block)." A device can of course
support non-native formats.

As such the number can't be used to determine n, and the existing code
will filter non-timing 18byte descriptors anyway.

V2 removes an unused variable warning.

Signed-off-by: Christian Schmidt 

-- next part --
A non-text attachment was scrubbed...
Name: fix_cea_for_each_detailed_block.patch
Type: text/x-patch
Size: 884 bytes
Desc: not available
URL: 

[PATCH] Fix wrong assumptions in cea_for_each_detailed_block v2

[Christian Schmidt]

The current logic misunderstands the spec about CEA 18byte descriptors.
First, the spec doesn't state detailed timing descriptors but 18 byte
descriptors, so any data record could be stored, mixed timings and
other data, just as in the standard EDID.
Second, the lower four bit of byte 3 of the CEA record do not contain
the number of descriptors, but the total number of DTDs defining native
formats in the whole EDID [...], starting with the first DTD in the DTD
list (which starts in the base EDID block). A device can of course
support non-native formats.

As such the number can't be used to determine n, and the existing code
will filter non-timing 18byte descriptors anyway.

V2 removes an unused variable warning.

Signed-off-by: Christian Schmidt schmidt@digadd,de

diff -ur linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c linux-3.2-rc1/drivers/gpu/drm/drm_edid.c
--- linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c	2011-11-13 09:51:21.722124401 +0100
+++ linux-3.2-rc1/drivers/gpu/drm/drm_edid.c	2011-11-13 09:54:47.399553081 +0100
@@ -508,25 +508,10 @@
 cea_for_each_detailed_block(u8 *ext, detailed_cb *cb, void *closure)
 {
 	int i, n = 0;
-	u8 rev = ext[0x01], d = ext[0x02];
+	u8 d = ext[0x02];
 	u8 *det_base = ext + d;
 
-	switch (rev) {
-	case 0:
-		/* can't happen */
-		return;
-	case 1:
-		/* have to infer how many blocks we have, check pixel clock */
-		for (i = 0; i  6; i++)
-			if (det_base[18*i] || det_base[18*i+1])
-n++;
-		break;
-	default:
-		/* explicit count */
-		n = min(ext[0x03]  0x0f, 6);
-		break;
-	}
-
+	n = (127 - d) / 18;
 	for (i = 0; i  n; i++)
 		cb((struct detailed_timing *)(det_base + 18 * i), closure);
 }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel