Re: [Xen-devel] [PATCH v1 2/2] xen/kbdif: add multi-touch support

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 24 Jan 2017, Oleksandr Andrushchenko wrote:
> On 01/23/2017 09:49 PM, Stefano Stabellini wrote:
> > On Sat, 21 Jan 2017, Oleksandr Andrushchenko wrote:
> > > In the mail-thread you mentioned above there is a picture of
> > > the xenstore entries and conclusion:
> > > 
> > > 1. No change to the existing kbd+ptr:
> > > 1.1. They still share a dedicated (single) connection
> > > channel as they did before multi-touch: page-ref + event-channel:
> > > 
> > > /local/domain/2/device/vkbd/0/page-ref = "1248025"
> > > /local/domain/2/device/vkbd/0/page-gref = "336"
> > > /local/domain/2/device/vkbd/0/event-channel = "43"
> > > 
> > > 1.2. No multi-touch events via that connection
> > > 1.3. Old backends/frontends will see no difference
> > > after multi-touch
> > > 
> > > 2. Each multi-touch device has its own:
> > > 2.1. Configuration (width x height, num-contacts)
> > > 2.2. Own connection channel (page-ref, event-channel)
> > > 
> > > /local/domain/2/device/vkbd/0/mtouch/0/width = "3200"
> > > /local/domain/2/device/vkbd/0/mtouch/0/height = "3200"
> > > /local/domain/2/device/vkbd/0/mtouch/0/num-contacts = "5"
> > > /local/domain/2/device/vkbd/0/mtouch/0/page-ref = "1252386"
> > > /local/domain/2/device/vkbd/0/mtouch/0/page-gref = "335"
> > > /local/domain/2/device/vkbd/0/mtouch/0/event-channel = "44"
> > > 
> > > /local/domain/2/device/vkbd/0/mtouch/1/width = "6400"
> > > /local/domain/2/device/vkbd/0/mtouch/1/height = "6400"
> > > /local/domain/2/device/vkbd/0/mtouch/1/num-contacts = "10"
> > > /local/domain/2/device/vkbd/0/mtouch/1/page-ref = "2376038"
> > > /local/domain/2/device/vkbd/0/mtouch/1/page-gref = "337"
> > > /local/domain/2/device/vkbd/0/mtouch/1/event-channel = "45"
> > > So, for the example above: 1 kbd + 1 ptr + 2 mt devices
> > > within a single driver, *3 connections*
> > > 
> > > > There are no ring-ref and event-channel under mtouch, are there?
> > > there are ring-refs and event-channels under mtouch *per multi-touch*
> > > device
> > Now, it is clear. Sorry it took so many back and forth.
> np
> > page-ref and event-channel should definitely be described under
> > "Multi-touch Device Parameters". When I asked you to remove them, I
> > meant not just from the description but also from the protocol. This is
> > where the confusion originated: in the current patch the two parameters
> > are not described, hence I assumed they didn't exist and you were
> > reusing the existing ring (/local/domain/2/device/vkbd/0/page-ref).
> understood
> > 
> > Aside from new message structs, which look fine to me, we have two
> > important choices to make:
> > 
> > a) number of multitouch devices per PV frontend/backend connection
> > (By frontend/backend connection, I mean by xenstore device,
> > such as /local/domain/2/device/vkbd/0.)
> > 
> > b) number of multitouch devices sharing the same ring
> > (By ring I mean page-ref + event channel.)
> > 
> > Both these choices need to be motivated. As I wrote in the past, I would
> > go for 1 multitouch device per frontend/backend connection, reusing the
> > existing ring (/local/domain/2/device/vkbd/0/page-ref and
> > /local/domain/2/device/vkbd/0/event-channel), because I assume that
> > there won't be many multitouch devices, less than 5,
> yes, I have 2 mt devices in my use-case
> > so we can afford to
> > create new PV devices for each of them.
> see at the bottom
> >   I am also assuming that it is
> > fine for them to share ring with the keyboard or pointer devices without
> > effects on performance because they should be all low frequency devices.
> sounds reasonable, but see below
> > The current proposal supports multiple multitouch devices per
> > frontend/backnend connection and allocates a new dedicated ring for each
> > multitouch device. Your proposal might very well be the best solution,
> > but why? The architectural choices need to be motivated. How many
> > multitouch devices is reasonable to expect to be present on a platform?
> > What is the multitouch events frequency we expect from them? The answer
> > to the first question motivates choice a), the answer to the second
> > question motivates choice b). Please add them both to the protocol
> > description.  It is OK to add the explanation to kbdif.h. You also
> > mentioned something about multiple PV devices causing troubles to Linux.
> > If that is an issue somehow, please add info about that too.
> Well, all the above sounds reasonable, some comments:
> 1. Event frequency: here we are talking about 60-120(?)Hz
> scan frequency of a touch screen generating at least 2
> events for each contact: frame (syn) and position.
> So, we can estimate for 10 fingers the value to be
> 60 * 2 * 10 = 1200 events a second per multi-touch device.
> Of course, this is rough estimate which I believe would
> be smaller in real life.
> 
> 2. "create new PV devices for each of them"
> 2.1. Driver
> This is something which you are seeing in xenstore as
>   /local/domain/2/device/vkbd/0
> 0 - being index of the *driver*.
> And the challenge here is that if you want multiple *devices*
> to be allocated in this way:
>   /local/domain/2/device/vkbd/0
>   /local/domain/2/device/vkbd/1
>   /local/domain/2/device/vkbd/2
> then you have to register multiple *DRIVERS*.
> What we normally do in the PV front driver is we call
> *xenbus_register_frontend*,passing a structure
> *struct xenbus_driver* with *.ids* set to a single entry "vkbd".
> 
> I am a little bit confused here about front implementation:
> how to dynamically register multiple "vkbd" *drivers* depending on
> configuration, so we have the layout you mention.

You shouldn't have to do anything in Linux, it should happen naturally.
The toolstack (libxl, xl) should configure two or more vkbd devices on
xenstore, for example:

   /local/domain/2/device/vkbd/0
   /local/domain/2/device/vkbd/1

then the Linux xenbus frontend should automatically allocate a driver
for each of them. xenkbd_init should be called once, but you would get
two xenkbd_probe calls, with different struct xenbus_device* arguments,
one for each vkbd device.

I don't think that the toolstack is able to create multiple vkbd devices
today (that's where you would need to make your modifications), but it
certainly can create multiple vifs:

vif=[ 'bridge=xenbr0', 'bridge=xenbr1' ]

You can experiment with that to see how the xenbus driver initialization
works. You should see multiple drivers/net/xen-netfront.c:netfront_probe
calls. Then, you could hack libxl to create two vkbd devices and see
what happens. Linux should pick them up automatically.

Does that answer your question?


> 2.2. Input device
> You are virtually not limited on number of *DEVICES within a DRIVER*,
> but they all are under "/local/domain/2/device/vkbd/0" entry.
> That being said, under the same xenstore entry you can allocate
> ptr+kbd+N*mt input devices.
> 
> Bottom line:
> ================
> a) From (1), probably it would be fine to send these events
> over the existing ring (kbd+ptr), but mt event structures
> need an index to indicate which mt device is the owner of
> the event
> b) We may still want to have a dedicated ring either for all
> mt devices (depending on event frequency we consider reasonable)
> or per mt-device.

The events frequency you describe is high enough that it's better not to
share a ring across multiple mt devices. However, it might still be OK
for a single mt device to share a ring with kbd and ptr as ptr and kbd
are lower frequency.


> c) I see no simple way to support multiple drivers comparing to
> multiple devices (*DISCLAIMER* which is natural to Linux).
> 
> All,
> what do you think on the above? What is the approach we are going to
> take?

Thanks for the info, this is very helpful!

I would appreciate feedback from others as well, but I think that we
need one ring per mt device, maybe shared with ptr and kbd. Having one
xenstore device per mt device would be more in line with the usual way
of doing things, and would be my preference, but we need to figure out
if there is a problem with that approach first. Let me know if you find
any issues with that.

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