[PATCH 0/2] input: synaptics - make image sensors report finger widths

[PATCH 0/2] input: synaptics - make image sensors report finger widths

[Gabriele Mazzotta]

Image sensors were not reporting finger widths despite being able to do
it. These patches will make sure that finger widths are always reported.

Gabriele Mazzotta (2):
  input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR
  input: synaptics - fix width calculation on image sensors

 drivers/input/mouse/synaptics.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

-- 
2.1.4

[PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Gabriele Mazzotta]

Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
instead ABS_TOOL_WIDTH.

Since the 'w' slot is used to report the finger count when two or more
fingers are on the touchpad, make sure that only meaningful values are
emitted, i.e. values greater than or equal to 4, and assign the correct
range to ABS_MT_TOUCH_MAJOR.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/input/mouse/synaptics.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f947292..ea0563e 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
 	input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
 	input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
 	input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
+	if (hw->w >= 4)
+		input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w);
 }
 
 static void synaptics_report_mt_data(struct psmouse *psmouse,
@@ -1462,8 +1464,13 @@ static void set_input_params(struct psmouse *psmouse,
 					INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
 	}
 
-	if (SYN_CAP_PALMDETECT(priv->capabilities))
-		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
+	if (SYN_CAP_PALMDETECT(priv->capabilities)) {
+		if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
+			input_set_abs_params(dev,
+					     ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
+		else
+			input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
+	}
 
 	__set_bit(BTN_TOUCH, dev->keybit);
 	__set_bit(BTN_TOOL_FINGER, dev->keybit);
-- 
2.1.4

[PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Gabriele Mazzotta]

When two or more fingers are on the touchpad, the 'w' slot holds the
finger count rather than the width. Retrieve the correct value encoded
in the lower bits of 'x', 'y' and 'z'.

The minimum width reported is 8 rather than 4 in this case, while the
maximum remains 15.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/input/mouse/synaptics.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ea0563e..5ff4c5b 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned char buf[],
 	switch (agm_packet_type) {
 	case 1:
 		/* Gesture packet: (x, y, z) half resolution */
-		agm->w = hw->w;
+		agm->w = ((buf[1] & 0x01) |
+			  ((buf[2] & 0x01) << 1) |
+			  ((buf[5] & 0x01) << 2)) + 8;
 		agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
 		agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
 		agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
-- 
2.1.4

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Benjamin Tissoires]

Hi Gabriele,

On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> When two or more fingers are on the touchpad, the 'w' slot holds the
> finger count rather than the width. Retrieve the correct value encoded
> in the lower bits of 'x', 'y' and 'z'.
>
> The minimum width reported is 8 rather than 4 in this case, while the
> maximum remains 15.
>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index ea0563e..5ff4c5b 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned char buf[],
>         switch (agm_packet_type) {
>         case 1:
>                 /* Gesture packet: (x, y, z) half resolution */
> -               agm->w = hw->w;
> +               agm->w = ((buf[1] & 0x01) |
> +                         ((buf[2] & 0x01) << 1) |
> +                         ((buf[5] & 0x01) << 2)) + 8;
>                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;

For completeness, I think, we should also removes the w bits from x, y
and z (by masking buf[1], buf[2] and buf[5] with 0xfe).

Cheers,
Benjamin

>                 agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
>                 agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Gabriele Mazzotta]

On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
> Hi Gabriele,
> 
> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > When two or more fingers are on the touchpad, the 'w' slot holds the
> > finger count rather than the width. Retrieve the correct value encoded
> > in the lower bits of 'x', 'y' and 'z'.
> >
> > The minimum width reported is 8 rather than 4 in this case, while the
> > maximum remains 15.
> >
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > ---
> >  drivers/input/mouse/synaptics.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index ea0563e..5ff4c5b 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned char buf[],
> >         switch (agm_packet_type) {
> >         case 1:
> >                 /* Gesture packet: (x, y, z) half resolution */
> > -               agm->w = hw->w;
> > +               agm->w = ((buf[1] & 0x01) |
> > +                         ((buf[2] & 0x01) << 1) |
> > +                         ((buf[5] & 0x01) << 2)) + 8;
> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> 
> For completeness, I think, we should also removes the w bits from x, y
> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).

You are right, I forgot about it.

I was going to re-submit these patches anyway since there have been
quite a few changes and these can no longer be applied on next.

> Cheers,
> Benjamin
> 
> >                 agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
> >                 agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> > --
> > 2.1.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Benjamin Tissoires]

On Mon, Jan 5, 2015 at 1:37 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
>> Hi Gabriele,
>>
>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
>> <gabriele.mzt@gmail.com> wrote:
>> > When two or more fingers are on the touchpad, the 'w' slot holds the
>> > finger count rather than the width. Retrieve the correct value encoded
>> > in the lower bits of 'x', 'y' and 'z'.
>> >
>> > The minimum width reported is 8 rather than 4 in this case, while the
>> > maximum remains 15.
>> >
>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>> > ---
>> >  drivers/input/mouse/synaptics.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> > index ea0563e..5ff4c5b 100644
>> > --- a/drivers/input/mouse/synaptics.c
>> > +++ b/drivers/input/mouse/synaptics.c
>> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned char buf[],
>> >         switch (agm_packet_type) {
>> >         case 1:
>> >                 /* Gesture packet: (x, y, z) half resolution */
>> > -               agm->w = hw->w;
>> > +               agm->w = ((buf[1] & 0x01) |
>> > +                         ((buf[2] & 0x01) << 1) |
>> > +                         ((buf[5] & 0x01) << 2)) + 8;
>> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>>
>> For completeness, I think, we should also removes the w bits from x, y
>> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).
>
> You are right, I forgot about it.
>
> I was going to re-submit these patches anyway since there have been
> quite a few changes and these can no longer be applied on next.

OK, I am trying to put together my thoughts for 1/2 right now. If you
could just wait a little before resubmitting, I would be grateful.

Thanks,
Benjamin

>
>> Cheers,
>> Benjamin
>>
>> >                 agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
>> >                 agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>> > --
>> > 2.1.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Dmitry Torokhov]

