[RFC] kbdif: add multi-touch support

[RFC] kbdif: add multi-touch support

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>

Hi, all!

I am working on adding multi-touch support to the existing
kbdif protocol and would like to request for comments on the
below patch.
The aim is to provide multi-touch support to unprivileged
domains which may employ multiple virtual displays.
Thus, the protocol should handle multiple virtual touch input
devices withing the same domain, so those can be mapped to
different virtual displays.

Thank you in advance,
Oleksandr

Oleksandr Andrushchenko (1):
  kbdif: add multi-touch support

 xen/include/public/io/kbdif.h | 64 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

-- 
2.7.4


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

[RFC] kbdif: add multi-touch support

[Oleksandr Andrushchenko]

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
+ */
+#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];
 };
 
-- 
2.7.4


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

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

[Jan Beulich]

>>> 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


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

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

[Oleksandr Andrushchenko]

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


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

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

[Stefano Stabellini]

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
> 

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

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

[Oleksandr Andrushchenko]

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

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

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

[Stefano Stabellini]

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
> 

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

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

[Oleksandr Andrushchenko]

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
>>


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

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

[Stefano Stabellini]

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
> > > 
> 

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

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

[Oleksandr Andrushchenko]

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
>>>>


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

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

[Oleksandr Andrushchenko]

> 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


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