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

vdb at picaros.org vdb at picaros.org
Fri Nov 19 05:52:50 PST 2010 

> 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?"

The "why is this mode here" question is common when driving almost 
fixed frequency fixed sync ratio monitors.  Arcade game monitors is 
an example.  Even analog monitors driven over 10 m long rg179 coaxes 
require good modelines, the harmonics of the sync must line up with 
the dot spectrum or you will get vertical stripes.  Not surprisingly 
scaled TV modes work.  The command 'X -retro' is a very good test to 
see such effects.  

When I added the logic for a zoom mode list per monitor, I was 
mystified by the presence of an unconfigured modeline.  Tracing 
such oddity while testing is time consuming so I wrote this patch.  
It helped to see the type bits in the ModeDebug log.  Note that the 
bits U and P are used in the final modeline sort and also determine 
XRROutputInfo npreferred.

It turned out that the ati radeon video driver appended a GTF zoom 
mode from the xorg.conf screen section.  

> > 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

For readability 'z' for zoom is better, agreed.  I chose lowercase for 
informational bits and uppercase for sorted list influential bits.  
The U and P modelines are priority modes and by-pass normal sorting 
rules.  There should be at most 2 such modelines.  But I agree that 
lowercase reads better.  

> > 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'?

Yes I considered the long form, but
1. a modeline is compact and the log should be kept short, e.g. below 
   132 characters, 
2. users who modify xorg.conf are likely familiar with chmod o+r, ls,
   tcpdump tcp, which all use the lettered bitmap logic.

> > -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?

The invariant here is: 
  tlen is the byte count stored in type[] ==> tlen<=sizeof(type)
The alternative is: 
  tlen is the character count of the string type[] ==> tlen<sizeof(type)

For char *p the equivalent is *p++ = '\0' versus a *p = '\0'.  
I find the former a bit easier to debug, bounds checking, but 
optimization may indeed remove the '++' if p remains unused further 
on.  

Here all is evident but in e.g. the sendmail 8 code both approaches 
are seen, conditionals append fields by overwriting '\0' by ':' in 
'for' loops of 200 lines long.

This is just style really, if it isn't liked then that is fine.  

> > +
> >     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?

The leading space is added only for a nonzero mode->type.  This is 
similar to the add(&flags, "shortname") logic.  If no bits are set 
this gives "(62.5 kHz)".  If just one unknown bit is set this gives 
"(75.0 kHz )".  If known bits are set this gives "(75.0 kHz eP)".

> > +              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 */

More information about the xorg-devel mailing list