[PATCH] xserver: add udev/systemd multi-seat support

[Lennart Poettering]

On Mon, 18.07.11 12:28, Peter Hutterer (peter.hutterer at who-t.net) wrote:

> 
> [removing xorg, adding xorg-devel, see
> http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches]
> 
> On Sun, Jul 17, 2011 at 02:33:14AM +0200, Lennart Poettering wrote:
> > Add support for multi-seat-aware input device hotplugging. This
> > implements the multi-seat scheme explained here:
> > 
> > http://www.freedesktop.org/wiki/Software/systemd/multiseat
> > 
> > This introduces a new X server switch "-seat" which allows configuration
> > of the seat to enumerate hotplugging devices on. If specified the value
> > of this parameter will also be exported as root window property Xorg_Seat.
> > 
> > To properly support input hotplugging devices need to be tagged in udev
> > according to the seat they are on. Untagged devices are assumed to be on
> > the default seat "seat0". If no "-seat" parameter is passed only devices
> > on "seat0" are used. This means that the new scheme is perfectly
> > compatible with existing setups which have no tagged input devices.
> > 
> > This also optimizes the udev code in away that is beneficial for systems
> > that do not support new udev/systemd style multi-seat: the patch makes
> > use of libudev functions to limit enumerating/monitoring of devices to
> > actual input devices. This should speed up start-up and minimize
> > wake-ups, since we will only enumerate/monitor the devices we are
> > interested in instead of all devices on the machine and all events they
> > generate.
> 
> see this recent thread
> http://lists.x.org/archives/xorg-devel/2011-May/022332.html
> summary: we don't know which subsystems will supply input devices, hints are
> appreciated.

Uh, this appears completely bogus to me. You check ID_INPUT on all
devices, which is set by the udev tool /lib/udev/input_id which works
only on input devices, not on serial devices. Hence by enumerating only
input subsys devices is a valid optimization.

If I understand this correctly, then the udev enumeration code only
generates code suitable for the evdev input backend anyway, right? evdev
is always in the "input" subsystem, never in the "tty" subsystem. If you
have a serial input device, then this will show up as a device from the
"input" subsystem which is a child of one from the "tty" subsystem, but
X11 should never be interested in the serial device itself.

If this is supposed to help with the wacom driver, then this is bogus
too, as the associated rule in /lib/udev/rules.d/70-wacom.rules actually
does check for ID_INPUT.

