Bug 1490

Summary:

DDC code confused by EDID V1.3

Product:

xorg

Reporter:

Jay Cotton <jay.cotton>

Component:

Server/General

Assignee:

Egbert Eich <eich>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

high

CC:

alan.coopersmith, mharris, roland.mainz

Version:

6.8.1

Hardware:

All

OS:

Solaris

Whiteboard:

i915 platform:

i915 features:

Attachments:

Description	Flags
[FIXED_X11R68x] Fix	roland.mainz: 6.8-branch+
[FIXED_X11R68x] Patch to bring 6.8.2 branch in line with CVS head	roland.mainz: 6.8-branch+

Description Jay Cotton 2004-09-28 21:44:54 UTC

Flat panel monitors that return EDID V1.3 don't get the h/v sync they need. 

The data in the monitor's EDID
PROM for the Standard Timing Identificaiton slot #0 (byte 26) is
(interpretation based on page 17 of EDID spec):

  byte 0: 0xd1: 0xd1 = 209 => (209 + 31) * 8 = 1920  (horizontal size)
  byte 1: 0x00: 00b => 16:10 aspect ratio => 1920 * 10 / 16 = 1200 (vert size)
		0000000b => refresh rate of 0 + 60 => 60Hz

Which is correct.

The sticking point, I think, is how to interpret the 00b in byte 1.  In
EDID versions prior to 1.3, it indicated an aspect ratio of 1:1.  In
EDID 1.3 and higher, it indicates an aspect ratio of 16:10. 

-----

I'll take this bug, since I think I know how to fix it.

Comment 1 Jay Cotton 2004-10-14 17:09:32 UTC

Egbert is the original author of the code in question.

Comment 2 Egbert Eich 2004-10-18 02:24:03 UTC

Created attachment 1122 [details] [review]
[FIXED_X11R68x] Fix

Please test this (I haven't gotten around to it yet) and report your findings.
I will commit it to freedesktop. It's on my TODO list.

Comment 3 Jay Cotton 2004-10-20 17:54:49 UTC

New code does not produce better results on nv driver.  I will now go to
NV and see what it up there.

Comment 4 Jay Cotton 2004-10-20 18:07:46 UTC

New code does not produce better results on nv driver.  I will now go to
NV and see what it up there.

(II) NV(0): Probing for EDID on I2C bus A...
(II) NV(0): I2C device "DDC:ddc2" registered at address 0xA0.
(II) NV(0): I2C device "DDC:ddc2" removed.
(--) NV(0): DDC detected a DFP:
(II) NV(0): Manufacturer: SUN  Model: 586  Serial#: 808464432
(II) NV(0): Year: 2004  Week: 26
(II) NV(0): EDID Version: 1.3
(II) NV(0): Digital Display Input
(II) NV(0): Max H-Image Size [cm]: horiz.: 51  vert.: 32
(II) NV(0): Gamma: 2.50
(II) NV(0): DPMS capabilities: Off; RGB/Color Display
(II) NV(0): First detailed timing is preferred mode
(II) NV(0): redX: 0.633 redY: 0.353   greenX: 0.295 greenY: 0.596
(II) NV(0): blueX: 0.141 blueY: 0.082   whiteX: 0.318 whiteY: 0.348
(II) NV(0): Supported VESA Video Modes:
(II) NV(0): 720x400@70Hz
(II) NV(0): 640x480@60Hz
(II) NV(0): 640x480@67Hz
(II) NV(0): 640x480@72Hz
(II) NV(0): 640x480@75Hz
(II) NV(0): 800x600@56Hz
(II) NV(0): 800x600@60Hz
(II) NV(0): 800x600@72Hz
(II) NV(0): 800x600@75Hz
(II) NV(0): 832x624@75Hz
(II) NV(0): 1024x768@60Hz
(II) NV(0): 1024x768@70Hz
(II) NV(0): 1024x768@75Hz
(II) NV(0): 1280x1024@75Hz
(II) NV(0): 1152x870@75Hz
(II) NV(0): Manufacturer's mask: 0
(II) NV(0): Supported Future Video Modes:
(II) NV(0): #0: hsize: 1920  vsize 1920  refresh: 60  vid: 209
-----------------------------------^^^^ this needs to be 1200.

(II) NV(0): #1: hsize: 1920  vsize 1080  refresh: 60  vid: 49361
(II) NV(0): #2: hsize: 1280  vsize 1024  refresh: 60  vid: 32897
(II) NV(0): #3: hsize: 1280  vsize 1024  refresh: 76  vid: 36993
(II) NV(0): #4: hsize: 1600  vsize 1200  refresh: 60  vid: 16553
(II) NV(0): #5: hsize: 1152  vsize 921  refresh: 66  vid: 34417

Comment 5 Alan Coopersmith 2004-10-21 10:43:59 UTC

+  case RATIO1_1: y = _HSIZE1(x) * ((v->version>1||v->revision>2) ? 10/16 : 1); \
+                                         break; \

Will that work in integers?  Won't it precompute 10/16 as 0?

Would this work better?
+  case RATIO1_1: y = ((v->version>1||v->revision>2) ? (_HSIZE1(x) * 10)/16 :
_HSIZE1(x)); \
+                                         break; \

Comment 6 Jay Cotton 2004-10-21 11:35:20 UTC

Alans fix gets us much closer.  

Looks like we lost mode line #0

(II) NV(0): Supported Future Video Modes:
(II) NV(0): #1: hsize: 1920  vsize 1080  refresh: 60  vid: 49361
(II) NV(0): #2: hsize: 1280  vsize 1024  refresh: 60  vid: 32897
(II) NV(0): #3: hsize: 1280  vsize 1024  refresh: 76  vid: 36993
(II) NV(0): #4: hsize: 1600  vsize 1200  refresh: 60  vid: 16553
(II) NV(0): #5: hsize: 1152  vsize 921  refresh: 66  vid: 34417

Comment 7 Jay Cotton 2004-10-21 13:58:57 UTC

With Alans fix and removeing the VALID_TIMING macro, I can get the correct
'Supported Future Video Modes' info.  

So, 
#define _VALID_TIMING(x) ((x[0] != 0x01 && x[1] != 0x01) \
                       && (x[0] != 0x00 && x[1] != 0x00) \
                       && (x[0] != 0x20 && x[1] != 0x20) )