On January 5, 2015 10:42:13 AM PST, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
>On Mon, Jan 5, 2015 at 1:37 PM, Gabriele Mazzotta
><gabriele.mzt@gmail.com> wrote:
>> On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
>>> Hi Gabriele,
>>>
>>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
>>> <gabriele.mzt@gmail.com> wrote:
>>> > When two or more fingers are on the touchpad, the 'w' slot holds
>the
>>> > finger count rather than the width. Retrieve the correct value
>encoded
>>> > in the lower bits of 'x', 'y' and 'z'.
>>> >
>>> > The minimum width reported is 8 rather than 4 in this case, while
>the
>>> > maximum remains 15.
>>> >
>>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>> > ---
>>> >  drivers/input/mouse/synaptics.c | 4 +++-
>>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/input/mouse/synaptics.c
>b/drivers/input/mouse/synaptics.c
>>> > index ea0563e..5ff4c5b 100644
>>> > --- a/drivers/input/mouse/synaptics.c
>>> > +++ b/drivers/input/mouse/synaptics.c
>>> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned
>char buf[],
>>> >         switch (agm_packet_type) {
>>> >         case 1:
>>> >                 /* Gesture packet: (x, y, z) half resolution */
>>> > -               agm->w = hw->w;
>>> > +               agm->w = ((buf[1] & 0x01) |
>>> > +                         ((buf[2] & 0x01) << 1) |
>>> > +                         ((buf[5] & 0x01) << 2)) + 8;
>>> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>>>
>>> For completeness, I think, we should also removes the w bits from x,
>y
>>> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).
>>
>> You are right, I forgot about it.
>>
>> I was going to re-submit these patches anyway since there have been
>> quite a few changes and these can no longer be applied on next.
>
>OK, I am trying to put together my thoughts for 1/2 right now. If you
>could just wait a little before resubmitting, I would be grateful.
>

BTW, do we really get different widths four different contacts?

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Benjamin Tissoires]

On Mon, Jan 5, 2015 at 2:04 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On January 5, 2015 10:42:13 AM PST, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
>>On Mon, Jan 5, 2015 at 1:37 PM, Gabriele Mazzotta
>><gabriele.mzt@gmail.com> wrote:
>>> On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
>>>> Hi Gabriele,
>>>>
>>>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
>>>> <gabriele.mzt@gmail.com> wrote:
>>>> > When two or more fingers are on the touchpad, the 'w' slot holds
>>the
>>>> > finger count rather than the width. Retrieve the correct value
>>encoded
>>>> > in the lower bits of 'x', 'y' and 'z'.
>>>> >
>>>> > The minimum width reported is 8 rather than 4 in this case, while
>>the
>>>> > maximum remains 15.
>>>> >
>>>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>>> > ---
>>>> >  drivers/input/mouse/synaptics.c | 4 +++-
>>>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>>>> >
>>>> > diff --git a/drivers/input/mouse/synaptics.c
>>b/drivers/input/mouse/synaptics.c
>>>> > index ea0563e..5ff4c5b 100644
>>>> > --- a/drivers/input/mouse/synaptics.c
>>>> > +++ b/drivers/input/mouse/synaptics.c
>>>> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned
>>char buf[],
>>>> >         switch (agm_packet_type) {
>>>> >         case 1:
>>>> >                 /* Gesture packet: (x, y, z) half resolution */
>>>> > -               agm->w = hw->w;
>>>> > +               agm->w = ((buf[1] & 0x01) |
>>>> > +                         ((buf[2] & 0x01) << 1) |
>>>> > +                         ((buf[5] & 0x01) << 2)) + 8;
>>>> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>>>>
>>>> For completeness, I think, we should also removes the w bits from x,
>>y
>>>> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).
>>>
>>> You are right, I forgot about it.
>>>
>>> I was going to re-submit these patches anyway since there have been
>>> quite a few changes and these can no longer be applied on next.
>>
>>OK, I am trying to put together my thoughts for 1/2 right now. If you
>>could just wait a little before resubmitting, I would be grateful.
>>
>
> BTW, do we really get different widths four different contacts?
>

I think we should. However, on capacitive sensors, the "width" and the
"pressure" fields are 2 ways of sending the same initial information.
Given that we already have two different pressure info, we should thus
get 2 different widths.

Cheers,
Benjamin

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Gabriele Mazzotta]

On Monday 05 January 2015 11:04:07 Dmitry Torokhov wrote:
> On January 5, 2015 10:42:13 AM PST, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
> >On Mon, Jan 5, 2015 at 1:37 PM, Gabriele Mazzotta
> ><gabriele.mzt@gmail.com> wrote:
> >> On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
> >>> Hi Gabriele,
> >>>
> >>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> >>> <gabriele.mzt@gmail.com> wrote:
> >>> > When two or more fingers are on the touchpad, the 'w' slot holds
> >the
> >>> > finger count rather than the width. Retrieve the correct value
> >encoded
> >>> > in the lower bits of 'x', 'y' and 'z'.
> >>> >
> >>> > The minimum width reported is 8 rather than 4 in this case, while
> >the
> >>> > maximum remains 15.
> >>> >
> >>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> >>> > ---
> >>> >  drivers/input/mouse/synaptics.c | 4 +++-
> >>> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >>> >
> >>> > diff --git a/drivers/input/mouse/synaptics.c
> >b/drivers/input/mouse/synaptics.c
> >>> > index ea0563e..5ff4c5b 100644
> >>> > --- a/drivers/input/mouse/synaptics.c
> >>> > +++ b/drivers/input/mouse/synaptics.c
> >>> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned
> >char buf[],
> >>> >         switch (agm_packet_type) {
> >>> >         case 1:
> >>> >                 /* Gesture packet: (x, y, z) half resolution */
> >>> > -               agm->w = hw->w;
> >>> > +               agm->w = ((buf[1] & 0x01) |
> >>> > +                         ((buf[2] & 0x01) << 1) |
> >>> > +                         ((buf[5] & 0x01) << 2)) + 8;
> >>> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> >>>
> >>> For completeness, I think, we should also removes the w bits from x,
> >y
> >>> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).
> >>
> >> You are right, I forgot about it.
> >>
> >> I was going to re-submit these patches anyway since there have been
> >> quite a few changes and these can no longer be applied on next.
> >
> >OK, I am trying to put together my thoughts for 1/2 right now. If you
> >could just wait a little before resubmitting, I would be grateful.
> >
> 
> BTW, do we really get different widths four different contacts?
> 
> Thanks.

I think that only the width of the last finger that was laid on the
touchpad is reported.

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Benjamin Tissoires]

Hi Gabriele,

[Adding Peter and Hans as this change will impact both
xf86-input-synaptics and libinput]

On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> instead ABS_TOOL_WIDTH.

It looks like the current xorg-synaptics code already handles
ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
confirm this because xorg-synaptics still relies a lot on the single
touch emulation.

>
> Since the 'w' slot is used to report the finger count when two or more
> fingers are on the touchpad, make sure that only meaningful values are
> emitted, i.e. values greater than or equal to 4, and assign the correct
> range to ABS_MT_TOUCH_MAJOR.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index f947292..ea0563e 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,

Just FYI, this does not apply anymore on top of Dmitry's tree.
synaptics_report_slot() has been removed, and you should now report
the slot state in synaptics_report_mt_data().

>         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
>         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
>         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> +       if (hw->w >= 4)

That I don't like. IMO, at this point, .w should only contain the
finger width, unconditionally.
Also, with 2/2, .w is computed accurately for the second finger, but
not for the first.

I tried to figure out a way to properly extract the actual width
information from the sgm packet when the w is 0 or 1, and the only way
I found was to do the fix in synaptics_image_sensor_process(). I would
have preferred dealing with that in synaptics_parse_hw_state()
directly, but I think the final code would be more and more ugly.
Dealing with the true finger width in synaptics_image_sensor_process()
is not a problem for cr48 sensors, because they will not have the
ABS_MT_TOUCH_MAJOR event exported.

Anyway, something like that "should" work (sorry for the tab/space,
gmail in use), of course, this is untested when I send the mail :) :

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 95c15451..caf7ccd 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -852,6 +852,17 @@ static void synaptics_image_sensor_process(struct
psmouse *psmouse,
        else
                num_fingers = 4;

+       /* When multiple fingers are on the TouchPad, the width is encoded in
+        * X, Y and Z */
+       if (sgm->w == 0 || sgm->w == 1) {
+               sgm->w = ((sgm->x & BIT(1)) >> 1) |
+                         (sgm->y & BIT(1)) |
+                        ((sgm->z & BIT(0)) << 1);
+               sgm->x &= ~BIT(1);
+               sgm->y &= ~BIT(1);
+               sgm->z &= ~BIT(0);
+       }
+
        /* Send resulting input events to user space */
        synaptics_report_mt_data(psmouse, sgm, num_fingers);
 }


> +               input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w);
>  }
>
>  static void synaptics_report_mt_data(struct psmouse *psmouse,
> @@ -1462,8 +1464,13 @@ static void set_input_params(struct psmouse *psmouse,
>                                         INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
>         }
>
> -       if (SYN_CAP_PALMDETECT(priv->capabilities))
> -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> +       if (SYN_CAP_PALMDETECT(priv->capabilities)) {
> +               if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
> +                       input_set_abs_params(dev,
> +                                            ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> +               else
> +                       input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> +       }

Again, I'd prefer having Peter's point of view here.

If we really need both, we will need to change input-mt to also
emulate ABS_TOOL_WIDTH from ABS_MT_TOUCH_MAJOR like we do between
ABS_MT_POSITION_X|Y and ABS_X|Y.

Cheers,
Benjamin

>
>         __set_bit(BTN_TOUCH, dev->keybit);
>         __set_bit(BTN_TOOL_FINGER, dev->keybit);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] input: synaptics - fix width calculation on image sensors

[Benjamin Tissoires]

On Mon, Jan 5, 2015 at 2:15 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> On Monday 05 January 2015 11:04:07 Dmitry Torokhov wrote:
>> On January 5, 2015 10:42:13 AM PST, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
>> >On Mon, Jan 5, 2015 at 1:37 PM, Gabriele Mazzotta
>> ><gabriele.mzt@gmail.com> wrote:
>> >> On Monday 05 January 2015 13:25:38 Benjamin Tissoires wrote:
>> >>> Hi Gabriele,
>> >>>
>> >>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
>> >>> <gabriele.mzt@gmail.com> wrote:
>> >>> > When two or more fingers are on the touchpad, the 'w' slot holds
>> >the
>> >>> > finger count rather than the width. Retrieve the correct value
>> >encoded
>> >>> > in the lower bits of 'x', 'y' and 'z'.
>> >>> >
>> >>> > The minimum width reported is 8 rather than 4 in this case, while
>> >the
>> >>> > maximum remains 15.
>> >>> >
>> >>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>> >>> > ---
>> >>> >  drivers/input/mouse/synaptics.c | 4 +++-
>> >>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>> >
>> >>> > diff --git a/drivers/input/mouse/synaptics.c
>> >b/drivers/input/mouse/synaptics.c
>> >>> > index ea0563e..5ff4c5b 100644
>> >>> > --- a/drivers/input/mouse/synaptics.c
>> >>> > +++ b/drivers/input/mouse/synaptics.c
>> >>> > @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned
>> >char buf[],
>> >>> >         switch (agm_packet_type) {
>> >>> >         case 1:
>> >>> >                 /* Gesture packet: (x, y, z) half resolution */
>> >>> > -               agm->w = hw->w;
>> >>> > +               agm->w = ((buf[1] & 0x01) |
>> >>> > +                         ((buf[2] & 0x01) << 1) |
>> >>> > +                         ((buf[5] & 0x01) << 2)) + 8;
>> >>> >                 agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>> >>>
>> >>> For completeness, I think, we should also removes the w bits from x,
>> >y
>> >>> and z (by masking buf[1], buf[2] and buf[5] with 0xfe).
>> >>
>> >> You are right, I forgot about it.
>> >>
>> >> I was going to re-submit these patches anyway since there have been
>> >> quite a few changes and these can no longer be applied on next.
>> >
>> >OK, I am trying to put together my thoughts for 1/2 right now. If you
>> >could just wait a little before resubmitting, I would be grateful.
>> >
>>
>> BTW, do we really get different widths four different contacts?
>>
>> Thanks.
>
> I think that only the width of the last finger that was laid on the
> touchpad is reported.

According to the Synaptics PS2 Interfacing guide, the width of the
primary finger is also reported. See
http://www.synaptics.com/sites/default/files/511-000275-01_RevB.pdf
section 3.2.10. page 32)

Cheers,
Benjamin

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Gabriele Mazzotta]

