Bug 5892

Summary: Improvement of auto-config by using more EDID data in mode validation process
Product: xorg Reporter: henry.zhao <henry.zhao>
Component: Server/GeneralAssignee: henry.zhao <henry.zhao>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: high CC: alan.coopersmith, alexdeucher, libv, stuart.kreitman
Version: gitKeywords: patch
Hardware: x86 (IA32)   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description henry.zhao@oracle.com 2006-02-15 08:33:21 UTC
Xorg server does not start at a satisfactory mode without a config file.

Current use of Use of monitor probed data (or EDID data), if available, is only 
limited to deriving min/max vertical and horizontal rates:

* Mode names defined in standard and detailed timing sectors are not used. 
  During auto-config, the most satisfactory mode (mode with largest mode size 
  the monitor supports) is not guaranteed.

* Detailed timing data for modes the monitor supports is not used. Some monitors 
  use timing data that deviate from those defined in default database 
  xf86DefaultModes[]. During auto-config, these privately defined EDID timing 
  data can not be taken advantage of.

Besides, the current server Solely depend on drivers to do monitor probing. 
Most drivers use DDC2 probing which has an advantage of being able to probe
separate monitors when multiple monitors are connected to a single graphics 
card. However it sometimes fails on some CRT monitors and vga port connected 
LCD monitors.
Comment 1 henry.zhao@oracle.com 2006-02-15 09:13:23 UTC
1 Goals

  (a) During auto-config, the system should start at a best mode the 
      monitor and graphics device supports with appropriate timing.

  (b) Use fallback monitor probing when probing from driver side
      fails.

2 Fallback VBE DDC probing

  If monitor probing fails from driver side, no EDID data exits, i.e.
  scrp->monitor->DDC is NULL, invoke VBE DDC probing vbeProbeDDC()
  to try to get EDID data. vbeProbeDDC() is a new function created
  in libvbe.so. Using vbeProbeDDC() maximizes the success rate of 
  probing.

3 Creation of EDID modes and EDID modelines

  Parse EDID data and build:

  (a) EDID modes, a list of mode names the monitor supports.
      The list is built based on mode names defined in 

         Detailed timing sector; if none then
         Standard timing sector, if none then
         Standard timing 2 sector

         (Some old CRT monitor such as ViewSonic P815 only provides 
         standard timing 2 sector)

      The list is sorted at a descending order.

  (b) EDID modelines with type M_T_EDID, based on the timing data in 
      detailed timing sector. 

4 Use of EDID modes in validation list

  If there is no config file, or there is one, but no "Modes" is 
  defined, prepend EDID modes to validation list the same way
  modes on "Modes" line are added to the list when there is
  a config file with "Modes" line. This ensures that the largest 
  EDID mode is on top of validation list, and will be selected if it 
  passes validation test.  If no EDID modes are available (this 
  happens when both driver and fallback probing fails), use default 
  mode "1024x768".

5 Use of EDID modelines

  Prepend EDID modelines in the process of building Modeline database 
  in scrp->modePool. The prepended EDID modelines are also 
  subject to driver and monitor timing tests. Modes of type 
  M_T_EDID are treated with the same priority as M_T_BUILDTIN 
  therefore in the validation process, once a modeline with type 
  M_T_EDID is found, it is used and no more further search is needed. 
  This ensures the use of EDID modeline if it passes validation test.

  Before an EDID modeline is prepended, "availModes" is searched to
  see if there exits any user defined or built-in modeline with the 
  same mode name.  The EDID modeline will not be prepended if there 
  exits such a modeline. This ensures EDID modelines to be overridden 
  by user defined or built-in modeline. In the improvement work, to 
  avoid type confusion with nvidia driver defined EDID modeline 
  the type of user defined modelines has been changed to M_T_USERDEF.

6 Simplification of validation process

  Validation process is simplified by removing redundancy of modes 
  with same mode name on validation list. In extracting mode names
  from Modeline database to form the second part of validation list, 
  entries with mode names that are already on validation list are 
  not considered. This results in great simplicity and efficiency.

  Mode validation related messages are enhanced, and new messages
  are created  to provide more information and make themselves
  more understandable.

7 Topic for discussion

  The proposed validation process improvement is EDID data oriented, it
  maximizes the chance of starting server at an appropriate mode, 
  specially when a monitor needs a private modeline that deviates from,
  or doesn't exist in, default modeline database. However at the same time 
  the improvement mostly disable the the use of "strategy lookup" method 
  that used to be the principal algorithm for the process. To be exactly, 
  with the improvement, "strategy lookup" works only when user defines 
  a mode that is not covered in EDID data, or there is no EDID data. 
  However, sometimes the timing given in EDID data is not the best in terms 
  of the refresh rate. Do we need to invent an option flag that revokes the 
  privilege of EDID modeline so that "strategy lookup" can apply ? This
  is a topic for discussion.
Comment 2 Luc Verhaegen 2006-02-15 11:51:15 UTC
Erm, isn't this a duplicate of #5386?
Comment 3 henry.zhao@oracle.com 2006-02-15 12:10:48 UTC
No. It is not a duplicate of #5386. While #5386 parses EDID data and saves the
derived mode data at a well known location (pScrn->monitor->Modes), it does not 
address the topic of how these mode data will be used. This bug makes a 
recommended use of EDID data (both EDID modes, and EDID modelines) to improve
auto-configuration.
Comment 4 Luc Verhaegen 2006-02-15 12:19:30 UTC
5386 _is_ about that. I just haven't gotten my code as far yet. 2 direct loops
to jump through still, and i have quite verbosely been describing what was needed.

The second of those direct loops is a rewrite of the handling of modes, and a
cleanup of dotclock handling is a welcome change before that. This requires
cleaning up drivers, which takes time, as always.
Comment 5 henry.zhao@oracle.com 2006-02-15 13:19:13 UTC
I am submitting code changes for a review. The changes are based on the current
CVS (not on top of changes of #5386). In the future the changes for the two bugs
can be merged:

------- xf86str.h -------
*** /tmp/sccs.9XaO_v    Tue Jan 31 12:16:16 2006
--- xf86str.h   Thu Jan 26 20:02:22 2006
***************
*** 126,131 ****
--- 126,132 ----
                                 /* built-in mode - configure CRTC and clock */
  # define M_T_DEFAULT 0x10     /* (VESA) default modes */
  # define M_T_USERDEF 0x20     /* One of the modes from the config file */
+ # define M_T_EDID    0x40       /* Mode from probed EDID data section */
  
  /* Video mode */
  typedef struct _DisplayModeRec {



------- xf86Priv.h -------
*** /tmp/sccs.kYaaaw    Tue Jan 31 12:16:16 2006
--- xf86Priv.h  Fri Jan 13 19:56:19 2006
***************
*** 39,44 ****
--- 39,45 ----
  
  #include "xf86Privstr.h"
  #include "propertyst.h"
+ #include "xf86DDC.h"
  
  /*
   * Parameters set ONLY from the command line options
***************
*** 98,103 ****
--- 99,105 ----
  extern DriverPtr *xf86DriverList;
  extern ModuleInfoPtr *xf86ModuleInfoList;
  extern int xf86NumModuleInfos;
+ extern xf86MonPtr (*xf86FallbackDDCProbe)(int entityIndex, ScrnInfoPtr scrp);
  #else
  extern DriverPtr xf86DriverList[];
  #endif



------- xf86Config.c -------
*** /tmp/sccs.uYayaw    Tue Jan 31 12:16:16 2006
--- xf86Config.c        Fri Jan 27 10:09:19 2006
***************
*** 2212,2218 ****
      while( cmodep ) {
          mode = xnfalloc(sizeof(DisplayModeRec));
          memset(mode,'\0',sizeof(DisplayModeRec));
!       mode->type       = 0;
          mode->Clock      = cmodep->ml_clock;
          mode->HDisplay   = cmodep->ml_hdisplay;
          mode->HSyncStart = cmodep->ml_hsyncstart;
--- 2212,2218 ----
      while( cmodep ) {
          mode = xnfalloc(sizeof(DisplayModeRec));
          memset(mode,'\0',sizeof(DisplayModeRec));
!       mode->type       = M_T_USERDEF;
          mode->Clock      = cmodep->ml_clock;
          mode->HDisplay   = cmodep->ml_hdisplay;
          mode->HSyncStart = cmodep->ml_hsyncstart;



------- xf86Globals.c -------
*** /tmp/sccs.KYaWaw    Tue Jan 31 12:16:16 2006
--- xf86Globals.c       Fri Jan 13 19:53:16 2006
***************
*** 196,201 ****
--- 196,202 ----
  int xf86NumInputDrivers = 0;
  ModuleInfoPtr *xf86ModuleInfoList = NULL;
  int xf86NumModuleInfos = 0;
+ xf86MonPtr (*xf86FallbackDDCProbe) (int entityIndex, ScrnInfoPtr scrp) = NULL;
  #endif
  int xf86NumScreens = 0;
  



------- edid.h -------
*** /tmp/sccs.RdaWcw    Tue Jan 31 12:18:04 2006
--- edid.h      Thu Nov 17 13:09:17 2005
***************
*** 227,235 ****
  #define STEREO _STEREO(c)
  #define _STEREO1(x) (x[17] & 0x1)
  #define STEREO1 _STEREO(c)
! #define _SYNC_T(x) ((x[17] & 0x18) >> 4)
  #define SYNC_T _SYNC_T(c)
! #define _MISC(x) ((x[17] & 0x06) >> 2)
  #define MISC _MISC(c)
  
  #define _MONITOR_DESC_TYPE(x) x[3]
--- 227,235 ----
  #define STEREO _STEREO(c)
  #define _STEREO1(x) (x[17] & 0x1)
  #define STEREO1 _STEREO(c)
! #define _SYNC_T(x) ((x[17] & 0x18) >> 3)
  #define SYNC_T _SYNC_T(c)
! #define _MISC(x) ((x[17] & 0x06) >> 1)
  #define MISC _MISC(c)
  
  #define _MONITOR_DESC_TYPE(x) x[3]




------- vbe_module.c -------
*** /tmp/sccs.z7aqdw    Tue Jan 31 12:18:19 2006
--- vbe_module.c        Fri Jan 13 20:10:21 2006
***************
*** 6,11 ****
--- 6,12 ----
  
  #include "xf86.h"
  #include "xf86str.h"
+ #include "xf86Priv.h"
  #include "vbe.h"
  
  extern const char *vbe_ddcSymbols[];
***************
*** 30,35 ****
--- 31,55 ----
  
  XF86ModuleData vbeModuleData = { &vbeVersRec, vbeSetup, NULL };
  
+ static xf86MonPtr
+ vbeProbeDDC(int entityIndex, ScrnInfoPtr scrp)
+ {
+    vbeInfoPtr pVbe;
+    xf86MonPtr MonInfo = NULL;
+ 
+    pVbe = VBEInit(NULL, entityIndex);
+    MonInfo = vbeDoEDID(pVbe, NULL);
+    if (MonInfo) {
+         xf86DrvMsg(scrp->scrnIndex, X_PROBED,
+         "VBE DDC detected a %s:\n", MonInfo->features.input_type ?
+         "DFP" : "CRT");
+         xf86PrintEDID( MonInfo );
+         xf86DrvMsg(scrp->scrnIndex, X_INFO, "end of VBE DDC Monitor info\n\n");
+    }
+ 
+    return (MonInfo);
+ }
+ 
  static pointer
  vbeSetup(pointer module, pointer opts, int *errmaj, int *errmin)
  {
***************
*** 42,47 ****
--- 62,68 ----
         * Tell the loader about symbols from other modules that this module
         * might refer to.
         */
+       xf86FallbackDDCProbe = vbeProbeDDC;
      } 
      /*
       * The return value must be non-NULL on success even though there





------- xf86Mode.c -------
*** /tmp/sccs.TNaq3H    Tue Feb 14 16:36:11 2006
--- xf86Mode.c  Tue Feb 14 16:33:20 2006
***************
*** 49,54 ****
--- 49,57 ----
  #include "xf86Priv.h"
  #include "xf86DDC.h"
  
+ 
+ static void PrintModeline(int scrnIndex,DisplayModePtr mode);
+ 
  /*
   * xf86GetNearestClock --
   *    Find closest clock to given frequency (in kHz).  This assumes the
***************
*** 110,116 ****
      case MODE_BAD_WIDTH:
        return "width requires unsupported line pitch";
      case MODE_NOMODE:
!       return "no mode of this name";
      case MODE_NO_INTERLACE:
        return "interlace mode not supported";
      case MODE_NO_DBLESCAN:
--- 113,119 ----
      case MODE_BAD_WIDTH:
        return "width requires unsupported line pitch";
      case MODE_NOMODE:
!       return "no mode is found";
      case MODE_NO_INTERLACE:
        return "interlace mode not supported";
      case MODE_NO_DBLESCAN:
***************
*** 269,275 ****
        return p->status;
  
      /* Reject previously considered modes */
!     if (p->prev)
          return MODE_NOMODE;
  
      if ((p->type & M_T_CLOCK_C) == M_T_CLOCK_C) {
--- 272,278 ----
        return p->status;
  
      /* Reject previously considered modes */
!     if ((p->prev) && (p->type == M_T_BUILTIN))
          return MODE_NOMODE;
  
      if ((p->type & M_T_CLOCK_C) == M_T_CLOCK_C) {
***************
*** 426,432 ****
      int ModePrivFlags = 0;
      ModeStatus status = MODE_NOMODE;
      Bool allowDiv2 = (strategy & LOOKUP_CLKDIV2) != 0;
!     Bool haveBuiltin;
  
      strategy &= ~(LOOKUP_CLKDIV2 | LOOKUP_OPTIONAL_TOLERANCES);
  
--- 429,435 ----
      int ModePrivFlags = 0;
      ModeStatus status = MODE_NOMODE;
      Bool allowDiv2 = (strategy & LOOKUP_CLKDIV2) != 0;
!     Bool haveBuiltin, haveEdid;
  
      strategy &= ~(LOOKUP_CLKDIV2 | LOOKUP_OPTIONAL_TOLERANCES);
  
***************
*** 447,452 ****
--- 450,456 ----
      }
  
      haveBuiltin = FALSE;
+     haveEdid = FALSE;
      /* Scan the mode pool for matching names */
      for (p = scrp->modePool; p != NULL; p = p->next) {
        if (strcmp(p->name, modep->name) == 0) {
***************
*** 460,466 ****
                haveBuiltin = TRUE;
            }
  
!           if (haveBuiltin && !(p->type & M_T_BUILTIN))
                continue;
  
            /* Skip over previously rejected modes */
--- 464,475 ----
                haveBuiltin = TRUE;
            }
  
!           if (p->type & M_T_EDID) {
!               haveEdid = TRUE;
!           }
! 
!           if ((haveBuiltin && !(p->type & M_T_BUILTIN)) || 
!               (haveEdid && !(p->type & M_T_EDID) && (modep->type & M_T_EDID)))
                continue;
  
            /* Skip over previously rejected modes */
***************
*** 474,483 ****
            if (p->prev)
                continue;
  
!           if (p->type & M_T_BUILTIN) {
!               return xf86HandleBuiltinMode(scrp, p,modep, clockRanges,
                                             allowDiv2);
-           }
  
            /* Check clock is in range */
            cp = xf86FindClockRangeForMode(clockRanges, p);
--- 483,492 ----
            if (p->prev)
                continue;
  
!           if (p->type & (M_T_BUILTIN) ||
!               ((p->type & M_T_EDID) && (modep->type & M_T_EDID)))
!               return  xf86HandleBuiltinMode(scrp, p,modep, clockRanges,
                                             allowDiv2);
  
            /* Check clock is in range */
            cp = xf86FindClockRangeForMode(clockRanges, p);
***************
*** 649,655 ****
  static void
  xf86SetModeCrtc(DisplayModePtr p, int adjustFlags)
  {
!     if ((p == NULL) || ((p->type & M_T_CRTC_C) == M_T_BUILTIN))
        return;
  
      p->CrtcHDisplay             = p->HDisplay;
--- 658,664 ----
  static void
  xf86SetModeCrtc(DisplayModePtr p, int adjustFlags)
  {
!     if ((p == NULL) || (((p->type & M_T_CRTC_C) == M_T_BUILTIN) && (p->HSync)))
        return;
  
      p->CrtcHDisplay             = p->HDisplay;
***************
*** 1164,1169 ****
--- 1173,1199 ----
      return MODE_OK;
  }
  
+ static int
+ modestrcmp(const char **p1, const char **p2) 
+ {
+     char str1[16], str2[16], *q1, *q2;
+ 
+     strncpy(str1, *p1, 15); strncpy(str2, *p2, 15);
+     q1 = strchr(str1, 'x'); q2 = strchr(str2, 'x');
+     if (q1 && q2)
+       *q1 = *q2 = 0;
+     else
+       return 0;
+     if (!str1 || !(q1+1) || !str2 || (q2+1))
+       return 0;
+     if ((atoi(str1) * atoi(q1+1)) < (atoi(str2) * atoi(q2+1)))
+       return 1;
+     else if ((atoi(str1) * atoi(q1+1)) > (atoi(str2) * atoi(q2+1)))
+       return -1;
+ 
+     return 0;
+ }
+ 
  /*
   * xf86ValidateModes
   *
***************
*** 1226,1232 ****
      int newLinePitch, newVirtX, newVirtY;
      int modeSize;                                     /* in pixels */
      Bool validateAllDefaultModes = FALSE;
!     Bool userModes = FALSE;
      int saveType;
      PixmapFormatRec *BankFormat;
      ClockRangePtr cp;
--- 1256,1263 ----
      int newLinePitch, newVirtX, newVirtY;
      int modeSize;                                     /* in pixels */
      Bool validateAllDefaultModes = FALSE;
!     Bool userModes = TRUE;
!     Bool fallbackMode = FALSE;
      int saveType;
      PixmapFormatRec *BankFormat;
      ClockRangePtr cp;
***************
*** 1236,1241 ****
--- 1267,1281 ----
      int numTimings = 0;
      range hsync[MAX_HSYNC];
      range vrefresh[MAX_VREFRESH];
+     DisplayModeRec dt_modes[DET_TIMINGS];
+     char *dt_mode_names[DET_TIMINGS+1];
+     char *std_mode_names[DET_TIMINGS*5+1], *std2_mode_names[STD_TIMINGS+1];
+     char *fallback_mode[] = {"1024x768", NULL};
+     int numEdidModes;
+     int numDtModes = 0;
+     int numStdModes = 0;
+     int numStd2Modes = 0;
+     char **rmodeNames;
  
  #ifdef DEBUG
      ErrorF("xf86ValidateModes(%p, %p, %p, %p,\n\t\t  %p, %d, %d, %d, %d, %d,
%d, %d, %d, 0x%x)\n",
***************
*** 1266,1271 ****
--- 1306,1323 ----
      }
  
      /*
+      * If DDC is empty, try VBE DDC probing, since xf86DoEDID_DDC2
+      * probing, which at least nv and ati drivers count on, sometimes
+      * fails for some CRT monitors
+      */
+     if (!scrp->monitor->DDC) {
+       int entityIndex = scrp->entityList[0];
+ 
+       if ((xf86LoadSubModule(scrp, "vbe")) && xf86FallbackDDCProbe)
+               scrp->monitor->DDC = xf86FallbackDDCProbe (entityIndex, scrp);
+     }
+ 
+     /*
       * Probe monitor so that we can enforce/warn about its limits.
       * If one or more DS_RANGES descriptions are present, use the parameters
       * that they provide.  Otherwise, deduce limits based on the modes that
***************
*** 1281,1286 ****
--- 1333,1339 ----
        float h;
        struct std_timings *t;
        struct detailed_timings *dt;
+       char name[20];
  
        numTimings = 0;
        for (i = 0; i < DET_TIMINGS; i++) {
***************
*** 1311,1316 ****
--- 1364,1372 ----
                            hmin = h;
                        if (h > hmax)
                            hmax = h;
+                       sprintf(name, "%dx%d", t[j].hsize, t[j].vsize);
+                       std_mode_names[numStdModes++]   = xnfstrdup(name);
+                       std_mode_names[numStdModes] = NULL;
                    }
                }
                break;
***************
*** 1333,1338 ****
--- 1389,1431 ----
                        hmin = h;
                    if (h > hmax)
                        hmax = h;
+                   memset(&dt_modes[numDtModes], '\0', sizeof(DisplayModeRec));
+                   dt_modes[numDtModes].HSync  = 0.0;
+                   dt_modes[numDtModes].type       = M_T_EDID;
+                   dt_modes[numDtModes].Clock      = dt->clock/1000.0;
+                   dt_modes[numDtModes].HDisplay   = dt->h_active;
+                   dt_modes[numDtModes].HSyncStart = dt->h_sync_off + dt->h_active;
+                   dt_modes[numDtModes].HSyncEnd   = dt->h_sync_off + 
+                       dt->h_sync_width + dt->h_active;
+                   dt_modes[numDtModes].HTotal     = dt->h_active + dt->h_blanking;
+                   dt_modes[numDtModes].VDisplay   = dt->v_active;
+                   dt_modes[numDtModes].VSyncStart = dt->v_sync_off + dt->v_active;
+                   dt_modes[numDtModes].VSyncEnd   = dt->v_sync_off + 
+                       dt->v_sync_width + dt->v_active;
+                   dt_modes[numDtModes].VTotal     = dt->v_active + dt->v_blanking;
+                   if (dt->sync == 0x03)  {
+                       if (dt->misc & 0x02)
+                           dt_modes[numDtModes].Flags |= V_PVSYNC;
+                       else 
+                           dt_modes[numDtModes].Flags |= V_NVSYNC;
+                       if (dt->misc & 0x01)
+                           dt_modes[numDtModes].Flags |= V_PHSYNC;
+                       else 
+                           dt_modes[numDtModes].Flags |= V_NHSYNC;
+                   }
+                   if (dt->sync == 0x02) {
+                       dt_modes[numDtModes].Flags |= V_NHSYNC;
+                       if (dt->misc & 0x01)
+                           dt_modes[numDtModes].Flags |= V_PCSYNC;
+                       else 
+                           dt_modes[numDtModes].Flags |= V_NCSYNC;
+                   }
+                   if (dt->interlaced)
+                        dt_modes[numDtModes].Flags |= V_INTERLACE;
+                   sprintf(name, "%dx%d", dt->h_active, dt->v_active);
+                   dt_modes[numDtModes].name       = xnfstrdup(name);
+                   dt_mode_names[numDtModes++] = xnfstrdup(name);
+                   dt_mode_names[numDtModes] = NULL;
                }
                break;
            }
***************
*** 1341,1346 ****
--- 1434,1461 ----
                break;
        }
  
+       for (i = 0; i < STD_TIMINGS; i++) {
+           t = &DDC->timings2[i];
+           if (t->hsize > 256) { /* sanity check */
+               if (t->refresh < vmin)
+                   vmin = t->refresh;
+               if (t->refresh > vmax)
+                   vmax = t->refresh;
+               /*
+                * For typical modes this is a reasonable estimate
+                * of the horizontal sync rate.
+                */
+               h = t->refresh * 1.07 * t->vsize / 1000.0;
+               if (h < hmin)
+                   hmin = h;
+               if (h > hmax)
+                   hmax = h;
+               sprintf(name, "%dx%d", t->hsize, t->vsize);
+               std2_mode_names[numStd2Modes++] = xnfstrdup(name);
+               std2_mode_names[numStd2Modes] = NULL;
+           }
+       }
+ 
        if (numTimings == 0) {
            t = DDC->timings2;
            for (i = 0; i < STD_TIMINGS; i++) {
***************
*** 1443,1449 ****
                }
            } else {
                scrp->monitor->hsync[0].lo = 28;
!               scrp->monitor->hsync[0].hi = 33;
                scrp->monitor->nHsync = 1;
            }
            type = "default ";
--- 1558,1564 ----
                }
            } else {
                scrp->monitor->hsync[0].lo = 28;
!               scrp->monitor->hsync[0].hi = 60;
                scrp->monitor->nHsync = 1;
            }
            type = "default ";
***************
*** 1600,1607 ****
       * member of the scrp->modes list for which a match was considered.
       */
      if (scrp->modePool == NULL) {
        q = NULL;
!       for (p = availModes; p != NULL; p = p->next) {
            status = xf86InitialCheckModeForDriver(scrp, p, clockRanges,
                                                   strategy, maxPitch,
                                                   virtualX, virtualY);
--- 1715,1752 ----
       * member of the scrp->modes list for which a match was considered.
       */
      if (scrp->modePool == NULL) {
+       r = availModes;
        q = NULL;
!       for (i = 0;  ;  ) {
!           if (i < numDtModes) {
!               DisplayModePtr  s;
! 
!               p = &dt_modes[i];
!               for (s = availModes; s != NULL; s = s->next) {
!                  /* 
!                   * Don't prepend EDID mode if there exits a user-defined or 
!                   * built-in mode having the same name.
!                   */
!                  if (((s->type & M_T_USERDEF) || (s->type & M_T_BUILTIN)) && 
!                       (!strcmp(p->name, s->name)))
!                       break;
!               }
!               if (s) {
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO, "EDID Modeline \"%s\"
%.1fMHz not prepended to Modeline database:\n", 
!                       p->name, p->Clock/1000.0);
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO, " -- overridden by %s
Modeline \"%s\" %.1fMHz.\n", 
!                       (s->type|M_T_USERDEF) ? "user" : "builtin", s->name,
s->Clock/1000.0);
!                   i++;
!                   continue;
!               }
!               xf86DrvMsg(scrp->scrnIndex, X_INFO, "Prepend EDID Modeline
\"%s\" %.1fMHz to Modeline database\n",
!                   p->name, p->Clock/1000.0);
!           }
!           else {
!               if ((p = r) == NULL)
!                   break;
!           }
!           
            status = xf86InitialCheckModeForDriver(scrp, p, clockRanges,
                                                   strategy, maxPitch,
                                                   virtualX, virtualY);
***************
*** 1624,1642 ****
                q->name = xnfstrdup(p->name);
                q->status = MODE_OK;
            } else {
!               if (p->type & M_T_BUILTIN)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using built-in mode \"%s\" (%s)\n",
!                              p->name, xf86ModeStatusToString(status));
!               else if (p->type & M_T_DEFAULT)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using default mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
!               else
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
            }
        }
  
        if (scrp->modePool == NULL) {
--- 1769,1801 ----
                q->name = xnfstrdup(p->name);
                q->status = MODE_OK;
            } else {
!               char *typestring;
! 
!               switch (p->type) {
!               case M_T_BUILTIN:
!                   typestring = xnfstrdup("builtin");
!                   break;
!               case M_T_USERDEF:
!                   typestring = xnfstrdup("user");
!                   break;
!               case M_T_EDID:
!                   typestring = xnfstrdup("EDID");
!                   break;
!               case M_T_DEFAULT:
!                   typestring = xnfstrdup("default");
!                   break;
!               default:
!                   typestring = xnfstrdup("");
!                   break;
!               }
!               xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                   "Not including %s \"%s\" %.1fMHz (%s) in pool\n", typestring,
!                   p->name, p->Clock/1000.0, xf86ModeStatusToString(status));
            }
+           if (i < numDtModes)
+               i++;
+           else
+               r = r->next;
        }
  
        if (scrp->modePool == NULL) {
***************
*** 1675,1690 ****
       */
      while (scrp->modes)
        xf86DeleteMode(&scrp->modes, scrp->modes);
      endp = &scrp->modes;
      last = NULL;
!     if (modeNames != NULL) {
!       for (i = 0; modeNames[i] != NULL; i++) {
!           userModes = TRUE;
            new = xnfcalloc(1, sizeof(DisplayModeRec));
            new->prev = last;
!           new->type = M_T_USERDEF;
!           new->name = xnfalloc(strlen(modeNames[i]) + 1);
!           strcpy(new->name, modeNames[i]);
            if (new->prev)
                new->prev->next = new;
            *endp = last = new;
--- 1834,1912 ----
       */
      while (scrp->modes)
        xf86DeleteMode(&scrp->modes, scrp->modes);
+ 
+     if (((rmodeNames = modeNames) == NULL) || (rmodeNames[0] == NULL)) {
+          /* Use EDID modes */
+       userModes = FALSE;
+       if (numDtModes) {
+           rmodeNames = dt_mode_names;
+           numEdidModes = numDtModes;
+       }
+       else if (numStdModes) {
+           rmodeNames = std_mode_names;
+           numEdidModes = numStdModes;
+       }
+       else if (numStd2Modes) {
+           rmodeNames = std2_mode_names;
+           numEdidModes = numStd2Modes;
+       } else {
+           fallbackMode = TRUE;
+           rmodeNames = fallback_mode;
+           numEdidModes = 1;
+       }
+ 
+       if ((rmodeNames) && (rmodeNames != modeNames))
+           /* Sort and make them unique */
+           qsort((void *) rmodeNames, numEdidModes, sizeof (char *),
+               (int (*) (const void *, const void *)) modestrcmp);
+           for (i=0; i< numEdidModes; ) {
+               int j;
+ 
+               if (!rmodeNames[i] || !rmodeNames[i+1]) {
+                   break;
+               }
+               if (!strcmp(rmodeNames[i], rmodeNames[i+1])) {
+                   for (j = i+1; j < numEdidModes; j++) {
+                       if (rmodeNames[j+1])
+                               rmodeNames[j] = 
+                                       xnfstrdup (rmodeNames[j+1]); 
+                       else
+                               rmodeNames[j] = NULL;
+                   }
+               } else
+                       i++;
+           }
+     }
+ 
      endp = &scrp->modes;
      last = NULL;
!     if (rmodeNames != NULL) {
!       for (i = 0; rmodeNames[i] != NULL; i++) {
!           if (fallbackMode)
!               xf86DrvMsg(scrp->scrnIndex, X_INFO, 
!                   "Prepend Fallback mode name \"%s\" to validation list\n", 
!                   rmodeNames[i]);
!           else {
!               if (userModes)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO, 
!                       "Prepend User mode name \"%s\" to validation list\n", 
!                       rmodeNames[i]);
!               else
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO, 
!                       "Prepend EDID mode name \"%s\" to validation list\n",
rmodeNames[i]);
!           }
            new = xnfcalloc(1, sizeof(DisplayModeRec));
            new->prev = last;
!           if (userModes)
!               new->type = M_T_USERDEF;
!           else {
!               if (fallbackMode)
!                   new->type = M_T_DEFAULT;
!               else
!                   new->type = M_T_EDID;
!           }
!           new->name = xnfalloc(strlen(rmodeNames[i]) + 1);
!           strcpy(new->name, rmodeNames[i]);
            if (new->prev)
                new->prev->next = new;
            *endp = last = new;
***************
*** 1719,1736 ****
            r = NULL;
            modeSize = 0;
            for (q = scrp->modePool;  q != NULL;  q = q->next) {
                if ((q->prev == NULL) && (q->status == MODE_OK)) {
-                   /*
-                    * Deal with the case where this mode wasn't considered
-                    * because of a builtin mode of the same name.
-                    */
                    for (p = scrp->modes; p != NULL; p = p->next) {
!                       if ((p->status != MODE_OK) &&
!                           !strcmp(p->name, q->name))
                            break;
                    }
  
!                   if (p != NULL)
                        q->prev = p;
                    else {
                        /*
--- 1941,1962 ----
            r = NULL;
            modeSize = 0;
            for (q = scrp->modePool;  q != NULL;  q = q->next) {
+               /* EDID modes are already in validation list (scrp->modes) */
+               if (!userModes && (q->type == M_T_EDID))
+                   continue;
                if ((q->prev == NULL) && (q->status == MODE_OK)) {
                    for (p = scrp->modes; p != NULL; p = p->next) {
!                       /* Unique timing for each mode */
!                       if (!strcmp(p->name, q->name)) 
!                       /* Multiple timing for each mode */
!                       /*
!                       if (!strcmp(p->name, q->name) && (p->status != MODE_OK))
!                       */
                            break;
                    }
  
!                   /* Skip already previously considered (in scrp->modes) */
!                   if (p != NULL) 
                        q->prev = p;
                    else {
                        /*
***************
*** 1760,1771 ****
  
            if (r == NULL)
                break;
- 
            p = xnfcalloc(1, sizeof(DisplayModeRec));
            p->prev = last;
            p->name = xnfalloc(strlen(r->name) + 1);
!           if (!userModes)
!               p->type = M_T_USERDEF;
            strcpy(p->name, r->name);
            if (p->prev)
                p->prev->next = p;
--- 1986,1995 ----
  
            if (r == NULL)
                break;
            p = xnfcalloc(1, sizeof(DisplayModeRec));
            p->prev = last;
            p->name = xnfalloc(strlen(r->name) + 1);
!           p->type = r->type;
            strcpy(p->name, r->name);
            if (p->prev)
                p->prev->next = p;
***************
*** 1776,1793 ****
        repeat = FALSE;
      lookupNext:
        if (repeat && ((status = p->status) != MODE_OK)) {
!               if (p->type & M_T_BUILTIN)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using built-in mode \"%s\" (%s)\n",
!                              p->name, xf86ModeStatusToString(status));
!               else if (p->type & M_T_DEFAULT)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using default mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
!               else
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
        }
        saveType = p->type;
        status = xf86LookupMode(scrp, p, clockRanges, strategy);
--- 2000,2027 ----
        repeat = FALSE;
      lookupNext:
        if (repeat && ((status = p->status) != MODE_OK)) {
!           char *typestring;
! 
!           switch (p->type) {
!               case M_T_BUILTIN:
!                   typestring = xnfstrdup("builtin");
!                   break;
!               case M_T_USERDEF:
!                   typestring = xnfstrdup("user");
!                   break;
!               case M_T_EDID:
!                   typestring = xnfstrdup("EDID");
!                   break;
!               case M_T_DEFAULT:
!                   typestring = xnfstrdup("default");
!                   break;
!               default:
!                   typestring = xnfstrdup("");
!                   break;
!           }
!           xf86DrvMsg(scrp->scrnIndex, X_INFO,
!               "Not using %s \"%s\" %.1f MHz (%s)\n", typestring,
!               p->name, p->Clock/1000.0, xf86ModeStatusToString(status));
        }
        saveType = p->type;
        status = xf86LookupMode(scrp, p, clockRanges, strategy);
***************
*** 1795,1812 ****
            continue;
        }
        if (status != MODE_OK) {
!               if (p->type & M_T_BUILTIN)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using built-in mode \"%s\" (%s)\n",
!                              p->name, xf86ModeStatusToString(status));
!               else if (p->type & M_T_DEFAULT)
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using default mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
!               else
!                   xf86DrvMsg(scrp->scrnIndex, X_INFO,
!                              "Not using mode \"%s\" (%s)\n", p->name,
!                              xf86ModeStatusToString(status));
        }
        if (status == MODE_ERROR) {
            ErrorF("xf86ValidateModes: "
--- 2029,2056 ----
            continue;
        }
        if (status != MODE_OK) {
!           char *typestring;
! 
!           switch (p->type) {
!               case M_T_BUILTIN:
!                   typestring = xnfstrdup("builtin");
!                   break;
!               case M_T_USERDEF:
!                   typestring = xnfstrdup("user");
!                   break;
!               case M_T_EDID:
!                   typestring = xnfstrdup("EDID");
!                   break;
!               case M_T_DEFAULT:
!                   typestring = xnfstrdup("default");
!                   break;
!               default:
!                   typestring = xnfstrdup("");
!                   break;
!           }
!           xf86DrvMsg(scrp->scrnIndex, X_INFO,
!               "Not using %s \"%s\" %.1f MHz (%s)\n", typestring,
!               p->name, p->Clock/1000.0, xf86ModeStatusToString(status));
        }
        if (status == MODE_ERROR) {
            ErrorF("xf86ValidateModes: "
***************
*** 1924,1930 ****
  
      if (numModes <= 0)
        return 0;
!     
      /* Make the mode list into a circular list by joining up the ends */
      p = scrp->modes;
      while (p->next != NULL)
--- 2168,2184 ----
  
      if (numModes <= 0)
        return 0;
!   
!     for (p = scrp->modes; p != NULL; p = p->next)
!         if (p->status == MODE_OK) 
!           break;
!     if (p) {
!       xf86DrvMsg(scrp->scrnIndex, X_INFO,
!           "Valid mode on top of list : \"%s\" %.1f MHz %.1f kHz, %.1f Hz with
--\n", 
!           p->name, p->Clock/1000.0, ModeHSync(p), ModeVRefresh(p));
!       PrintModeline(scrp->scrnIndex, p);
!     }
! 
      /* Make the mode list into a circular list by joining up the ends */
      p = scrp->modes;
      while (p->next != NULL)
***************
*** 1932,1938 ****
      /* p is now the last mode on the list */
      p->next = scrp->modes;
      scrp->modes->prev = p;
- 
      if (minHeight > 0 && virtY < minHeight) {
        xf86DrvMsg(scrp->scrnIndex, X_ERROR,
                   "Virtual height (%d) is too small for the hardware "
--- 2186,2191 ----
***************
*** 2148,2163 ****
        if (p->VScan > 1) {
            desc2 = " (VScan)";
        }
!       if (p->type & M_T_BUILTIN)
!           prefix = "Built-in mode";
        else if (p->type & M_T_DEFAULT)
            prefix = "Default mode";
        else
            prefix = "Mode";
!       if (p->type & M_T_USERDEF)
!           uprefix = "*";
!       else
!           uprefix = " ";
        if (hsync == 0 || refresh == 0) {
            if (p->name)
                xf86DrvMsg(scrp->scrnIndex, X_CONFIG,
--- 2401,2417 ----
        if (p->VScan > 1) {
            desc2 = " (VScan)";
        }
!       if (!(p->type) || (p->type & M_T_EDID))
!             prefix = "EDID mode";
!       else if (p->type & M_T_USERDEF)
!           prefix = "User mode";
!       else if (p->type & M_T_BUILTIN)
!           prefix = "Builtin mode";
        else if (p->type & M_T_DEFAULT)
            prefix = "Default mode";
        else
            prefix = "Mode";
!       uprefix = "";
        if (hsync == 0 || refresh == 0) {
            if (p->name)
                xf86DrvMsg(scrp->scrnIndex, X_CONFIG,
Comment 6 Luc Verhaegen 2006-02-16 03:11:06 UTC
Not many people are used to reading non-unified diffs (please use diff -u).
I'm also having a terrible time applying this, so i'm not immediately able to
retrieve a unified diff from it.
Can you also just attach patches next time, by using the "Create a New
Attachment", above the additional comments window.

Anyway:
- offsets in edid.h:
I believe that it already contains the changes you have there (_MISC _SYNC_T).
My commit to edid.h could be very wrong though, as i don't have access to VESA
docs. In one edid block i had, the old values didn't parse right.

- xf86FallbackDDCProbe is not the right solution.
It is entirely up to the driver to handle this; the driver knows how to get the
EDID block and just needs a helper to attach and parse it. The driver knows what
monitor it's using and thus is the only thing that can decide _which_ edid block
should be considered for modes. Wether it uses VBE or proper i2c for retrieval,
is up to the driver. There are also devices which are not capable of DDC and VBE
doesn't change anything there. If the driver doesn't support it, then we simply
don't have EDID. If that's a shortcoming of the driver only, then the driver
needs to be fixed.

- We don't need a callback for DDC probing only. We need one for _all_ outputs,
not only for a single CRTs EDID block. It's too soon for this though and moving
on this now will result in the wrong choices being made. Correcting those wrong
choices later on is a lot more painful than just holding off for a bit now.
ScrnInfo would be the place for such a callback.

- None of the EDID parsing code should sit in xf86Mode.c.
This is one big mistake made in Dawes' ranges code in xf86Mode.c and one that
#5386 corrects. What we need at xf86ValidateModes is proper prioritisation based
on types, everything else should happen outside xf86Mode.c.

The root of the problem here is that it's approaching the problem from the wrong
angle. This is modesetting and things should thus be approached from the driver
up, everything else invariably leads to wrong choices and akward solutions. Not
many people realise this or they prefer to ignore it due to the slow and painful
work that driver side modesetting is.

Please do look at 5386 extensively, and join in on the discussion there. The
type priority is still wide open, i believe that the rest is pretty much "The
Right Thing" already (but don't let that stop you). Since you've been focussing
on mode selection and priority, you should try to bring in your views and ideas.

I will try write a more tangible proposal for type priority this evening,
outlining how i see things. I don't believe that ordering before mode validation
is the right thing to do. I intend to add helpers to make the handling of
modelists more dynamic after this, and that will clash when you depend on the
order of modes.

An actual implementation of the type priority will have to wait until the
dotclock hurdle is taken. First things first.
Comment 7 henry.zhao@oracle.com 2006-02-16 09:43:22 UTC
First of all, I agree none of the EDID parsing code should sit in xf86Mode, and 
like your solution to move it to ddc layer in 5386. I also agree it is driver's 
choice to doo i2c or VBE probing, as in current situation. But I still believe
it should be the common code, the valication code xf86ValidateMode(), to use the
parsed EDID modes/modelines and treat them with priority in valication process
so that a most satisfactory mode can be selected. My implementation is an
example for reference.

xf86FallbackDDCProbe is actually hack. Theoretically this shouldn't happen
since, as we all agreed, driver is supposed to take full probing responsibility.
However given the fact that currently i2c (DDC2) probing is not reliable enough, 
probably due to timing issue (this topic was briefly discussed in XorgDev
meeting, some one suggested using a kernel driver), use of xf86FallbackDDCProbe
really solves a lot problems in reading EDID data in  cases where probing from
driver side fails. Some of examples are: ViewSonic P815 CRT monitors, vga port
from some LCD monitors.
Comment 8 Luc Verhaegen 2006-02-16 10:54:48 UTC
Yeah, they should be treated with priority, which is what the next big step of
5386 is about. I'm just taking a detour through the rest of xf86Mode.c while i'm
at it. But please start the priority discussion there, it's the big question
mark that remains at 5386.

VBE is never a solution.

If VBE EDID probing works comprehensively while many drivers with xf86i2c fail,
then the timing of the xf86i2c code should be looked into. The solutions already
proposed only seems like "Let's move to VBE or kernel, as we don't want to deal
with X itself" again.

I'd love to point a scope with a decent bit of memory at a normal X i2c
implementation and correct some of the timing problems that might exist though.
Comment 9 Daniel Stone 2007-02-27 01:30:27 UTC
Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.
Comment 10 Luc Verhaegen 2007-02-27 05:32:28 UTC
I think that this can be closed.

Ajax committed most of what #5386 was about, at least the bits that are relevant here.

#5386 should still be kept open, as there is a list of drivers there that could become useful with minor adjustments.

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.