is catching the 209 dec. value.

Still drilling.

Comment 8 Jay Cotton 2004-10-21 14:38:33 UTC

O.k.  WIth the following 2 changes to edid.h, I was able to get the correct
output.

#define _VALID_TIMING(x) (((x[0] != 0x01) && (x[1] != 0x01)) \
                       || ((x[0] != 0x00) && (x[1] != 0x00)) \
                       || ((x[0] != 0x20) && (x[1] != 0x20)) )

and
#define _VSIZE1(x,y,r) switch(RATIO(x)){ \
  case RATIO1_1: y = ((v->version>1||v->revision>2) ? ((_HSIZE1(x)*10)/16) :
_HSIZE1(x)); \
                                         break; \

With these fixes in, the code works correctly, more testing is needed.

Comment 9 Egbert Eich 2004-10-22 06:10:00 UTC

Yes, you're right. The other lines which do '* 3 / 4' do the multiplication
first, then the division. I didn't pay any attention to the type of the
variables. Stupid me.

Comment 10 Egbert Eich 2004-10-22 06:22:27 UTC

#define _VALID_TIMING(x) (((x[0] != 0x01) && (x[1] != 0x01)) \
                       || ((x[0] != 0x00) && (x[1] != 0x00)) \
                       || ((x[0] != 0x20) && (x[1] != 0x20)) )
is still wrong.
It sould be valid unless (x0==1 and x1==1) or (x0==0 and x1==0) or
(x0==0x20 and x1==0x20):

#define _VALID_TIMING(x) !(((x[0] == 0x01) && (x[1] == 0x01)) \
                       || ((x[0] == 0x00) && (x[1] == 0x00)) \
                       || ((x[0] == 0x20) && (x[1] == 0x20)) )
or
#define _VALID_TIMING(x) (!((x[0] == 0x01) && (x[1] == 0x01)) \
                       && !((x[0] == 0x00) && (x[1] == 0x00)) \
                       && !((x[0] == 0x20) && (x[1] == 0x20)) )