I really really doubt you need to check for any devices outside the
"input" subsystem. And even if you did you should be aware that if you
pass two subsystems to match against to the udev enumerator/monitor
you'll get the union, not the intersection. That means even if X had any
business with enuemrating serial ports (which it really hasn't) it could
just do that by adding multiple subsys matches to the same enum/monitor
objects.

There isnt really an excuse for enumerating all devices on a system. You
should not underestimate the number of devices that can appear in the
udev tree.

(And I discussed this with Kay actually, and he agrees with what I am
saying here...)

> either way, sneaking this into the multi-seat patch is a bit rough. Some
> devices would have stopped working and the blame would have been on the new
> multi-seat support. Making this a separate patch would be easier to review
> and debug.

Sure, I will split this up, but I will leave the input subsys match in
there. Everything else is just borked.

> > index 15fdbc3..e3769eb 100644
> > --- a/hw/xfree86/common/xf86Init.c
> > +++ b/hw/xfree86/common/xf86Init.c
> > @@ -654,6 +654,25 @@ InitOutput(ScreenInfo *pScreenInfo, int argc, char **argv)
> >        }
> >      }
> >  
> > +    if (SeatId) {
> > +#define SEAT_ATOM_NAME "Xorg_Seat"
> 
> please move this define to include/xserver-properties.h

Uhmm, will do. Note however that just a few lines above this code you'll
find the code setting the XFree86_VT as root property on the display,
which is what I basically copied. That prop does not appear in xs-p.h either.

> Also, property names can include spaces, so the '_' isn't really necessary.
> Any reason we use SeadId in code, but Xorg_Seat in the atom? Why not use
> "Seat ID"? (with or without space)

To follow the scheme of the XFree86_VT property. And I had discussed
this with ajax on IRC and we agreed to use use Xorg_Seat in this
capitalization.

> > +        Atom SeatAtom;
> > +
> > +        SeatAtom = MakeAtom(SEAT_ATOM_NAME, sizeof(SEAT_ATOM_NAME) - 1, TRUE);
> 
> i think strlen() is more common than sizeof() for this.

Well, it is also slower, and just five lines north of this sizeof()-1 is
used too. There's really no point in invoking a function here.

> > +
> > +        for (i = 0; i < xf86NumScreens; i++) {
> > +            int ret;
> > +
> > +            ret = xf86RegisterRootWindowProperty(xf86Screens[i]->scrnIndex,
> > +                                                 SeatAtom, XA_STRING, 8,
> > +                                                 strlen(SeatId)+1, SeatId );
> > +            if (ret != Success) {
> > +                xf86DrvMsg(xf86Screens[i]->scrnIndex, X_WARNING,
> > +                           "Failed to register seat property\n");
> > +            }
> > +        }
> > +    }
> > +
> >      /* If a screen uses depth 24, show what the pixmap format is */
> >      for (i = 0; i < xf86NumScreens; i++) {
> >  	if (xf86Screens[i]->depth == 24) {
> > diff --git a/include/globals.h b/include/globals.h
> > index 8b80a65..17bca82 100644
> > --- a/include/globals.h
> > +++ b/include/globals.h
> > @@ -21,7 +21,7 @@ extern _X_EXPORT int defaultColorVisualClass;
> >  
> >  extern _X_EXPORT int GrabInProgress;
> >  extern _X_EXPORT Bool noTestExtensions;
> > -
> > +extern _X_EXPORT char *SeatId;
> >  extern _X_EXPORT char *ConnectionInfo;
> >  
> >  #ifdef DPMSExtension
> > diff --git a/os/utils.c b/os/utils.c
> > index 36cb46f..17869f0 100644
> > --- a/os/utils.c
> > +++ b/os/utils.c
> > @@ -201,6 +201,8 @@ Bool PanoramiXExtensionDisabledHack = FALSE;
> >  
> >  int auditTrailLevel = 1;
> >  
> > +char *SeatId = NULL;
> > +
> >  #if defined(SVR4) || defined(__linux__) || defined(CSRG_BASED)
> >  #define HAS_SAVED_IDS_AND_SETEUID
> >  #endif
> > @@ -511,6 +513,7 @@ void UseMsg(void)
> >      ErrorF("-render [default|mono|gray|color] set render color alloc policy\n");
> >      ErrorF("-retro                 start with classic stipple and cursor\n");
> >      ErrorF("-s #                   screen-saver timeout (minutes)\n");
> > +    ErrorF("-seat string           seat to run on\n");
> >      ErrorF("-t #                   default pointer threshold (pixels/t)\n");
> >      ErrorF("-terminate             terminate at server reset\n");
> >      ErrorF("-to #                  connection time out\n");
> > @@ -802,6 +805,13 @@ ProcessCommandLine(int argc, char *argv[])
> >  	    else
> >  		UseMsg();
> >  	}
> > +        else if ( strcmp( argv[i], "-seat") == 0)
> > +        {
> > +            if(++i < argc)
> > +                SeatId = argv[i];
> > +            else
> > +                UseMsg();
> > +        }

> indentation.

The indentation is actually right in my patch. It's just chaos in the source file
between tabs and spaces. Or what do you mean?

> And this should be added to the man page as well, together with the
> "Linux only"-ness.

Will add.

Thanks,

Lennart

-- 
Lennart Poettering - Red Hat, Inc.