[PATCH] xserver xf86PrintModeline(): print DisplayMode type bits.

[Jesse Adkins]

I have to say that I'm not really sure of the reasoning behind this.
How does this really help? From reading modelines from different bug
reports over time, the most common issue seems to be not "why is this
mode here" but rather "why is this supposedly valid mode being
rejected?"

I wouldn't trust my opinion, since I haven't been around for too long.
You might want to ask one of the other regulars on the xorg-devel IRC.

> type_bit       flag   origin
> M_T_USERPREF   U      set by the Option "PreferredMode"
Can you make this a lowercase 'u'?
> M_T_DRIVER     e      driver: EDID, flat panel native
> M_T_USERDEF    u      configured zoom mode Ctrl+Alt+Keypad-{Plus,Minus}
Can you make this 'z' for zoom?
> M_T_DEFAULT    d      VESA default
> M_T_PREFERRED  P      driver preferred timing, also set by PreferredMode
Can you lowercase the 'p'? I ask all this because it's easier to read
when there's no confusing U and u for things.
> M_T_BUILTIN    b      hardware fixed CRTC mode

> Modeline "1600x1200 at 60"x60.0  144.00  1600 1632 1792 1920  1200 1201 1204 1250
>  (75.0 kHz UP)
> Modeline "1600x1200"x60.0  162.00  1600 1664 1856 2160  1200 1201 1204 1250
>  +hsync +vsync (75.0 kHz eP)
> Modeline "1600x1200 at 50"x50.0  120.00  1600 1628 1788 1920  1200 1201 1204 1250
>  (62.5 kHz)
What? Cryptic letters? No 'edid' or * for preferred or 'builtin'?


> -xf86PrintModeline(int scrnIndex,DisplayModePtr mode)
> +xf86PrintModeline(int scrnIndex, DisplayModePtr mode)
>  {
>     char tmp[256];
> +    char tchar[] = "UeudPb";
> +    int tbit[] = { M_T_USERPREF, M_T_DRIVER, M_T_USERDEF,
> +                  M_T_DEFAULT, M_T_PREFERRED, M_T_BUILTIN };
> +    char type[sizeof(tchar)+2];
> +    int tlen = 0;
>     char *flags = xnfcalloc(1, 1);
>
> +    if (mode->type) {
> +      int i;
> +
> +      type[tlen++] = ' ';
> +      for (i = 0; i < sizeof(tchar); i++)
> +       if (mode->type & tbit[i])
> +         type[tlen++] = tchar[i];
> +    }
> +    type[tlen++] = '\0';
Why not type[tlen]? tlen isn't going to be used again, so why increment it?
> +
>     if (mode->HSkew) {
>        snprintf(tmp, 256, "hskew %i", mode->HSkew);
>        add(&flags, tmp);
> @@ -318,12 +333,12 @@ #if 0
>     if (mode->Flags & V_CLKDIV2) add(&flags, "vclk/2");
>  #endif
>     xf86DrvMsg(scrnIndex, X_INFO,
> -                  "Modeline \"%s\"x%.01f  %6.2f  %i %i %i %i  %i %i %i %i%s "
> -                  "(%.01f kHz)\n",
> -                  mode->name, mode->VRefresh, mode->Clock/1000., mode->HDisplay,
> -                  mode->HSyncStart, mode->HSyncEnd, mode->HTotal,
> -                  mode->VDisplay, mode->VSyncStart, mode->VSyncEnd,
> -                  mode->VTotal, flags, xf86ModeHSync(mode));
> +              "Modeline \"%s\"x%.01f  %6.2f  %i %i %i %i  %i %i %i %i%s"
> +              " (%.01f kHz%s)\n",
I'm probably being nitpicky at this point, but I can see this being
confusing if you just looked at this "Wouldn't this result in things
like kHzEP?", until you look at 'type' and see that it's got a leading
space.
Why isn't there a space after the kHz, and the mode printed without a
leading space?
> +              mode->name, mode->VRefresh, mode->Clock/1000.,
> +              mode->HDisplay, mode->HSyncStart, mode->HSyncEnd, mode->HTotal,
> +              mode->VDisplay, mode->VSyncStart, mode->VSyncEnd, mode->VTotal,
> +              flags, xf86ModeHSync(mode), type);
>     free(flags);
>  }
>  #endif /* XORG_VERSION_CURRENT <= 7.2.99.2 */
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>