On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
> Hi Gabriele,
> 
> [Adding Peter and Hans as this change will impact both
> xf86-input-synaptics and libinput]
> 
> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> > instead ABS_TOOL_WIDTH.
> 
> It looks like the current xorg-synaptics code already handles
> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> confirm this because xorg-synaptics still relies a lot on the single
> touch emulation.

Yes, xorg-synaptics handles ABS_MT_TOUCH_MAJOR, I sent the patch for it.
The width of the fingers is used only for the two fingers emulation and
the palm detection, both of which are broken on image sensors as the
width is never reported.

I'm normally using hid-rmi for my touchpad which never sends
ABS_TOOL_WIDTH and everything seems to work correctly. Same for
synaptics with this patch, although it seems that the pressure drops
drastically when there are more than one finger, so this must be taken
into account when defining the threshold values. I'll double check that
everything is correct for 'z'.

> >
> > Since the 'w' slot is used to report the finger count when two or more
> > fingers are on the touchpad, make sure that only meaningful values are
> > emitted, i.e. values greater than or equal to 4, and assign the correct
> > range to ABS_MT_TOUCH_MAJOR.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > ---
> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index f947292..ea0563e 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> 
> Just FYI, this does not apply anymore on top of Dmitry's tree.
> synaptics_report_slot() has been removed, and you should now report
> the slot state in synaptics_report_mt_data().
> 
> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> > +       if (hw->w >= 4)
> 
> That I don't like. IMO, at this point, .w should only contain the
> finger width, unconditionally.
> Also, with 2/2, .w is computed accurately for the second finger, but
> not for the first.
> 
> I tried to figure out a way to properly extract the actual width
> information from the sgm packet when the w is 0 or 1, and the only way
> I found was to do the fix in synaptics_image_sensor_process(). I would
> have preferred dealing with that in synaptics_parse_hw_state()
> directly, but I think the final code would be more and more ugly.
> Dealing with the true finger width in synaptics_image_sensor_process()
> is not a problem for cr48 sensors, because they will not have the
> ABS_MT_TOUCH_MAJOR event exported.
> 
> Anyway, something like that "should" work (sorry for the tab/space,
> gmail in use), of course, this is untested when I send the mail :) :
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 95c15451..caf7ccd 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -852,6 +852,17 @@ static void synaptics_image_sensor_process(struct
> psmouse *psmouse,
>         else
>                 num_fingers = 4;
> 
> +       /* When multiple fingers are on the TouchPad, the width is encoded in
> +        * X, Y and Z */
> +       if (sgm->w == 0 || sgm->w == 1) {
> +               sgm->w = ((sgm->x & BIT(1)) >> 1) |
> +                         (sgm->y & BIT(1)) |
> +                        ((sgm->z & BIT(0)) << 1);
> +               sgm->x &= ~BIT(1);
> +               sgm->y &= ~BIT(1);
> +               sgm->z &= ~BIT(0);
> +       }
> +

It works perfectly with some adjustments:

	/* When multiple fingers are on the TouchPad, the width is encoded in
	 * X, Y and Z */
	if (sgm->w == 0 || sgm->w == 1) {
		sgm->w = (((sgm->x & BIT(1)) >> 1) |
			  (sgm->y & BIT(1)) |
			  ((sgm->z & BIT(0)) << 2)) + 8;
		sgm->x &= ~BIT(1);
		sgm->y &= ~BIT(1);
		sgm->z &= ~BIT(0);
	}

>         /* Send resulting input events to user space */
>         synaptics_report_mt_data(psmouse, sgm, num_fingers);
>  }
> 
> 
> > +               input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w);
> >  }
> >
> >  static void synaptics_report_mt_data(struct psmouse *psmouse,
> > @@ -1462,8 +1464,13 @@ static void set_input_params(struct psmouse *psmouse,
> >                                         INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
> >         }
> >
> > -       if (SYN_CAP_PALMDETECT(priv->capabilities))
> > -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > +       if (SYN_CAP_PALMDETECT(priv->capabilities)) {
> > +               if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
> > +                       input_set_abs_params(dev,
> > +                                            ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> > +               else
> > +                       input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > +       }
> 
> Again, I'd prefer having Peter's point of view here.
> 
> If we really need both, we will need to change input-mt to also
> emulate ABS_TOOL_WIDTH from ABS_MT_TOUCH_MAJOR like we do between
> ABS_MT_POSITION_X|Y and ABS_X|Y.
> 
> Cheers,
> Benjamin
> 
> >
> >         __set_bit(BTN_TOUCH, dev->keybit);
> >         __set_bit(BTN_TOOL_FINGER, dev->keybit);
> > --
> > 2.1.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Gabriele Mazzotta]

On Monday 05 January 2015 21:13:09 Gabriele Mazzotta wrote:
> On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
> > Hi Gabriele,
> > 
> > [Adding Peter and Hans as this change will impact both
> > xf86-input-synaptics and libinput]
> > 
> > On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> > <gabriele.mzt@gmail.com> wrote:
> > > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> > > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> > > instead ABS_TOOL_WIDTH.
> > 
> > It looks like the current xorg-synaptics code already handles
> > ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> > replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> > confirm this because xorg-synaptics still relies a lot on the single
> > touch emulation.
> 
> Yes, xorg-synaptics handles ABS_MT_TOUCH_MAJOR, I sent the patch for it.
> The width of the fingers is used only for the two fingers emulation and
> the palm detection, both of which are broken on image sensors as the
> width is never reported.
> 
> I'm normally using hid-rmi for my touchpad which never sends
> ABS_TOOL_WIDTH and everything seems to work correctly. Same for
> synaptics with this patch, although it seems that the pressure drops
> drastically when there are more than one finger, so this must be taken
> into account when defining the threshold values. I'll double check that
> everything is correct for 'z'.

It was wrong

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 808c8e3..b3aed2e 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -590,7 +590,7 @@ static void synaptics_parse_agm(const unsigned char buf[],
                          ((buf[5] & 0x01) << 2)) + 8;
                agm->x = (((buf[4] & 0x0f) << 8) | (buf[1] & 0xfe)) << 1;
                agm->y = (((buf[4] & 0xf0) << 4) | (buf[2] & 0xfe)) << 1;
-               agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0e)) << 1;
+               agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0e)) << 2;
                break;
 
        case 2:

