[PATCH weston 3/4] compositor-drm: Enable seat constraining when configured in weston.ini

[Pekka Paalanen]

On Thu, 13 Jun 2013 16:17:17 +0100
Rob Bradford <robert.bradford at intel.com> wrote:

> On Tue, Jun 11, 2013 at 03:33:46PM +0300, Pekka Paalanen wrote:
> > Hi Rob,
> > 
> > I think patches 2, 3, and 4 should be all squashed, and I would
> > like to know more of what is the use case here.
> > 
> > More questions below.
> > 
> > 
> > On Mon, 10 Jun 2013 15:17:30 +0100
> > Rob Bradford <robert.bradford at intel.com> wrote:
> > 
> > > From: Rob Bradford <rob at linux.intel.com>
> > > 
> > > ---
> > >  src/compositor-drm.c | 16 ++++++++++++++++
> > >  src/compositor.h     |  2 ++
> > >  src/input.c          |  2 ++
> > >  src/udev-seat.c      |  3 +++
> > >  4 files changed, 23 insertions(+)
> > > 
> > > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > > index 76d0810..7d33977 100644
> > > --- a/src/compositor-drm.c
> > > +++ b/src/compositor-drm.c
> > > @@ -1796,6 +1796,22 @@ create_output_for_connector(struct
> > > drm_compositor *ec, transform = parse_transform(s,
> > > output->base.name); free(s);
> > >  
> > > +	weston_config_section_get_string(section, "seat", &s,
> > > "");
> > > +	if (strcmp(s, "") != 0) {
> > > +		struct udev_seat *seat;
> > > +
> > > +		seat = udev_seat_get_named(&ec->base, s);
> > > +		if (seat) {
> > > +			seat->base.output = &output->base;
> > > +			if (seat->base.pointer) {
> > > +
> > > weston_pointer_output_center(seat->base.pointer,
> > > +
> > > &output->base);
> 
> Hi Pekka,
> 
> Thanks for the feedback below. I'll do my best to answer everything.
> 
> > Does this mean, that every time an output is hotplugged in this
> > seat, the pointer will warp?
> > 
> > What if a seat has several outputs?
> > 
> 
> Excellent point. I think I could instead go with a
> weston_pointer_ensure_valid_location.
> 
> This function would be based off of the code in pointer_clip_motion()
> and then that function could be written in terms of this.
> 
> Thus warping would only happen if the seat was previously
> unconstrained and the output that came about wanted it constrained to
> it.

Yeah, that sounds sensible.

And I believe the current implementation of pointer_clip_motion()
relies on the previous position being valid, and if it is not, then it
segfaults. Or maybe it just used to be like that. Anyway, a monitor
hot-unplug could be problematic there already.

> > > +	if (seat->base.output && seat->base.pointer)
> > > +		weston_pointer_output_center(seat->base.pointer,
> > > seat->base.output);
> > 
> > Do I understand right, that whenever any input device is hotplugged,
> > like a keyboard, the pointer will warp?
> 
> Yeh I think you're right here. The function I mentioned above could
> probably fix this too. But it would be nice to be cleverer about when
> to call this function
> 
> > > +
> > >  	return 0;
> > >  }
> > >  
> > 
> > From the man page patch:
> > 
> > +.BI "seat=" name
> > +The logical seat name that that this output should be associated
> > with. If this +is set then the seat's input will be confined to the
> > output that has the seat +set on it. The expectation is that this
> > functionality will be used in a +multiheaded environment with a
> > single compositor for multiple output and +input configurations.
> > 
> > So, this is an output key. It is almost as if you were trying to
> > implement seats by using wl_seats. I don't think that will work.
> > Like here, you try to confine the pointer, but what if a user
> > alt+tabs? No constraints there. I'm confused what this is trying to
> > achieve.
> > 
> 
> > Let me explain. I believe that wl_seat is misnamed. wl_seat is a
> > set of foci for a set on input devices, used by one person, or by
> > several people collaborating on a common desktop. Everyone has
> > access to everything, using their own input devices.
> > 
> > A seat is a physical entity, a console; an output or outputs, and
> > the associated input devices. Each seat has its own user
> > session(s), which is easiest to implement by running a compositor
> > per seat. Sessions are separate by definition, and for security.
> > Separating the sessions within a compositor is very hard.
> > 
> > I think, that seats are configured in udev, and wl_seats will be
> > configured in weston.ini or equivalent per-session configuration. A
> > seat may have multiple wl_seats, if you want e.g. multi-pointer.
> > Currently, we have no implementation for configuring multiple
> > wl_seats; device_added() will ignore all devices that do not match
> > the wanted ID_SEAT property, and only one udev_input with a wanted
> > seat_id is created in a backend.
> > 
> > Could you elaborate on what you are trying to do?
> 
> The requirement we have for this this is to be able to run a
> single compositor (with it's own custom shell) that would be driving
> multiple outputs using the same GPU. Clients might need to multiple
> outputs or move outputs - so having multiple compositor instances
> would be complicated.

Ah, custom shell.

> (Also can we have multiple compositor instances
> using the same GPU - and driving different outputs....?)

Not yet, I think, but I believe the required kernel interfaces are
being developed. E.g. Dave Airlie should know.

> Each of these outputs has some input devices associated with it -
> probably a touch screen and perhaps some buttons. Obviously if a
> touchscreen is part of a particular physical display
> 
> In my patches that are now merged I added support to create multiple
> udev_seats based on labelling the devices in udev with ENV{WL_SEAT}.
> 
> The goal of these combined patch sets is to be able to create multiple
> independent logical seats with some primitive isolation: touch and
> pointer confinement. Of course the custom shell will need to make sure
> client surface appear on the correct outputs as well as making sure
> things like "Alt-Tab" you mentioned don't happen.

Ok, yes, that explains it.

> Do you think improving the terminology - using "logical seat" to refer
> to the soft weston seat would be helpful.

Maybe "physical seat"? Since you are really trying to make separate
physical seats within one compositor, and probably representing each
physical seat with just one wl_seat.

Btw. does this work somehow imply, that if you do seat isolation
within a single Weston instance, then each physical seat will
necessarily be exactly one wl_seat?

If you didn't already, that would be good to document.

Do you have plans for Weston's desktop shell, how it would behave, if
one has ENV{WL_SEAT} set up, or would it just be "broken" without your
special shell? For instance, a window appears on another output and
then you can't reach it with your own seat's pointer.

Should this perhaps be a Weston capability bit, "shell supports
isolated wl_seats" or just "wl_seat & output isolation"? Set by the
shell, used by the backend/input code.


Thanks,
pq