[PATCH] xserver: add udev/systemd multi-seat support
Peter Hutterer
peter.hutterer at who-t.net
Sun Jul 17 19:28:12 PDT 2011
[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.
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.
> Finally, this also unifies the code paths for the udev "change" and
> "add" events, since these should be handled the same way in order to be
> robust to "udevadm trigger" calls, or lost netlink messages. This also
> simplifies the code.
>
> Note that the -seat switch takes a completely generic identifier, and
> that it has no effect on non-Linux systems. In fact, on other OSes a
> completely different identifier scheme for seats could be used but still
> be exposed with the Xorg_Seat and -seat.
>
> I tried to follow the coding style of the surrounding code blocks if
> there was any one could follow.
that _is_ the coding style :)
> ---
> config/udev.c | 34 +++++++++++++++++++++++++++++-----
> hw/xfree86/common/xf86Init.c | 19 +++++++++++++++++++
> include/globals.h | 2 +-
> os/utils.c | 10 ++++++++++
> 4 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/config/udev.c b/config/udev.c
> index 9ac34ee..b30f075 100644
> --- a/config/udev.c
> +++ b/config/udev.c
> @@ -35,6 +35,7 @@
> #include "hotplug.h"
> #include "config-backends.h"
> #include "os.h"
> +#include "globals.h"
>
> #define UDEV_XKB_PROP_KEY "xkb"
>
> @@ -65,6 +66,7 @@ device_added(struct udev_device *udev_device)
> struct udev_list_entry *set, *entry;
> struct udev_device *parent;
> int rc;
> + const char *dev_seat;
>
> path = udev_device_get_devnode(udev_device);
>
> @@ -73,6 +75,16 @@ device_added(struct udev_device *udev_device)
> if (!path || !syspath)
> return;
>
> + dev_seat = udev_device_get_property_value(udev_device, "ID_SEAT");
> + if (!dev_seat)
> + dev_seat = "seat0";
> +
> + if (SeatId && strcmp(dev_seat, SeatId))
> + return;
> +
> + if (!SeatId && strcmp(dev_seat, "seat0"))
> + return;
> +
> if (!udev_device_get_property_value(udev_device, "ID_INPUT")) {
> LogMessageVerb(X_INFO, 10,
> "config/udev: ignoring device %s without "
> @@ -251,14 +263,12 @@ wakeup_handler(pointer data, int err, pointer read_mask)
> return;
> action = udev_device_get_action(udev_device);
> if (action) {
> - if (!strcmp(action, "add"))
> - device_added(udev_device);
> - else if (!strcmp(action, "remove"))
> - device_removed(udev_device);
> - else if (!strcmp(action, "change")) {
> + if (!strcmp(action, "add") || !strcmp(action, "change")) {
> device_removed(udev_device);
> device_added(udev_device);
> }
> + else if (!strcmp(action, "remove"))
> + device_removed(udev_device);
> }
> udev_device_unref(udev_device);
> }
this should be a separate patch, it doesn't really have anything to do with
the multiseat patches.
> @@ -283,6 +293,10 @@ config_udev_init(void)
> if (!udev_monitor)
> return 0;
>
> + udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, "input", NULL);
> + if (strcmp(SeatId, "seat0"))
> + udev_monitor_filter_add_match_tag(udev_monitor, SeatId);
> +
> if (udev_monitor_enable_receiving(udev_monitor)) {
> ErrorF("config/udev: failed to bind the udev monitor\n");
> return 0;
> @@ -291,11 +305,21 @@ config_udev_init(void)
> enumerate = udev_enumerate_new(udev);
> if (!enumerate)
> return 0;
> +
> + udev_enumerate_add_match_subsystem(enumerate, "input");
> + if (strcmp(SeatId, "seat0"))
> + udev_enumerate_add_match_tag(enumerate, SeatId);
> +
> udev_enumerate_scan_devices(enumerate);
> devices = udev_enumerate_get_list_entry(enumerate);
> udev_list_entry_foreach(device, devices) {
> const char *syspath = udev_list_entry_get_name(device);
> struct udev_device *udev_device = udev_device_new_from_syspath(udev, syspath);
> +
> + /* Device might be gone by the time we try to open it */
> + if (!udev_device)
> + continue;
> +
this seems unrelated as well
> device_added(udev_device);
> udev_device_unref(udev_device);
> }
> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> 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
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)
> + Atom SeatAtom;
> +
> + SeatAtom = MakeAtom(SEAT_ATOM_NAME, sizeof(SEAT_ATOM_NAME) - 1, TRUE);
i think strlen() is more common than sizeof() for this.
> +
> + 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.
And this should be added to the man page as well, together with the
"Linux only"-ness.
Cheers,
Peter
> else if ( strcmp( argv[i], "-t") == 0)
> {
> if(++i < argc)
> --
> 1.7.6
More information about the xorg-devel
mailing list