or 
#define _VALID_TIMING(x) (((x[0] != 0x01) || (x[1] != 0x01)) \
                       && ((x[0] != 0x00) || (x[1] != 0x00)) \
                       && ((x[0] != 0x20) || (x[1] != 0x20)) )

Could you please try this?

Comment 11 Jay Cotton 2004-10-22 14:20:02 UTC

#define _VALID_TIMING(x) !(((x[0] == 0x01) && (x[1] == 0x01)) \
                       || ((x[0] == 0x00) && (x[1] == 0x00)) \
                       || ((x[0] == 0x20) && (x[1] == 0x20)) )

This one works, and is easyest to read....

Comment 12 Egbert Eich 2004-11-02 00:55:57 UTC

Jay, thanks for your help and all the testing. I've just committed the code.
Please update from CVS head. If problems arise please reopen.

Comment 13 Egbert Eich 2004-12-13 10:24:40 UTC

Comment on attachment 1122 [details] [review]
[FIXED_X11R68x] Fix

Thomas Winischhofer suggested limit support of DFP to EDID >=v1.2 instead of >
v1.2. However the 'VESA ENHANCED EXTENDED DISPLAY IDENTIFICATION DATA  
Implementation Guide' suggests to limit this the way it is currently
implemented.

Comment 14 Egbert Eich 2004-12-14 09:13:49 UTC

The issue in the last comment got resolved in a conversation on release-wranglers.

Comment 15 Roland Mainz 2004-12-14 11:50:24 UTC

Comment on attachment 1122 [details] [review]
[FIXED_X11R68x] Fix

Approval for X11R6.8.x stable branch granted in the 2004-12-13
release-wranglers phone call.

Comment 16 Roland Mainz 2004-12-16 17:28:38 UTC

Comment on attachment 1122 [details] [review]
[FIXED_X11R68x] Fix

Patch checked-in into X11R6.8.x stable branch:

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.365.2.105; previous revision: 1.365.2.104
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
/cvs/xorg/xc/programs/Xserver/hw/xfree86/ddc/edid.h,v  <--  edid.h
new revision: 1.2.4.1; previous revision: 1.2
/cvs/xorg/xc/programs/Xserver/hw/xfree86/ddc/interpret_edid.c,v  <-- 
interpret_edid.c
new revision: 1.3.2.1; previous revision: 1.3
/cvs/xorg/xc/programs/Xserver/hw/xfree86/ddc/print_edid.c,v  <--  print_edid.c
new revision: 1.2.4.1; previous revision: 1.2
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
Mailing the commit message to xorg-commit@lists.freedesktop.org...

Comment 17 Alan Coopersmith 2004-12-20 17:48:14 UTC

Created attachment 1576 [details] [review]
[FIXED_X11R68x] Patch to bring 6.8.2 branch in line with CVS head

Sorry - I did not realize the patch as attached in bugzilla did not include the
final two changes discussed in the comments.   This additional patch, when
applied on top of the current 6.8.2 branch brings it in line with the correct
code as integrated into HEAD branch.

Comment 18 Alan Coopersmith 2004-12-20 17:50:09 UTC

Comment on attachment 1576 [details] [review]
[FIXED_X11R68x] Patch to bring 6.8.2 branch in line with CVS head

Requesting approval for 6.8.2 branch since incomplete patch was provided in
bugzilla missing two small changes that went into the commit to CVS HEAD.

Comment 19 Roland Mainz 2005-01-12 15:59:23 UTC

Comment on attachment 1576 [details] [review]
[FIXED_X11R68x] Patch to bring 6.8.2 branch in line with CVS head

Approved for commit into X11R6.8.x stable branch in the 2005-01-10 Xorg
release-wranglers phone call.

Comment 20 Roland Mainz 2005-01-12 16:04:41 UTC

Comment on attachment 1576 [details] [review]
[FIXED_X11R68x] Patch to bring 6.8.2 branch in line with CVS head

Patch checked-in into X11R6.8.x stable branch:

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.365.2.117; previous revision: 1.365.2.116
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
/cvs/xorg/xc/programs/Xserver/hw/xfree86/ddc/edid.h,v  <--  edid.h
new revision: 1.2.4.2; previous revision: 1.2.4.1
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
Mailing the commit message to xorg-commit@lists.freedesktop.org...

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.