Xen project Mailing List

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

On Thu, 5 Jan 2017, Oleksandr Andrushchenko wrote: > 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 Yes, that's what I had in mind. But I think that height and width shouldn't have negative values here (movements should, but size measurements shouldn't). Maybe: width [0, INT32_MAX] height [0, INT32_MAX] > > > > 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 Good! > > > 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. OK. Unfortunately the xenstore protocol for xenkbd is not documented, so we'll never be able to get rid of it :-( > > > > > +/* 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 know, but it is better to have a useful link for a little while, then not having it at all. > > > 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 ? I wouldn't worry too much about inconsistency. As for any code style changes, often the new style is introduced alongside the old style. But it would be great if you took the time to introduce binary descriptions for the existing protocol too. > > > > > #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. Today 1 keyboard and 1 mouse share the same xenkbd ring. With the addition of multitouch devices, one could have 1 keyboard, 1 mouse and 1 mt device. Is that what you are saying? In that regard, yes, I think it is useful to add a dev_idx backward compatible field to the protocol to distinguish mouse events from mt events for example. > > 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 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. > 2. Updating kbdif document to reflect > new trends in documenting protocols. This would be great, thanks! > > > > > 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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.