Re: [Xen-devel] [RFC] kbdif: add multi-touch support

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

First of all, thank you for comments

On 01/04/2017 03:03 AM, Stefano Stabellini wrote:
> On Tue, 3 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 | 64 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 64 insertions(+)
> >
> > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h
> > index 2d2aebd..ad94b53 100644
> > --- a/xen/include/public/io/kbdif.h
> > +++ b/xen/include/public/io/kbdif.h
> > @@ -45,6 +45,19 @@
> >    */
> >   #define XENKBD_TYPE_POS     4
> >   
> > +/*
> > + * Multi-touch event
> > + * Capable backend sets feature-multi-touch in xenstore.
> > + * Frontend requests feature by setting request-multi-touch in xenstore.
> > + * Frontend supports up to XENKBD_MT_NUM_DEV virtual multi-touch input devices,
> > + * configured by the backend in xenstore under mt-%d folder, %d being
> > + * a sequential number of the virtual input device:
> > + *   o num-contacts - number of simultaneous touches supported
> > + *   o width - width of the touch area in pixels
> > + *   o height - height of the touch area in pixels
> Please write down the max width and height supported by the protocol,
> keeping in mind that motion events below use int32_t for coordinates.
I will put "width(height) of the... in pixels, 32-bit signed integer"
> Are there any benefits of this compared to just setting up multiple kbd
> connections, one per multi-touch device? The only benefit I can think of
> is saving few pages.
Well, not only saving a few pages, but somewhat
simplifying handling of the protocol on both back and
front ends. But, you are probably right as current
protocol is capable of holding
(XENKBD_IN_RING_SIZE - XENKBD_IN_RING_OFFS) / XENKBD_IN_EVENT_SIZE ==
(2048 - 1024) / 40 = 25 incoming events which may not be
enough for multiple mt devices delivering hundreds of
events per second.
Will set-up dedicated rings per mt device then.
Will also remove
  uint8_t dev_idx;        /* index of the multi-touch device */
as every device will have its own ring.
Will extend XenStore configuration for mt devices with:
 o page-ref (?)
 o page-gref
 o event-channel
as it is done by the Linux xen-kbdfront driver.

BTW, is there any reason we need "page-ref"? My understanding is
that the pair of page-gref + event-channel is enough to establish
and uniquely identify the connection.

> > + */
> > +#define XENKBD_TYPE_MTOUCH  5
> >   struct xenkbd_motion
> >   {
> >       uint8_t type;        /* XENKBD_TYPE_MOTION */
> > @@ -68,6 +81,56 @@ struct xenkbd_position
> >       int32_t rel_z;       /* relative Z motion (wheel) */
> >   };
> >   
> > +/* number of simultaneously supported multi-touch virtual input devices */
> > +#define XENKBD_MT_NUM_DEV   4
> If it turns out that supporting multiple devices per connection is a
> good idea, then I suggest that the max number of devices is a backend
> property on xenstore.
Instead of setting the number of mt devices front can easily
find this value by reading "mt-%d" XenStore entries.
The requirement in the protocol is that "...%d being
a sequential number of the virtual input device". Thus,
first entry which we fail to sequentially read will
indicate end of configured devices. Of course,
xenbus_scanf return value needs to be checked if it
has failed because there is no such value in XenStore
or for any other reason.

> > +/* Sent when a new touch is made: touch is assigned a unique contact
> > + * ID, sent with this and consequent events related to this touch.
> > + * Contact ID will be reused after XENKBD_MT_EV_UP event.
> Will be reused or can be reused?
I would probably say "may be reused" as it depends on how
and if new touches/contacts are made
>   Please provide an example of a Contact
> ID lifecycle.
Do you want it to be described in the protocol or just here?
If the latter then, for example, as Wayland documentation
describes it [1]:
"Touch interactions can consist of one or more contacts.
 For each contact, a series of events is generated, starting
 with a down event, followed by zero or more motion events,
 and ending with an up event. Events relating to the same
 contact point can be identified by the ID of the sequence."
So, if there is contact/touch a free Contact ID is assigned to
this contact(sequence) and it is "released" when contact is
done, e.g. after up event. From this point contact ID may be
reused.

I was basing the mt additions to the protocol on Wayland [1],
Linux [2] and Windows [3] multi-touch support, so those may
better explain the idea
> What is the max Contact ID?
/* contact ID, [0; num-contacts - 1] */
num-contacts - number of simultaneous touches supported
> > + */
> > +#define XENKBD_MT_EV_DOWN   0
> > +/* Touch point has been released */
> > +#define XENKBD_MT_EV_UP     1
> > +/* Touch point has changed its coordinate(s) */
> > +#define XENKBD_MT_EV_MOTION 2
> > +/* Input synchronization event: shows end of a set of events
> > + * which logically belong together.
> > + */
> > +#define XENKBD_MT_EV_SYN    3
> > +/* Touch point has changed its shape. Shape is approximated by an ellipse
> > + * through the major and minor axis lengths: major is the longer diameter
> > + * of the ellipse and minor is the shorter one. Center of the ellipse is
> > + * reported via XENKBD_MT_EV_DOWN/XENKBD_MT_EV_MOTION events.
> > + */
> > +#define XENKBD_MT_EV_SHAPE  4
> > +/* Touch point's shape has changed its orientation: calculated as a clockwise
> > + * angle between the major axis of the ellipse and positive Y axis in degrees,
> > + * [-180; +180].
> > + */
> > +#define XENKBD_MT_EV_ORIENT 5
> > +
> > +struct xenkbd_mtouch {
> > +    uint8_t type;             /* XENKBD_TYPE_MTOUCH */
> > +    uint8_t dev_idx;          /* index of the multi-touch device */
> > +    uint8_t event_type;       /* XENKBD_MT_EV_??? */
> > +    uint8_t reserved;         /* reserved for the future use */
> > +    int32_t contact_id;       /* contact ID, [0; num-contacts - 1] */
> > +    union {
> > +        /* XENKBD_MT_EV_DOWN/XENKBD_MT_EV_MOTION */
> > +        struct {
> > +            int32_t abs_x;    /* absolute X position, pixels */
> > +            int32_t abs_y;    /* absolute Y position, pixels */
> > +        } pos;
> > +        /* XENKBD_MT_EV_SHAPE */
> > +        struct {
> > +            uint32_t major;   /* length of the major axis, pixels */
> > +            uint32_t minor;   /* length of the minor axis, pixels */
> > +        } shape;
> > +        /* XENKBD_MT_EV_ORIENT */
> > +        uint16_t orientation; /* clockwise angle of the major axis */
> > +    } u;
> > +};
> Need a binary representation.
Could you please elaborate more on this, this is not
clear to me.
> >   #define XENKBD_IN_EVENT_SIZE 40
> >   
> >   union xenkbd_in_event
> > @@ -76,6 +139,7 @@ union xenkbd_in_event
> >       struct xenkbd_motion motion;
> >       struct xenkbd_key key;
> >       struct xenkbd_position pos;
> > +    struct xenkbd_mtouch mtouch;
> >       char pad[XENKBD_IN_EVENT_SIZE];
> >   };
> >   
> > --
> > 2.7.4
Related question: what if I also extend existing
xenkbd_motion/key/position by adding uint32(8?)_t dev_idx
at the bottom of the structures, e.g.

struct xenkbd_motion {
    uint8_t type;        /* XENKBD_TYPE_MOTION */
    int32_t rel_x;        /* relative X motion */
    int32_t rel_y;        /* relative Y motion */
    int32_t rel_z;        /* relative Z motion (wheel) */
    uint32_t dev_idx; or uint8_t dev_idx;
};

This way:
1. Existing fronts/backs will not be affected
2. New fronts/backs may use this field to serve
multiple keyboards and/or pointer devices via single ring

Should I make this change as a dedicated patch or do you think
it can be in the same one for mt?

Thank you,
Oleksandr
[1] 
https://cgit.freedesktop.org/wayland/wayland/tree/protocol/wayland.xml#n2196
[2] https://www.kernel.org/doc/Documentation/input/multi-touch-protocol.txt
[3] 
https://msdn.microsoft.com/en-us/library/jj151564(v=vs.85).aspx#optional_hid_usages
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel