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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 01/04/2017 08:23 PM, Stefano Stabellini wrote:
> On Wed, 4 Jan 2017, Oleksandr Andrushchenko wrote:
> > First of all, thank you for comments
> You are welcome :-)
>
>
> > 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"
> I don't think I understand what you wrote here. To clarify, I meant
> that the doc should say what is the theoretical maximum for width and
> height, for example INT32_MAX.
How about:
 *   o width - width of the touch area in pixels, in
 *       [INT_LEAST32_MIN; INT32_MAX] range
 *   o height - height of the touch area in pixels, in
 *       [INT_LEAST32_MIN; INT32_MAX] range
> > > 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.
> I think you'll find it is going to be simpler and faster for little
> extra memory costs.
Well, I have implemented that yesterday and after some cleanup
it will look ok
> > Will also remove
> >    uint8_t dev_idx;        /* index of the multi-touch device */
> > as every device will have its own ring.
> Right
>
>
> > 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.
> It's page-gref that is superfluous. I don't know why the Linux frontend
> writes it, in fact the QEMU backend doesn't even read it.
I'll keep it for consistency. I have refactored the
original kbdfront so there is common code for ring
and event channel handling which creates all the above.
If we decide that page-ref can be dropped it will
be dropped for all the devices at a time.
> > > > +/* 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.
> This is very useful info for people not familiar with Wayland. Please
> write this in the doc, and link to the Wayland documentation.
I have put some description, but not sure if it is a good
idea to put links to Wayland or any other documentation
from the Web: it might happen that in a little while the
link disappears or changes
> > 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.
> Sure, sorry. I meant something like:
>
>     0    1    2    3    4
>     +----+----+----+----+
>     |type|dev_|even|res |
>     +----+----+----+----+
>
> I know it is cumbersome, and I might not be a fun of it myself, but it
> is required for new Xen protocol changes. I wrote all of the binary
> representations manually but if you find a tool to do it, please let me
> know :-)
Now it's clear. I already did it (manually for sndif + displif),
so I can also do this for kbdif. The only concern I have is that
if I put this for my changes, then the whole protocol will
look inconsistent. So, it probably needs to be addressed
in two steps:
a) Adding the changes
b) Refactoring kbdif
 o put binary descriptions
 o describe XenStore entries
 o add workflow?
 o explicitly define reserved fields of all the structures
 o ?
> > > >    #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?
> I thought we decided to use one ring per device?
per multi-touch device - yes
existing kbd and ptr left unchanged
> If so, why would you
> want to introduce dev_idx in xenkbd_motion? Also, where would the list
> of available devices be described? The current xenstore info assumes one
> ring per device.
This is not exactly true. Currently, both kbd and ptr
devices share the same ring, so adding dev_idx will help
identifying which device instance an incoming event
belongs to.
> In any case, it should be separate from the mt discussion.
Agree, I see these topics for discussion
(at least but not at last):
1. Extending existing protocol + front (back?)
in a non-intrusive method in order to support
multiple mice + keyboards
2. Updating kbdif document to reflect
new trends in documenting protocols.
> > 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