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.
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.
Erm, isn't this a duplicate of #5386?
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.
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.
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,
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.
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.
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.
Sorry about the phenomenal bug spam, guys. Adding xorg-team@ to the QA contact so bugs don't get lost in future.
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.