Re: [Xen-devel] [PATCH 2/2] xen/kbdif: add multi-touch support

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 10 Jan 2017, Oleksandr Andrushchenko wrote:
> On 01/07/2017 12:37 AM, Stefano Stabellini wrote:
> > On Fri, 6 Jan 2017, Oleksandr Andrushchenko wrote:
> > > From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
> > > 
> > > Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
> > > ---
> > >   xen/include/public/io/kbdif.h | 228
> > > ++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 228 insertions(+)
> > > 
> > > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h
> > > index 0e19a40..1b446f9 100644
> > > --- a/xen/include/public/io/kbdif.h
> > > +++ b/xen/include/public/io/kbdif.h
> > > @@ -57,6 +57,12 @@
> > >    *      Backends, which support reporting of absolute coordinates for
> > > pointer
> > >    *      device should set this to 1.
> > >    *
> > > + * feature-multi-touch
> > > + *      Values:         <uint>
> > > + *
> > > + *      Backends, which support reporting of multi-touch events
> > > + *      should set this to 1.
> > > + *
> > >    *------------------------- Pointer Device Parameters
> > > ------------------------
> > >    *
> > >    * width
> > > @@ -87,6 +93,11 @@
> > >    *      Request from backend to report absolute pointer coordinates
> > >    *      (XENKBD_TYPE_POS) instead of relative ones (XENKBD_TYPE_MOTION).
> > >    *
> > > + * request-multi-touch
> > > + *      Values:         <uint>
> > > + *
> > > + *      Request from backend to report multi-touch events.
> > I think in English should be "request backend to report multi-touch
> > events". Same above.
> Done
> > 
> > > + *
> > >    *----------------------- Request Transport Parameters
> > > -----------------------
> > >    *
> > >    * event-channel
> > > @@ -107,6 +118,43 @@
> > >    *      OBSOLETE, not recommended for use.
> > >    *      A unique (within the guest domain given) value identifying event
> > >    *      channel and page reference pair.
> > > + *
> > > + *----------------------- Multi-touch Device Parameters
> > > -----------------------
> > > + *
> > > + * Every multi-touch input device uses a dedicated event ring and is
> > For clarity I would say "Every multi-touch input device uses a dedicated
> > frontend/backend connection". That includes a ring, an event channel,
> > and xenstore entries.
> > 
> Done
> > > + * configured via XenStore properties under XENKBD_PATH_MTOUCH folder.
> > I don't understand why we need a new xenstore folder. Why do we need
> > XENKBD_PATH_MTOUCH?
> This is just another (IMO, cleaner) approach to define
> properties in XenStore for multiple instances.
> Let's see what vif does on my PC:
> /local/domain/11/device/vif/0/queue-0/tx-ring-ref = "2304"
> ...
> /local/domain/11/device/vif/0/queue-1/tx-ring-ref = "2306"
> ...
> /local/domain/11/device/vif/0/queue-2/tx-ring-ref = "2308"
> ...
> /local/domain/11/device/vif/0/queue-3/tx-ring-ref = "2310"
> ...
> /local/domain/11/device/vif/0/request-rx-copy = "1"
> 
> We have folders "queue-%d" per queue which in my case are
> 
> under XENKBD_PATH_MTOUCH.
> 
> /local/domain/11/device/vkbd/0/mtouch/0/width = "3200"
> /local/domain/11/device/vkbd/0/mtouch/0/height = "3200"
> /local/domain/11/device/vkbd/0/mtouch/0/num-contacts = "5"
> /local/domain/11/device/vkbd/0/mtouch/1/width = "6400"
> /local/domain/11/device/vkbd/0/mtouch/1/height = "6400"
> /local/domain/11/device/vkbd/0/mtouch/1/num-contacts = "10"
> 
> Namely, instead of "mtouch-%d" I use "mtouch/%d/.
> This is just like using "/local/domain/%d" but not
> "/local/domain-%d", so "mtouch/%d" seem to be more
> consistent.

I agree that mtouch/%d is better than mtouch-%d, but it's only necessary
if we have more than one mtouch per vkbd. I would configure it so that
one can only have one mtouch per vkbd:

  /local/domain/11/device/vkbd/0/mtouch/width = "3200"
  /local/domain/11/device/vkbd/0/mtouch/height = "3200"
  /local/domain/11/device/vkbd/1/mtouch/width = "6400"
  /local/domain/11/device/vkbd/1/mtouch/height = "6400"

This is also what I was referring to when I wrote:

> It is useful to distinguish mouse events from keyboard events, from
> multitouch events more easily, but I don't think we should support
> multiple keyboards or multiple mice on the same xenkbd ring. If we need
> two mice, we can setup two xenkdb rings.

The same applies to multitouch devices, I think.

But maybe you have good reasons for supporting multiple multitouch
devices on a single vkbd connection? How many do you expect to have on a
single VM?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel