| Message ID | 20170629081719.GA21557@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Dmitry! First of all thank you for both the comments and the patch On 06/29/2017 11:17 AM, Dmitry Torokhov wrote: > Hi Oleksandr, > > On Fri, Jun 23, 2017 at 09:09:55AM +0300, Oleksandr Andrushchenko wrote: >> + switch (event->mtouch.event_type) { >> + case XENKBD_MT_EV_DOWN: >> + input_mt_report_slot_state(dev, MT_TOOL_FINGER, >> + true); >> + 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); > I was looking at this and realized that this breaks the single touch > emulation for MT interface: for ST you are supposed to report the oldest > contact, here you report data for all of them. Luckily > input_mt_report_pointer_emulation() that is called as part of > input_mt_sync_frame() reports the correct ABS_X/ABS_Y data and fixes > that for you. > > We should simply remove reporting ABS_X/ABS_Y here and in > XENKBD_MT_EV_MOTION as well. > >> + >> + input_set_capability(mtouch, EV_KEY, BTN_TOUCH); >> + 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); > This is done automatically by input_mt_init_slots() when called with > INPUT_MT_DIRECT (as in your case) or INPUT_MT_POINTER, so this can be > removed as well. Great, I was not actually convinced that ABS is really needed to be put here while dealing with MT devices, so the above can be removed > Does the patch below (on top of yours) work for you? Unfortunately I didn't have time to test the patch today, but will try to do so tomorrow. Beside that, do you think that the removals above should go into my patch and the rest of yours (it looks like needed refactoring to me) should go into a separate one, not named "MT support fixups", but rather "Xen input driver refactoring"? Because part of the changes seems to be MT relevant and part is pure refactoring. If so, do you want me to rework your patch with these changes and add on top of mine (I will put your signed off) or you will handle it on your own? > Thanks. > Thank you, Oleksandr
On June 29, 2017 11:40:30 AM PDT, Oleksandr Andrushchenko <andr2000@gmail.com> wrote: >Hi, Dmitry! > >First of all thank you for both the comments and the patch > >On 06/29/2017 11:17 AM, Dmitry Torokhov wrote: >> Hi Oleksandr, >> >> On Fri, Jun 23, 2017 at 09:09:55AM +0300, Oleksandr Andrushchenko >wrote: >>> + switch (event->mtouch.event_type) { >>> + case XENKBD_MT_EV_DOWN: >>> + input_mt_report_slot_state(dev, MT_TOOL_FINGER, >>> + true); >>> + 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); >> I was looking at this and realized that this breaks the single touch >> emulation for MT interface: for ST you are supposed to report the >oldest >> contact, here you report data for all of them. Luckily >> input_mt_report_pointer_emulation() that is called as part of >> input_mt_sync_frame() reports the correct ABS_X/ABS_Y data and fixes >> that for you. >> >> We should simply remove reporting ABS_X/ABS_Y here and in >> XENKBD_MT_EV_MOTION as well. >> >>> + >>> + input_set_capability(mtouch, EV_KEY, BTN_TOUCH); >>> + 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); >> This is done automatically by input_mt_init_slots() when called with >> INPUT_MT_DIRECT (as in your case) or INPUT_MT_POINTER, so this can be >> removed as well. >Great, I was not actually convinced that ABS is really needed >to be put here while dealing with MT devices, >so the above can be removed >> Does the patch below (on top of yours) work for you? >Unfortunately I didn't have time to test the patch today, but will try >to do so tomorrow. > >Beside that, do you think that the removals above should go into my >patch >and the rest of yours (it looks like needed refactoring to me) should >go >into >a separate one, not named "MT support fixups", but rather "Xen input >driver refactoring"? Because part of the changes seems to be MT >relevant >and part is pure refactoring. >If so, do you want me to rework your patch with these changes and add >on >top of mine (I will put your signed off) or you will handle it on your >own? I was planning on simply folding my changes into your patch and calling it a day, unless your testing would show there is an issue. It wasn't intended to be a separate patch in it's own right, I simply sent it out this way to show what exactly I was changing. Thanks.
Hi, Dmitry! On 06/29/2017 10:24 PM, Dmitry Torokhov wrote: > On June 29, 2017 11:40:30 AM PDT, Oleksandr Andrushchenko <andr2000@gmail.com> wrote: >> Hi, Dmitry! >> >> First of all thank you for both the comments and the patch >> >> On 06/29/2017 11:17 AM, Dmitry Torokhov wrote: >>> Hi Oleksandr, >>> >>> On Fri, Jun 23, 2017 at 09:09:55AM +0300, Oleksandr Andrushchenko >> wrote: >>>> + switch (event->mtouch.event_type) { >>>> + case XENKBD_MT_EV_DOWN: >>>> + input_mt_report_slot_state(dev, MT_TOOL_FINGER, >>>> + true); >>>> + 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); >>> I was looking at this and realized that this breaks the single touch >>> emulation for MT interface: for ST you are supposed to report the >> oldest >>> contact, here you report data for all of them. Luckily >>> input_mt_report_pointer_emulation() that is called as part of >>> input_mt_sync_frame() reports the correct ABS_X/ABS_Y data and fixes >>> that for you. >>> >>> We should simply remove reporting ABS_X/ABS_Y here and in >>> XENKBD_MT_EV_MOTION as well. >>> >>>> + >>>> + input_set_capability(mtouch, EV_KEY, BTN_TOUCH); >>>> + 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); >>> This is done automatically by input_mt_init_slots() when called with >>> INPUT_MT_DIRECT (as in your case) or INPUT_MT_POINTER, so this can be >>> removed as well. >> Great, I was not actually convinced that ABS is really needed >> to be put here while dealing with MT devices, >> so the above can be removed >>> Does the patch below (on top of yours) work for you? >> Unfortunately I didn't have time to test the patch today, but will try >> to do so tomorrow. >> >> Beside that, do you think that the removals above should go into my >> patch >> and the rest of yours (it looks like needed refactoring to me) should >> go >> into >> a separate one, not named "MT support fixups", but rather "Xen input >> driver refactoring"? Because part of the changes seems to be MT >> relevant >> and part is pure refactoring. >> If so, do you want me to rework your patch with these changes and add >> on >> top of mine (I will put your signed off) or you will handle it on your >> own? > I was planning on simply folding my changes into your patch and calling it a day, unless your testing would show there is an issue. I found no issue with the patches, but I have only tested that on ARM with our new kbd/ptr/mt backend [1]. I am not able to test that on x86 unfortunately, thus cannot confirm QEMU's backend is ok as well. What will be the next steps on my side to get MT in? > It wasn't intended to be a separate patch in it's own right, I simply sent it out this way to show what exactly I was changing. > > > Thanks. > Thank you, Oleksandr [1] https://github.com/xen-troops/displ_be
diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c index 9fa005038773..fa130e7b734c 100644 --- a/drivers/input/misc/xen-kbdfront.c +++ b/drivers/input/misc/xen-kbdfront.c @@ -60,6 +60,112 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *); * to do that. */ +static void xenkbd_handle_motion_event(struct xenkbd_info *info, + struct xenkbd_motion *motion) +{ + input_report_rel(info->ptr, REL_X, motion->rel_x); + input_report_rel(info->ptr, REL_Y, motion->rel_y); + if (motion->rel_z) + input_report_rel(info->ptr, REL_WHEEL, -motion->rel_z); + input_sync(info->ptr); +} + +static void xenkbd_handle_position_event(struct xenkbd_info *info, + struct xenkbd_position *pos) +{ + input_report_abs(info->ptr, ABS_X, pos->abs_x); + input_report_abs(info->ptr, ABS_Y, pos->abs_y); + if (pos->rel_z) + input_report_rel(info->ptr, REL_WHEEL, -pos->rel_z); + input_sync(info->ptr); +} + +static void xenkbd_handle_key_event(struct xenkbd_info *info, + struct xenkbd_key *key) +{ + struct input_dev *dev; + + if (test_bit(key->keycode, info->ptr->keybit)) { + dev = info->ptr; + } else if (test_bit(key->keycode, info->kbd->keybit)) { + dev = info->kbd; + } else { + pr_warn("unhandled keycode 0x%x\n", key->keycode); + return; + } + + input_report_key(dev, key->keycode, key->pressed); + input_sync(dev); +} + +static void xenkbd_handle_mt_event(struct xenkbd_info *info, + struct xenkbd_mtouch *mtouch) +{ + if (unlikely(!info->mtouch)) + return; + + if (mtouch->contact_id != info->mtouch_cur_contact_id) { + info->mtouch_cur_contact_id = mtouch->contact_id; + input_mt_slot(info->mtouch, mtouch->contact_id); + } + + switch (mtouch->event_type) { + case XENKBD_MT_EV_DOWN: + input_mt_report_slot_state(info->mtouch, MT_TOOL_FINGER, true); + /* fall through */ + + case XENKBD_MT_EV_MOTION: + input_report_abs(info->mtouch, ABS_MT_POSITION_X, + mtouch->u.pos.abs_x); + input_report_abs(info->mtouch, ABS_MT_POSITION_Y, + mtouch->u.pos.abs_y); + break; + + case XENKBD_MT_EV_SHAPE: + input_report_abs(info->mtouch, ABS_MT_TOUCH_MAJOR, + mtouch->u.shape.major); + input_report_abs(info->mtouch, ABS_MT_TOUCH_MINOR, + mtouch->u.shape.minor); + break; + + case XENKBD_MT_EV_ORIENT: + input_report_abs(info->mtouch, ABS_MT_ORIENTATION, + mtouch->u.orientation); + break; + + case XENKBD_MT_EV_UP: + input_mt_report_slot_state(info->mtouch, MT_TOOL_FINGER, false); + break; + + case XENKBD_MT_EV_SYN: + input_mt_sync_frame(info->mtouch); + input_sync(info->mtouch); + break; + } +} + +static void xenkbd_handle_event(struct xenkbd_info *info, + union xenkbd_in_event *event) +{ + switch (event->type) { + case XENKBD_TYPE_MOTION: + xenkbd_handle_motion_event(info, &event->motion); + break; + + case XENKBD_TYPE_KEY: + xenkbd_handle_key_event(info, &event->key); + break; + + case XENKBD_TYPE_POS: + xenkbd_handle_position_event(info, &event->pos); + break; + + case XENKBD_TYPE_MTOUCH: + xenkbd_handle_mt_event(info, &event->mtouch); + break; + } +} + static irqreturn_t input_handler(int rq, void *dev_id) { struct xenkbd_info *info = dev_id; @@ -70,98 +176,8 @@ static irqreturn_t input_handler(int rq, void *dev_id) if (prod == page->in_cons) return IRQ_HANDLED; rmb(); /* ensure we see ring contents up to prod */ - for (cons = page->in_cons; cons != prod; cons++) { - union xenkbd_in_event *event; - struct input_dev *dev; - event = &XENKBD_IN_RING_REF(page, cons); - - dev = info->ptr; - switch (event->type) { - case XENKBD_TYPE_MOTION: - input_report_rel(dev, REL_X, event->motion.rel_x); - input_report_rel(dev, REL_Y, event->motion.rel_y); - if (event->motion.rel_z) - input_report_rel(dev, REL_WHEEL, - -event->motion.rel_z); - break; - case XENKBD_TYPE_KEY: - dev = NULL; - if (test_bit(event->key.keycode, info->kbd->keybit)) - dev = info->kbd; - if (test_bit(event->key.keycode, info->ptr->keybit)) - dev = info->ptr; - if (dev) - input_report_key(dev, event->key.keycode, - event->key.pressed); - else - pr_warn("unhandled keycode 0x%x\n", - event->key.keycode); - break; - case XENKBD_TYPE_POS: - input_report_abs(dev, ABS_X, event->pos.abs_x); - input_report_abs(dev, ABS_Y, event->pos.abs_y); - if (event->pos.rel_z) - input_report_rel(dev, REL_WHEEL, - -event->pos.rel_z); - break; - case XENKBD_TYPE_MTOUCH: - dev = info->mtouch; - if (unlikely(!dev)) - break; - if (event->mtouch.contact_id != - info->mtouch_cur_contact_id) { - 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); - 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); - } + for (cons = page->in_cons; cons != prod; cons++) + xenkbd_handle_event(info, &XENKBD_IN_RING_REF(page, cons)); mb(); /* ensure we got ring contents */ page->in_cons = cons; notify_remote_via_irq(info->irq); @@ -216,7 +232,7 @@ static int xenkbd_probe(struct xenbus_device *dev, ret = xenbus_write(XBT_NIL, dev->nodename, XENKBD_FIELD_REQ_MTOUCH, "1"); if (ret) { - pr_warning("xenkbd: can't request multi-touch"); + pr_warn("xenkbd: can't request multi-touch"); touch = 0; } } @@ -301,14 +317,6 @@ static int xenkbd_probe(struct xenbus_device *dev, mtouch->id.vendor = 0x5853; mtouch->id.product = 0xfffd; - input_set_capability(mtouch, EV_KEY, BTN_TOUCH); - 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,