> > >
> > > Since the 'w' slot is used to report the finger count when two or more
> > > fingers are on the touchpad, make sure that only meaningful values are
> > > emitted, i.e. values greater than or equal to 4, and assign the correct
> > > range to ABS_MT_TOUCH_MAJOR.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> > > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > ---
> > >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > > index f947292..ea0563e 100644
> > > --- a/drivers/input/mouse/synaptics.c
> > > +++ b/drivers/input/mouse/synaptics.c
> > > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> > 
> > Just FYI, this does not apply anymore on top of Dmitry's tree.
> > synaptics_report_slot() has been removed, and you should now report
> > the slot state in synaptics_report_mt_data().
> > 
> > >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> > >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> > >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> > > +       if (hw->w >= 4)
> > 
> > That I don't like. IMO, at this point, .w should only contain the
> > finger width, unconditionally.
> > Also, with 2/2, .w is computed accurately for the second finger, but
> > not for the first.
> > 
> > I tried to figure out a way to properly extract the actual width
> > information from the sgm packet when the w is 0 or 1, and the only way
> > I found was to do the fix in synaptics_image_sensor_process(). I would
> > have preferred dealing with that in synaptics_parse_hw_state()
> > directly, but I think the final code would be more and more ugly.
> > Dealing with the true finger width in synaptics_image_sensor_process()
> > is not a problem for cr48 sensors, because they will not have the
> > ABS_MT_TOUCH_MAJOR event exported.
> > 
> > Anyway, something like that "should" work (sorry for the tab/space,
> > gmail in use), of course, this is untested when I send the mail :) :
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index 95c15451..caf7ccd 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -852,6 +852,17 @@ static void synaptics_image_sensor_process(struct
> > psmouse *psmouse,
> >         else
> >                 num_fingers = 4;
> > 
> > +       /* When multiple fingers are on the TouchPad, the width is encoded in
> > +        * X, Y and Z */
> > +       if (sgm->w == 0 || sgm->w == 1) {
> > +               sgm->w = ((sgm->x & BIT(1)) >> 1) |
> > +                         (sgm->y & BIT(1)) |
> > +                        ((sgm->z & BIT(0)) << 1);
> > +               sgm->x &= ~BIT(1);
> > +               sgm->y &= ~BIT(1);
> > +               sgm->z &= ~BIT(0);
> > +       }
> > +
> 
> It works perfectly with some adjustments:
> 
> 	/* When multiple fingers are on the TouchPad, the width is encoded in
> 	 * X, Y and Z */
> 	if (sgm->w == 0 || sgm->w == 1) {
> 		sgm->w = (((sgm->x & BIT(1)) >> 1) |
> 			  (sgm->y & BIT(1)) |
> 			  ((sgm->z & BIT(0)) << 2)) + 8;
> 		sgm->x &= ~BIT(1);
> 		sgm->y &= ~BIT(1);
> 		sgm->z &= ~BIT(0);
> 	}
> 
> >         /* Send resulting input events to user space */
> >         synaptics_report_mt_data(psmouse, sgm, num_fingers);
> >  }
> > 
> > 
> > > +               input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w);
> > >  }
> > >
> > >  static void synaptics_report_mt_data(struct psmouse *psmouse,
> > > @@ -1462,8 +1464,13 @@ static void set_input_params(struct psmouse *psmouse,
> > >                                         INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
> > >         }
> > >
> > > -       if (SYN_CAP_PALMDETECT(priv->capabilities))
> > > -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > > +       if (SYN_CAP_PALMDETECT(priv->capabilities)) {
> > > +               if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
> > > +                       input_set_abs_params(dev,
> > > +                                            ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> > > +               else
> > > +                       input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > > +       }
> > 
> > Again, I'd prefer having Peter's point of view here.
> > 
> > If we really need both, we will need to change input-mt to also
> > emulate ABS_TOOL_WIDTH from ABS_MT_TOUCH_MAJOR like we do between
> > ABS_MT_POSITION_X|Y and ABS_X|Y.
> > 
> > Cheers,
> > Benjamin
> > 
> > >
> > >         __set_bit(BTN_TOUCH, dev->keybit);
> > >         __set_bit(BTN_TOOL_FINGER, dev->keybit);
> > > --
> > > 2.1.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Gabriele Mazzotta]

On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
> Hi Gabriele,
> 
> [Adding Peter and Hans as this change will impact both
> xf86-input-synaptics and libinput]
> 
> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> > instead ABS_TOOL_WIDTH.
> 
> It looks like the current xorg-synaptics code already handles
> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> confirm this because xorg-synaptics still relies a lot on the single
> touch emulation.
> 
> >
> > Since the 'w' slot is used to report the finger count when two or more
> > fingers are on the touchpad, make sure that only meaningful values are
> > emitted, i.e. values greater than or equal to 4, and assign the correct
> > range to ABS_MT_TOUCH_MAJOR.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > ---
> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index f947292..ea0563e 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> 
> Just FYI, this does not apply anymore on top of Dmitry's tree.
> synaptics_report_slot() has been removed, and you should now report
> the slot state in synaptics_report_mt_data().
> 
> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> > +       if (hw->w >= 4)
> 
> That I don't like. IMO, at this point, .w should only contain the
> finger width, unconditionally.
> Also, with 2/2, .w is computed accurately for the second finger, but
> not for the first.
> 
> I tried to figure out a way to properly extract the actual width
> information from the sgm packet when the w is 0 or 1, and the only way
> I found was to do the fix in synaptics_image_sensor_process(). I would
> have preferred dealing with that in synaptics_parse_hw_state()
> directly, but I think the final code would be more and more ugly.
> Dealing with the true finger width in synaptics_image_sensor_process()
> is not a problem for cr48 sensors, because they will not have the
> ABS_MT_TOUCH_MAJOR event exported.

Regarding the last part on cr48 sensors.
Currently these sensors are not reporting widths through ABS_TOOL_WIDTH
and I don't see what could go wrong if they start reporting
ABS_MT_TOUCH_MAJOR. If I understood correctly, they can report widths
only when one finger is on the touchpad. This means that they will
report widths through slot 0, but they won't through slot 1. Nothing
bad should happen.

I've just rebased my patches and included the support for cr48 sensors.
The only change I made was to change the default width value from
5 to 4 since this is the minimum values these sensors can report
(according to the existing code).

Anyway, should I simply include the changes you suggested to handle
widths of sgm packets in what will be the new 2/2?

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Benjamin Tissoires]

