Fix wrong assumptions in cea_for_each_detailed_block v2

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.

diff -ur linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c linux-3.2-rc1/drivers/gpu/drm/drm_edid.c
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Good catch, thanks.

- ajax

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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>

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

-JimC

James Cloos <[email protected]>         OpenPGP: 1024D/ED7DAEA6


CS> _______________________________________________
CS> dri-devel mailing list
CS> [email protected]
CS> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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.