| Message ID | 1483457954-5566-2-git-send-email-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.01.17 at 16:39, <andr2000@gmail.com> wrote: > --- 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 > + */ > +#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 Why is this limit needed? There's no use of it within the other interface additions you make. Jan
On 01/03/2017 06:28 PM, Jan Beulich wrote: >>>> On 03.01.17 at 16:39, <andr2000@gmail.com> wrote: >> --- 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 >> + */ >> +#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 > Why is this limit needed? There's no use of it within the other > interface additions you make. > > Jan Well, the only reason for that was a shy attempt to somewhat simplify changes to the existing frontend, e.g. handling fixed number of mt input devices rather than allocating all those dynamically, finding the number of devices configured at run-time etc. I will happily remove this limitation though Oleksandr
On Tue, 3 Jan 2017, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> > > Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> > --- > 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. 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. > + */ > +#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. > +/* 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? Please provide an example of a Contact ID lifecycle. What is the max Contact ID? > + */ > +#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. > #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 >
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@epam.com> >> >> Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> >> --- >> 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
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@epam.com> > > > > > > Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> > > > --- > > > 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. > > 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. > 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. > > > +/* 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 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 :-) > > > #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? 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. In any case, it should be separate from the mt discussion. > 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 >
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@epam.com> >>>> >>>> Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> >>>> --- >>>> 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 >>
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@epam.com> > > > > > > > > > > Signed-off-by: Oleksandr Andrushchenko > > > > > <Oleksandr_Andrushchenko@epam.com> > > > > > --- > > > > > 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 > > > >
On 01/05/2017 09:19 PM, Stefano Stabellini wrote: > 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@epam.com> >>>>>> >>>>>> Signed-off-by: Oleksandr Andrushchenko >>>>>> <Oleksandr_Andrushchenko@epam.com> >>>>>> --- >>>>>> 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] > even UINT32_MAX I think (width an height will be uint32_t) >>>>> 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 will >>>> 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. I'll try > >>>>>> #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. > see below >>> 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. well, what I am currently playing with is: o 1 ring for kbd and ptr (left unchanged) o 1 ring per mt device total number of rings is 1 + num_mt_devices So, kbd+ptr and mt(s) are totally independent For now, I will not add any index to kbd+ptr and will add if need be >> 2. Updating kbdif document to reflect >> new trends in documenting protocols. > This would be great, thanks! I'll work on 2 patches for kbdif then: 1. Update existing kbdif with binary description etc. 2. Add multi-touch > >>>> 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 >>>>
> 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 :-) Letting you know ;) there is a project in Python which can do this [1] I'm going to give it a try [1] https://github.com/luismartingarcia/protocol
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 + */ +#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 + +/* 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. + */ +#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; +}; + #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]; };