On Mon, Jan 5, 2015 at 5:00 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
>> Hi Gabriele,
>>
>> [Adding Peter and Hans as this change will impact both
>> xf86-input-synaptics and libinput]
>>
>> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
>> <gabriele.mzt@gmail.com> wrote:
>> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
>> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
>> > instead ABS_TOOL_WIDTH.
>>
>> It looks like the current xorg-synaptics code already handles
>> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
>> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
>> confirm this because xorg-synaptics still relies a lot on the single
>> touch emulation.
>>
>> >
>> > Since the 'w' slot is used to report the finger count when two or more
>> > fingers are on the touchpad, make sure that only meaningful values are
>> > emitted, i.e. values greater than or equal to 4, and assign the correct
>> > range to ABS_MT_TOUCH_MAJOR.
>> >
>> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
>> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>> > ---
>> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> > index f947292..ea0563e 100644
>> > --- a/drivers/input/mouse/synaptics.c
>> > +++ b/drivers/input/mouse/synaptics.c
>> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
>>
>> Just FYI, this does not apply anymore on top of Dmitry's tree.
>> synaptics_report_slot() has been removed, and you should now report
>> the slot state in synaptics_report_mt_data().
>>
>> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
>> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
>> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
>> > +       if (hw->w >= 4)
>>
>> That I don't like. IMO, at this point, .w should only contain the
>> finger width, unconditionally.
>> Also, with 2/2, .w is computed accurately for the second finger, but
>> not for the first.
>>
>> I tried to figure out a way to properly extract the actual width
>> information from the sgm packet when the w is 0 or 1, and the only way
>> I found was to do the fix in synaptics_image_sensor_process(). I would
>> have preferred dealing with that in synaptics_parse_hw_state()
>> directly, but I think the final code would be more and more ugly.
>> Dealing with the true finger width in synaptics_image_sensor_process()
>> is not a problem for cr48 sensors, because they will not have the
>> ABS_MT_TOUCH_MAJOR event exported.
>
> Regarding the last part on cr48 sensors.
> Currently these sensors are not reporting widths through ABS_TOOL_WIDTH
> and I don't see what could go wrong if they start reporting
> ABS_MT_TOUCH_MAJOR. If I understood correctly, they can report widths
> only when one finger is on the touchpad. This means that they will
> report widths through slot 0, but they won't through slot 1. Nothing
> bad should happen.

I am not entirely sure. The entire purpose of having widht for palm
detection is to filter palm from true finger events. So if we only
have the width info on the first slot, it would be useless IMO.
Still I agree with "nothing bad should happen" :)

