On Fri, Apr 21, 2017 at 09:40:36AM +0300, Oleksandr Andrushchenko wrote: > Hi, Dmitry! > > On 04/21/2017 05:10 AM, Dmitry Torokhov wrote: > >Hi Oleksandr, > > > >On Thu, Apr 13, 2017 at 02:38:04PM +0300, Oleksandr Andrushchenko wrote: > >>From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> > >> > >>Extend xen_kbdfront to provide multi-touch support > >>to unprivileged domains. > >> > >>Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> > >>--- > >> drivers/input/misc/xen-kbdfront.c | 142 +++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 140 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c > >>index 01c27b4c3288..e5d064aaa237 100644 > >>--- a/drivers/input/misc/xen-kbdfront.c > >>+++ b/drivers/input/misc/xen-kbdfront.c > >>@@ -17,6 +17,7 @@ > >> #include <linux/errno.h> > >> #include <linux/module.h> > >> #include <linux/input.h> > >>+#include <linux/input/mt.h> > >> #include <linux/slab.h> > >> #include <asm/xen/hypervisor.h> > >>@@ -34,11 +35,14 @@ > >> struct xenkbd_info { > >> struct input_dev *kbd; > >> struct input_dev *ptr; > >>+ struct input_dev *mtouch; > >> struct xenkbd_page *page; > >> int gref; > >> int irq; > >> struct xenbus_device *xbdev; > >> char phys[32]; > >>+ /* current MT slot/contact ID we are injecting events in */ > >>+ int mtouch_cur_contact_id; > >> }; > >> enum { KPARAM_X, KPARAM_Y, KPARAM_CNT }; > >>@@ -47,6 +51,12 @@ module_param_array(ptr_size, int, NULL, 0444); > >> MODULE_PARM_DESC(ptr_size, > >> "Pointing device width, height in pixels (default 800,600)"); > >>+enum { KPARAM_MT_X, KPARAM_MT_Y, KPARAM_MT_CNT }; > >>+static int mtouch_size[KPARAM_MT_CNT] = { XENFB_WIDTH, XENFB_HEIGHT }; > >>+module_param_array(mtouch_size, int, NULL, 0444); > >>+MODULE_PARM_DESC(ptr_size, > >>+ "Multi-touch device width, height in pixels (default 800,600)"); > >>+ > >Why do you need separate module parameters for multi-touch device? > please see below > > > >> static int xenkbd_remove(struct xenbus_device *); > >> static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *); > >> static void xenkbd_disconnect_backend(struct xenkbd_info *); > >>@@ -100,6 +110,60 @@ static irqreturn_t input_handler(int rq, void *dev_id) > >> input_report_rel(dev, REL_WHEEL, > >> -event->pos.rel_z); > >> break; > >>+ case XENKBD_TYPE_MTOUCH: > >>+ dev = info->mtouch; > >>+ if (unlikely(!dev)) > >>+ break; > >>+ if (unlikely(event->mtouch.contact_id != > >>+ info->mtouch_cur_contact_id)) { > >Why is this unlikely? Does contact ID changes once in 1000 packets or > >even less? > Mu assumption was that regardless of the fact that we are multi-touch > device still single touches will come in more frequently > But I can remove *unlikely* if my assumption is not correct I think the normal expectation is that "unlikely" is supposed for something that happens once in a blue moon, so I'd rather remove it. > >>+ info->mtouch_cur_contact_id = > >>+ event->mtouch.contact_id; > >>+ input_mt_slot(dev, event->mtouch.contact_id); > >>+ } > >>+ switch (event->mtouch.event_type) { > >>+ case XENKBD_MT_EV_DOWN: > >>+ input_mt_report_slot_state(dev, MT_TOOL_FINGER, > >>+ true); Should we establish tool event? We have MT_TOOL_PEN, etc. > >>+ input_event(dev, EV_ABS, ABS_MT_POSITION_X, > >>+ event->mtouch.u.pos.abs_x); > >>+ input_event(dev, EV_ABS, ABS_MT_POSITION_Y, > >>+ event->mtouch.u.pos.abs_y); > >>+ input_event(dev, EV_ABS, ABS_X, > >>+ event->mtouch.u.pos.abs_x); > >>+ input_event(dev, EV_ABS, ABS_Y, > >>+ event->mtouch.u.pos.abs_y); > >>+ break; > >>+ case XENKBD_MT_EV_UP: > >>+ input_mt_report_slot_state(dev, MT_TOOL_FINGER, > >>+ false); > >>+ break; > >>+ case XENKBD_MT_EV_MOTION: > >>+ input_event(dev, EV_ABS, ABS_MT_POSITION_X, > >>+ event->mtouch.u.pos.abs_x); > >>+ input_event(dev, EV_ABS, ABS_MT_POSITION_Y, > >>+ event->mtouch.u.pos.abs_y); > >>+ input_event(dev, EV_ABS, ABS_X, > >>+ event->mtouch.u.pos.abs_x); > >>+ input_event(dev, EV_ABS, ABS_Y, > >>+ event->mtouch.u.pos.abs_y); > >>+ break; > >>+ case XENKBD_MT_EV_SYN: > >>+ input_mt_sync_frame(dev); > >>+ break; > >>+ case XENKBD_MT_EV_SHAPE: > >>+ input_event(dev, EV_ABS, ABS_MT_TOUCH_MAJOR, > >>+ event->mtouch.u.shape.major); > >>+ input_event(dev, EV_ABS, ABS_MT_TOUCH_MINOR, > >>+ event->mtouch.u.shape.minor); > >>+ break; > >>+ case XENKBD_MT_EV_ORIENT: > >>+ input_event(dev, EV_ABS, ABS_MT_ORIENTATION, > >>+ event->mtouch.u.orientation); > >>+ break; > >>+ } > >>+ /* only report syn when requested */ > >>+ if (event->mtouch.event_type != XENKBD_MT_EV_SYN) > >>+ dev = NULL; > >> } > >> if (dev) > >> input_sync(dev); > >>@@ -115,9 +179,9 @@ static int xenkbd_probe(struct xenbus_device *dev, > >> const struct xenbus_device_id *id) > >> { > >> int ret, i; > >>- unsigned int abs; > >>+ unsigned int abs, touch; > >> struct xenkbd_info *info; > >>- struct input_dev *kbd, *ptr; > >>+ struct input_dev *kbd, *ptr, *mtouch; > >> info = kzalloc(sizeof(*info), GFP_KERNEL); > >> if (!info) { > >>@@ -152,6 +216,17 @@ static int xenkbd_probe(struct xenbus_device *dev, > >> } > >> } > >>+ touch = xenbus_read_unsigned(dev->nodename, > >>+ XENKBD_FIELD_FEAT_MTOUCH, 0); > >>+ if (touch) { > >>+ ret = xenbus_write(XBT_NIL, dev->nodename, > >>+ XENKBD_FIELD_REQ_MTOUCH, "1"); > >>+ if (ret) { > >>+ pr_warning("xenkbd: can't request multi-touch"); > >>+ touch = 0; > >>+ } > >>+ } > >>+ > >> /* keyboard */ > >> kbd = input_allocate_device(); > >> if (!kbd) > >>@@ -208,6 +283,67 @@ static int xenkbd_probe(struct xenbus_device *dev, > >> } > >> info->ptr = ptr; > >>+ /* multi-touch device */ > >>+ if (touch) { > >>+ int num_cont, width, height; > >>+ > >>+ mtouch = input_allocate_device(); > >>+ if (!mtouch) > >>+ goto error_nomem; > >>+ > >>+ num_cont = xenbus_read_unsigned(info->xbdev->nodename, > >>+ XENKBD_FIELD_MT_NUM_CONTACTS, > >>+ 1); Should we refuse MT devices with number of contacts less than 2? > >>+ width = xenbus_read_unsigned(info->xbdev->nodename, > >>+ XENKBD_FIELD_MT_WIDTH, > >>+ XENFB_WIDTH); > >>+ height = xenbus_read_unsigned(info->xbdev->nodename, > >>+ XENKBD_FIELD_MT_HEIGHT, > >>+ XENFB_HEIGHT); > >Curious why you need separate parameters here too... > This is because mt parameters are different from ptr > in a way that they are configurable per front driver's > instance rather than per backend, e.g. in XenStore: > > /local/domain/0/backend/vkbd/1/0/width = "1920" > /local/domain/0/backend/vkbd/1/0/height = "1080" > > /local/domain/1/device/vkbd/0/multi-touch-width = "1920" > /local/domain/1/device/vkbd/0/multi-touch-height = "1080" > /local/domain/1/device/vkbd/0/multi-touch-num-contacts = "10" > > /local/domain/1/device/vkbd/1/multi-touch-width = "800" > /local/domain/1/device/vkbd/1/multi-touch-height = "600" > /local/domain/1/device/vkbd/1/multi-touch-num-contacts = "3" > > The main reason for such configuration is that you can > configure multiple mt input devices even for the same guest > with different resolutions which may not match those > configured for ptr. > (In my use-case I use new displif protocol [1] in conjunction > with mt input devices and the corresponding backend is not > QEMU's xenfb) I see. > > As to module parameters, I added those to be consistent with > ptr device. Do you think we can live without them and > do you want me to remove them? Yes, I think we better. I am also confused by the way you are handling the module parameters. It looks to me you update them with data passed from the backend, but never use the data... > >>+ > >>+ mtouch->name = "Xen Virtual Multi-touch"; > >>+ mtouch->phys = info->phys; > >>+ mtouch->id.bustype = BUS_PCI; > >>+ mtouch->id.vendor = 0x5853; > >>+ mtouch->id.product = 0xfffd; > >>+ > >>+ __set_bit(EV_ABS, mtouch->evbit); > >>+ __set_bit(EV_KEY, mtouch->evbit); > >>+ __set_bit(BTN_TOUCH, mtouch->keybit); > >>+ > >>+ input_set_abs_params(mtouch, ABS_X, > >>+ 0, width, 0, 0); > >>+ input_set_abs_params(mtouch, ABS_Y, > >>+ 0, height, 0, 0); > >>+ input_set_abs_params(mtouch, ABS_PRESSURE, > >>+ 0, 255, 0, 0); > >>+ > >>+ input_set_abs_params(mtouch, ABS_MT_TOUCH_MAJOR, > >>+ 0, 255, 0, 0); > >>+ input_set_abs_params(mtouch, ABS_MT_POSITION_X, > >>+ 0, width, 0, 0); > >>+ input_set_abs_params(mtouch, ABS_MT_POSITION_Y, > >>+ 0, height, 0, 0); > >>+ input_set_abs_params(mtouch, ABS_MT_PRESSURE, > >>+ 0, 255, 0, 0); > >>+ > >>+ input_mt_init_slots(mtouch, num_cont, 0); We need error handling here. Also, it would be nice if we set INPUT_MT_* flags here, so that userspace had better chance of figuring how to handle the device. > >>+ > >>+ mtouch_size[KPARAM_MT_X] = width; > >>+ mtouch_size[KPARAM_MT_Y] = height; > >>+ info->mtouch_cur_contact_id = -1; > >>+ > >>+ ret = input_register_device(mtouch); > >>+ if (ret) { > >>+ input_free_device(mtouch); > >>+ xenbus_dev_fatal(info->xbdev, ret, > >>+ "input_register_device(mtouch)"); > >>+ goto error; > >>+ } > >>+ info->mtouch_cur_contact_id = -1; > >>+ info->mtouch = mtouch; > >>+ } > >>+ > >> ret = xenkbd_connect_backend(dev, info); > >> if (ret < 0) > >> goto error; > >>@@ -240,6 +376,8 @@ static int xenkbd_remove(struct xenbus_device *dev) > >> input_unregister_device(info->kbd); > >> if (info->ptr) > >> input_unregister_device(info->ptr); > >>+ if (info->mtouch) > >>+ input_unregister_device(info->mtouch); > >> free_page((unsigned long)info->page); > >> kfree(info); > >> return 0; > >>-- > >>2.7.4 > >> Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html