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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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

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