>
> I've just rebased my patches and included the support for cr48 sensors.
> The only change I made was to change the default width value from
> 5 to 4 since this is the minimum values these sensors can report
> (according to the existing code).
>
> Anyway, should I simply include the changes you suggested to handle
> widths of sgm packets in what will be the new 2/2?

Go ahead and include my changes in your 2/2. It's better to have a
consistent commit.

Cheers,
Benjamin

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Peter Hutterer]

On Mon, Jan 05, 2015 at 02:24:30PM -0500, Benjamin Tissoires wrote:
> Hi Gabriele,
> 
> [Adding Peter and Hans as this change will impact both
> xf86-input-synaptics and libinput]
> 
> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> > instead ABS_TOOL_WIDTH.
> 
> It looks like the current xorg-synaptics code already handles
> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> confirm this because xorg-synaptics still relies a lot on the single
> touch emulation.

synaptics doesn't actually use TOUCH_MAJOR for finger width, we just forward
it as axis information (not that anyone uses it).
dropping TOOL_WIDTH would only affect palm detection which unreliable, off
by default and mostly broken. so this is fine with me.

Cheers,
   Peter

> 
> >
> > Since the 'w' slot is used to report the finger count when two or more
> > fingers are on the touchpad, make sure that only meaningful values are
> > emitted, i.e. values greater than or equal to 4, and assign the correct
> > range to ABS_MT_TOUCH_MAJOR.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > ---
> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index f947292..ea0563e 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> 
> Just FYI, this does not apply anymore on top of Dmitry's tree.
> synaptics_report_slot() has been removed, and you should now report
> the slot state in synaptics_report_mt_data().
> 
> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> > +       if (hw->w >= 4)
> 
> That I don't like. IMO, at this point, .w should only contain the
> finger width, unconditionally.
> Also, with 2/2, .w is computed accurately for the second finger, but
> not for the first.
> 
> I tried to figure out a way to properly extract the actual width
> information from the sgm packet when the w is 0 or 1, and the only way
> I found was to do the fix in synaptics_image_sensor_process(). I would
> have preferred dealing with that in synaptics_parse_hw_state()
> directly, but I think the final code would be more and more ugly.
> Dealing with the true finger width in synaptics_image_sensor_process()
> is not a problem for cr48 sensors, because they will not have the
> ABS_MT_TOUCH_MAJOR event exported.
> 
> Anyway, something like that "should" work (sorry for the tab/space,
> gmail in use), of course, this is untested when I send the mail :) :
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 95c15451..caf7ccd 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -852,6 +852,17 @@ static void synaptics_image_sensor_process(struct
> psmouse *psmouse,
>         else
>                 num_fingers = 4;
> 
> +       /* When multiple fingers are on the TouchPad, the width is encoded in
> +        * X, Y and Z */
> +       if (sgm->w == 0 || sgm->w == 1) {
> +               sgm->w = ((sgm->x & BIT(1)) >> 1) |
> +                         (sgm->y & BIT(1)) |
> +                        ((sgm->z & BIT(0)) << 1);
> +               sgm->x &= ~BIT(1);
> +               sgm->y &= ~BIT(1);
> +               sgm->z &= ~BIT(0);
> +       }
> +
>         /* Send resulting input events to user space */
>         synaptics_report_mt_data(psmouse, sgm, num_fingers);
>  }
> 
> 
> > +               input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w);
> >  }
> >
> >  static void synaptics_report_mt_data(struct psmouse *psmouse,
> > @@ -1462,8 +1464,13 @@ static void set_input_params(struct psmouse *psmouse,
> >                                         INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
> >         }
> >
> > -       if (SYN_CAP_PALMDETECT(priv->capabilities))
> > -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > +       if (SYN_CAP_PALMDETECT(priv->capabilities)) {
> > +               if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
> > +                       input_set_abs_params(dev,
> > +                                            ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> > +               else
> > +                       input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> > +       }
> 
> Again, I'd prefer having Peter's point of view here.
> 
> If we really need both, we will need to change input-mt to also
> emulate ABS_TOOL_WIDTH from ABS_MT_TOUCH_MAJOR like we do between
> ABS_MT_POSITION_X|Y and ABS_X|Y.
> 
> Cheers,
> Benjamin
> 
> >
> >         __set_bit(BTN_TOUCH, dev->keybit);
> >         __set_bit(BTN_TOOL_FINGER, dev->keybit);
> > --
> > 2.1.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Peter Hutterer]

On Mon, Jan 05, 2015 at 05:04:55PM -0500, Benjamin Tissoires wrote:
> On Mon, Jan 5, 2015 at 5:00 PM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
> >> Hi Gabriele,
> >>
> >> [Adding Peter and Hans as this change will impact both
> >> xf86-input-synaptics and libinput]
> >>
> >> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> >> <gabriele.mzt@gmail.com> wrote:
> >> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> >> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> >> > instead ABS_TOOL_WIDTH.
> >>
> >> It looks like the current xorg-synaptics code already handles
> >> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> >> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> >> confirm this because xorg-synaptics still relies a lot on the single
> >> touch emulation.
> >>
> >> >
> >> > Since the 'w' slot is used to report the finger count when two or more
> >> > fingers are on the touchpad, make sure that only meaningful values are
> >> > emitted, i.e. values greater than or equal to 4, and assign the correct
> >> > range to ABS_MT_TOUCH_MAJOR.
> >> >
> >> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> >> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> >> > ---
> >> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> >> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >> > index f947292..ea0563e 100644
> >> > --- a/drivers/input/mouse/synaptics.c
> >> > +++ b/drivers/input/mouse/synaptics.c
> >> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> >>
> >> Just FYI, this does not apply anymore on top of Dmitry's tree.
> >> synaptics_report_slot() has been removed, and you should now report
> >> the slot state in synaptics_report_mt_data().
> >>
> >> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> >> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> >> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> >> > +       if (hw->w >= 4)
> >>
> >> That I don't like. IMO, at this point, .w should only contain the
> >> finger width, unconditionally.
> >> Also, with 2/2, .w is computed accurately for the second finger, but
> >> not for the first.
> >>
> >> I tried to figure out a way to properly extract the actual width
> >> information from the sgm packet when the w is 0 or 1, and the only way
> >> I found was to do the fix in synaptics_image_sensor_process(). I would
> >> have preferred dealing with that in synaptics_parse_hw_state()
> >> directly, but I think the final code would be more and more ugly.
> >> Dealing with the true finger width in synaptics_image_sensor_process()
> >> is not a problem for cr48 sensors, because they will not have the
> >> ABS_MT_TOUCH_MAJOR event exported.
> >
> > Regarding the last part on cr48 sensors.
> > Currently these sensors are not reporting widths through ABS_TOOL_WIDTH
> > and I don't see what could go wrong if they start reporting
> > ABS_MT_TOUCH_MAJOR. If I understood correctly, they can report widths
> > only when one finger is on the touchpad. This means that they will
> > report widths through slot 0, but they won't through slot 1. Nothing
> > bad should happen.
> 
> I am not entirely sure. The entire purpose of having widht for palm
> detection is to filter palm from true finger events. So if we only
> have the width info on the first slot, it would be useless IMO.
> Still I agree with "nothing bad should happen" :)

