[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC] kbdif: add multi-touch support
- To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
- From: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
- Date: Thu, 5 Jan 2017 22:20:37 +0200
- Cc: lars.kurth@xxxxxxxxxx, vlad.babchuk@xxxxxxxxx, dario.faggioli@xxxxxxxxxx, julien.grall@xxxxxxx, andrii.anisov@xxxxxxxxx, olekstysh@xxxxxxxxx, al1img@xxxxxxxxx, JBeulich@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx, joculator@xxxxxxxxx
- Delivery-date: Thu, 05 Jan 2017 20:20:56 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
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@xxxxxxxx>
Signed-off-by: Oleksandr Andrushchenko
<Oleksandr_Andrushchenko@xxxxxxxx>
---
xen/include/public/io/kbdif.h | 64
+++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/xen/include/public/io/kbdif.h
b/xen/include/public/io/kbdif.h
index 2d2aebd..ad94b53 100644
--- a/xen/include/public/io/kbdif.h
+++ b/xen/include/public/io/kbdif.h
@@ -45,6 +45,19 @@
*/
#define XENKBD_TYPE_POS 4
+/*
+ * Multi-touch event
+ * Capable backend sets feature-multi-touch in xenstore.
+ * Frontend requests feature by setting request-multi-touch in
xenstore.
+ * Frontend supports up to XENKBD_MT_NUM_DEV virtual multi-touch
input
devices,
+ * configured by the backend in xenstore under mt-%d folder, %d being
+ * a sequential number of the virtual input device:
+ * o num-contacts - number of simultaneous touches supported
+ * o width - width of the touch area in pixels
+ * o height - height of the touch area in pixels
Please write down the max width and height supported by the protocol,
keeping in mind that motion events below use int32_t for coordinates.
I will put "width(height) of the... in pixels, 32-bit signed integer"
I don't think I understand what you wrote here. To clarify, I meant
that the doc should say what is the theoretical maximum for width and
height, for example INT32_MAX.
How about:
* o width - width of the touch area in pixels, in
* [INT_LEAST32_MIN; INT32_MAX] range
* o height - height of the touch area in pixels, in
* [INT_LEAST32_MIN; INT32_MAX] range
Yes, that's what I had in mind. But I think that height and width
shouldn't have negative values here (movements should, but size
measurements shouldn't). Maybe:
width [0, INT32_MAX]
height [0, INT32_MAX]
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
