Review Request 112294: Implement multi-seat support in KDM

Wed Apr 9 20:09:22 BST 2014

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1397>
> >
> >     that seems questionable to me. why are you re-defining the display to be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
>     When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a "d->displayType = dLocal | dReserve" is missing for these cases.
>     But as long as the seat exists, you want it to behave like a static display (restart server after session exit), so dPermanent is IMHO correct.
> 
> Oswald Buddenhagen wrote:
>     i see what you mean ...
>     still, it seems wrong to change the display type just so. this is supposed to be a constant. this is of course a consequence of abusing reserve displays in the first place - technically, there should be a dDynamic display type. if having reserve displays on dynamic seats is technically possible at all, this would also need some thought.
> 
> Stefan Brüns wrote:
>     So, what approach to take here:
>     1. Its somewhat ugly, but it works, so use it.
>     2. Introduce a dDynamic display type, and make it behave like dPermanent.
>       a) add a DynamicServers option for this
>       b) create list of displays on the fly.
>     
>     Reserve displays on dynamic seats is not possible currently, you cannot change the VT. For this to work you need support for systemd-login, but this is more serious surgery.
> 
> Oswald Buddenhagen wrote:
>     i would prefer 2.a. it shouldn't be much work.
>     i had shortly pondered 2.b myself, but then you'd need a new factory concept, and the configuration would be different, so i discarded it.
> 
> Stefan Brüns wrote:
>     Currently I have not implemented (2a), but still use (1). I still can do that, although I see no apparent gain here until there is a possibility to configure individual seats. If this is really required I will do that, but this will take another week (I can only do this in my spare time ...)

the gain is that it makes the system cleaner and more predictable.
it should be really pretty much trivial to implement it (just follow the pattern for ReserveDisplays).

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1409
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1409>
> >
> >     i prefer !strcmp(). repeats several times.
> >     
> >     also, this seems like a hack. it should be possible to properly query whether kdm should manage this display.
> 
> Stefan Brüns wrote:
>     1. changed to !strcmp()
>     
>     2. with the current patch all seats but seat0 which also have the CanGraphical property are picked up by KDM. Although this is not ideal, it is still a major improvement to the current situation. Currently I can not see any use case for a graphical seat not managed by KDM, but I see a use case for multiseat in general.
>     In general this can be achieved if one extends the patch set with the extended configuration mentioned below (using display classes).
>     
>     I would prefer to keep the current patch set as is.

the question is why you are handling seat0 differently to start with.

the obvious solution would seem having an empty StaticServers and pulling :0 from DynamicServers as well.
now the question is what to do if systemd isn't actually there. does kdm have to do anything useful when it is configured with the incorrect assumption of systemd being there? i don't think so, given how central systemd is.

and given the config stuff discussed below, you can have both:
StaticServers=:0_ at seat0
DynamicServers=:3_ at seat1
i.e., kdm manages :0 as belonging to a seat, and if systemd *also* reports it, then kdm can ignore it without any hard-coded special cases.
not sure about automatically passing the -seat option to static servers declared this way - what does a server do in such a case if there is no systemd running?

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186613#file186613line79>
> >
> >     why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
>     There are no config matches taking the "seat" into account, so the only possibility to apply a specific config is to use "layout".
> 
> Oswald Buddenhagen wrote:
>     something is wrong about this.
>     what does the -seat option do in the first place? if it is sufficient to actually launch the server correctly, then that's all that should be done - the user can manually specifiy ServerArgs if he needs to configure the layout for some reason. otherwise every user is forced to actually have a "seatN" layout in his config.
>     also, cannot the layout determine the seat? seems a bit stupid that these are entirely orthogonal.
> 
> Stefan Brüns wrote:
>     The seat option is used for matching devices from udev, i.e. input devices, GPUs, ... It specifies *which* devices to use, but now *how to configure* these.
>     
>     Unfortunately the config files lack a "MatchSeat" option, so the only possible options are either specifying a config file (or directory) or a layout. There is a patch from Oleg Samarin floating around which adds MatchSeat, but this is not upstream so currently no option.
>     
>     If a layout is specified but not available, the default layout is chosen, so no problem here.
>     
>     ServerArgs is no alternative as it is specific to a display, but the seats will use a more or less random display, dependent on when they become available (much like Reserve servers, which take the first display available).
> 
> Oswald Buddenhagen wrote:
>     hrmpf. again somebody did half a job, and others need to make workarounds to make it usable.
>     
>     it may be actually possible to integrate this nicely with kdm's super-complex config system.
>     it already supports a display-identifying mechanism, the so-called display class. you can have groups that look like [X-*:*_foos-Core]. you can even have StaticServers=:0_foos,:1_foos,:3_bars.
>     
>     one way to make use of this would be simply replacing the display class with @seat1 or something like that. the actual display class is pretty much meaningless for local displays anyway (it was introduced to group remote (predominantly xdmcp) displays, which could have different physical properties for example).
>     
>     another way would be making local displays look like @seat1:1. that makes a lot of sense - the seat kinda is the "host", even if of a different kind. this approach might be a bit more complex, though (it would need special handling of the host part, while just replacing the display class would be practically zero effort).
>     
>     that way one could even have static secondary seats (something people had hacked a decade ago already). and it would have potential for supporting reserve displays on secondary seats (ReserveServers=:1,:2,:3, at seat1:5, at seat1:6). DynamicServers could use both static (@seat1:4, at seat2:7) and dynamic (:4,:7) seat to display assignment.
>     if the display class based approach was chosen, this would be :4_ at seat1 and so on.
>     
>     this seems quite complex from the user perspective, but the default set would be pretty static, so one can put it into config.def to have genkdmconf produce the common skeleton. at some point we'd remove the template, and people could remove it from their old configs (as opposed to leaving it in forever or making a hard cutoff, which would be the options if -layout was hard-coded in kdm itself).
>     
>     note that integration with the config system is sort of necessary anyway - for example to make auto-login configurable on secondary seats.
> 
> Stefan Brüns wrote:
>     IMHO this is material for a second patchset. Secondary seats with reserve displays will require much more integration with logind, and this will be major surgery. Auto-login on secondary seats would be nice to have (and you can use it as long as you only have one secondary seat in addition to seat0), but I prefer to postpone this.

reserve displays on secondary seats don't need to be implemented now, but it makes sense to consider them in the design to the degree i already did.
the config stuff should be done properly from the start.

- Oswald

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/112294/#review54418
-----------------------------------------------------------

On April 6, 2014, 10:01 p.m., Stefan Brüns wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -----------------------------------------------------------
> 
> (Updated April 6, 2014, 10:01 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is not running, nothing changes. If no additional seats have been configured (some Plugable USB-GPUs are automatically added as additional seats), nothing changes.
> 
> In case there are additional seats beyond seat0, a reserved display is promoted to a local static one (and demoted if the seat is removed) and a new X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -----
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
>   kdm/config-kdm.h.cmake 3e8912d 
>   plasma/desktop/applets/windowlist/plasma-applet-windowlist.desktop 50c9f66 
>   plasma/desktop/containments/desktop/plasma-containment-desktop.desktop ce9fe38 
>   plasma/generic/wallpapers/image/plasma-wallpaper-image.desktop 6484545 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> -------
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140409/6ec7accf/attachment.htm>