fwiw, I found finger width _very_ unreliable for palm detection. from what I
recorded, both finger width and palm width ranges overlap to a large amount,
making width useless for anything that's not the whole hand on the
touchpad.

Cheers,
   Peter

> > I've just rebased my patches and included the support for cr48 sensors.
> > The only change I made was to change the default width value from
> > 5 to 4 since this is the minimum values these sensors can report
> > (according to the existing code).
> >
> > Anyway, should I simply include the changes you suggested to handle
> > widths of sgm packets in what will be the new 2/2?
> 
> Go ahead and include my changes in your 2/2. It's better to have a
> consistent commit.
> 
> Cheers,
> Benjamin

Re: [PATCH 1/2] input: synaptics - make image sensors report ABS_MT_TOUCH_MAJOR

[Dmitry Torokhov]

On Mon, Jan 05, 2015 at 05:04:55PM -0500, Benjamin Tissoires wrote:
> On Mon, Jan 5, 2015 at 5:00 PM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:
> >> Hi Gabriele,
> >>
> >> [Adding Peter and Hans as this change will impact both
> >> xf86-input-synaptics and libinput]
> >>
> >> On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
> >> <gabriele.mzt@gmail.com> wrote:
> >> > Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
> >> > were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
> >> > instead ABS_TOOL_WIDTH.
> >>
> >> It looks like the current xorg-synaptics code already handles
> >> ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
> >> replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
> >> confirm this because xorg-synaptics still relies a lot on the single
> >> touch emulation.
> >>
> >> >
> >> > Since the 'w' slot is used to report the finger count when two or more
> >> > fingers are on the touchpad, make sure that only meaningful values are
> >> > emitted, i.e. values greater than or equal to 4, and assign the correct
> >> > range to ABS_MT_TOUCH_MAJOR.
> >> >
> >> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
> >> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> >> > ---
> >> >  drivers/input/mouse/synaptics.c | 11 +++++++++--
> >> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >> > index f947292..ea0563e 100644
> >> > --- a/drivers/input/mouse/synaptics.c
> >> > +++ b/drivers/input/mouse/synaptics.c
> >> > @@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,
> >>
> >> Just FYI, this does not apply anymore on top of Dmitry's tree.
> >> synaptics_report_slot() has been removed, and you should now report
> >> the slot state in synaptics_report_mt_data().
> >>
> >> >         input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> >> >         input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> >> >         input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> >> > +       if (hw->w >= 4)
> >>
> >> That I don't like. IMO, at this point, .w should only contain the
> >> finger width, unconditionally.
> >> Also, with 2/2, .w is computed accurately for the second finger, but
> >> not for the first.
> >>
> >> I tried to figure out a way to properly extract the actual width
> >> information from the sgm packet when the w is 0 or 1, and the only way
> >> I found was to do the fix in synaptics_image_sensor_process(). I would
> >> have preferred dealing with that in synaptics_parse_hw_state()
> >> directly, but I think the final code would be more and more ugly.
> >> Dealing with the true finger width in synaptics_image_sensor_process()
> >> is not a problem for cr48 sensors, because they will not have the
> >> ABS_MT_TOUCH_MAJOR event exported.
> >
> > Regarding the last part on cr48 sensors.
> > Currently these sensors are not reporting widths through ABS_TOOL_WIDTH
> > and I don't see what could go wrong if they start reporting
> > ABS_MT_TOUCH_MAJOR. If I understood correctly, they can report widths
> > only when one finger is on the touchpad. This means that they will
> > report widths through slot 0, but they won't through slot 1. Nothing
> > bad should happen.
> 
> I am not entirely sure. The entire purpose of having widht for palm
> detection is to filter palm from true finger events. So if we only
> have the width info on the first slot, it would be useless IMO.
> Still I agree with "nothing bad should happen" :)

>From conceptual perspective if device is not capable of reporting
contact size for each contact then it should not be sending
ABS_MT_TOUCH_MAJOR, same as we do not send ABS_MT_PRESSURE if we can't
provide per-contact pressure. For such devices we revert to ST-events
ABS_PRESSURE and I guess we'll have to continue with ABS_TOOL_WIDTH for
contact sizes in Synaptics.

Thanks.

-- 
Dmitry