| Message ID | 20190521132712.2818-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Fix Elan I2C touchpads in latest generation from Lenovo | expand |
Hi, On Tue, May 21, 2019 at 3:27 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi, > > This is the v2 from https://lkml.org/lkml/2018/10/12/633 > > So I initially thought it would be easy to integrate the suggested changes > in the v1, but it turns our that the changes to have the touchscreen-width > and height parameters were quite hard to do. > > I finally postponed the issue by blacklisting the 2 laptops we knew were > not working and tried to devote more time to understand both drivers more. > > But it s the time where Lenovo is preparing the new models, and guess what, > they suffer from the same symptoms. I just received the confirmation from Lenovo that this series makes the new laptops working. Cheers, Benjamin > > So I took a few time to work on this and finally got my head around the > width and height problem. Once I got it, it was simple clear, but this also > means we can not really rely on a device tree property for that. > > So in the elan* drivers, the "traces" are simply how many antennas there > are on each axis. Which means that if a trace of 4 is reported in the > events, it means it is simply seen by 4 antennas. So the computation of the > width/height is the following: we take how many antennas there are, we > subtract one to have the number of holes between the antennas, and we > divide the number of unit we have in the axis by the value we just > computed. > This gives a rough 4mm on the P52, in both directions. > > And once you get that, you can just realize that the unit of the width and > height are just the same than the X and Y coordinates, so we can apply the > same resolution. > > So, in the end, that means that elan_i2c needs the information, or it will > not be able to convert the number of crossed antennas into a size, but this > is something specific to this touchpad. > > So here come, 7 months later the v2 on the subject. > > Cheers, > Benjamin > > Benjamin Tissoires (10): > Input: elantech - query the min/max information beforehand too > Input: elantech - add helper function elantech_is_buttonpad() > Input: elantech - detect middle button based on firmware version > dt-bindings: add more optional properties for elan_i2c touchpads > Input: elan_i2c - do not query the info if they are provided > Input: elantech/SMBus - export all capabilities from the PS/2 node > Input: elan_i2c - handle physical middle button > Input: elan_i2c - export true width/height > Input: elan_i2c - correct the width/size base value > Input: elantech: remove P52 from SMBus blacklist > > .../devicetree/bindings/input/elan_i2c.txt | 11 + > drivers/input/mouse/elan_i2c_core.c | 85 +++-- > drivers/input/mouse/elantech.c | 318 ++++++++++-------- > drivers/input/mouse/elantech.h | 8 + > 4 files changed, 251 insertions(+), 171 deletions(-) > > -- > 2.21.0 >
On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote: > > Hi Benjamin, > > Thanks so much for all you do for Elan touchpad. > > For the width_*, I have a question for it. > Our antenna sensors fully occupied the whole touchpad PCB. > > The Gap between 2 sensors are 7.5 mil (0.19mm). > That's why we did not minus one trace. So, with the P52 I have: [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 [ +0.000002] size: (98,55) drivers/input/mouse/elan_i2c_core.c:430 [ +0.000001] res: (31,31) drivers/input/mouse/elan_i2c_core.c:431 calculated size (max/res): 98 x 56 mm true size, as measured: 101 x 60 mm Which gives (without the minus 1): width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm -> this gives a total size of the touchpad of: 96 x 57 mm (width_x * 24, width_y * 14) With the minus 1: width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm -> this gives a total size of the touchpad of: 102 x 60 mm (width_x * 24, width_y * 14) and considering traces-1: 98 x 56 mm Removing 1 to the number of traces gave a squarer values in rows and columns, and this is what is done in the PS/2 driver. Also, going back to the size of the touchpad gives a better value when removing 1 on the *traces. So maybe when forwarding the properties we should remove one there in the PS/2 driver? Cheers, Benjamin > > > Thanks > KT > -----Original Message----- > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Sent: Tuesday, May 21, 2019 9:27 PM > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; Benjamin Tissoires > Subject: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base > value > > *_traces are the number of antennas. width_* is thus the space between 2 > antennas. Which means, we should subtract 1 to the number of antennas to > divide the touchpad by the number of holes between each antenna. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > -- > > new in v2 > --- > drivers/input/mouse/elan_i2c_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c > b/drivers/input/mouse/elan_i2c_core.c > index 6f4feedb7765..3375eaa9a72e 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct > elan_tp_data *data) > if (error) > return error; > } > - data->width_x = data->max_x / x_traces; > - data->width_y = data->max_y / y_traces; > + data->width_x = data->max_x / (x_traces - 1); > + data->width_y = data->max_y / (y_traces - 1); > > if (device_property_read_u32(&client->dev, > "touchscreen-x-mm", &x_mm) || > -- > 2.21.0 >
Hi -----Original Message----- From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] Sent: Friday, May 24, 2019 3:06 PM To: 廖崇榮 Cc: Dmitry Torokhov; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org Subject: Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote: > > Hi Benjamin, > > Thanks so much for all you do for Elan touchpad. > > For the width_*, I have a question for it. > Our antenna sensors fully occupied the whole touchpad PCB. > > The Gap between 2 sensors are 7.5 mil (0.19mm). > That's why we did not minus one trace. So, with the P52 I have: [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 [ +0.000002] size: (98,55) drivers/input/mouse/elan_i2c_core.c:430 [ +0.000001] res: (31,31) drivers/input/mouse/elan_i2c_core.c:431 calculated size (max/res): 98 x 56 mm true size, as measured: 101 x 60 mm I list layout information of P52 touchpad as below. Physical size : 99 x 58 mm Active Area size : ~ 97 * 56 mm, (boarding is 1.008mm for each side) Sensor layout: X Pitch : 4.0286 mm Y Pitch : 4.0147 mm Which gives (without the minus 1): width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm -> this gives a total size of the touchpad of: 96 x 57 mm (width_x * 24, width_y * 14) With the minus 1: width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm -> this gives a total size of the touchpad of: 102 x 60 mm (width_x * 24, width_y * 14) and considering traces-1: 98 x 56 mm Removing 1 to the number of traces gave a squarer values in rows and columns, and this is what is done in the PS/2 driver. Also, going back to the size of the touchpad gives a better value when removing 1 on the *traces. So maybe when forwarding the properties we should remove one there in the PS/2 driver? Removing 1 trace may be better for some of previous touchpad. (depending on sensor pattern) mk_* indicate the number of trace which is touched, and it's not a precise value. I think the usability won't change too much whether removing one trace. PS/2 have supported plenty of touchpad. It's better to remain the same. Thanks KT Cheers, Benjamin > > > Thanks > KT > -----Original Message----- > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Sent: Tuesday, May 21, 2019 9:27 PM > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; Benjamin Tissoires > Subject: [PATCH v2 09/10] Input: elan_i2c - correct the width/size > base value > > *_traces are the number of antennas. width_* is thus the space between > 2 antennas. Which means, we should subtract 1 to the number of > antennas to divide the touchpad by the number of holes between each antenna. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > -- > > new in v2 > --- > drivers/input/mouse/elan_i2c_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c > b/drivers/input/mouse/elan_i2c_core.c > index 6f4feedb7765..3375eaa9a72e 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct > elan_tp_data *data) > if (error) > return error; > } > - data->width_x = data->max_x / x_traces; > - data->width_y = data->max_y / y_traces; > + data->width_x = data->max_x / (x_traces - 1); > + data->width_y = data->max_y / (y_traces - 1); > > if (device_property_read_u32(&client->dev, > "touchscreen-x-mm", &x_mm) || > -- > 2.21.0 >
On Fri, May 24, 2019 at 11:00 AM 廖崇榮 <kt.liao@emc.com.tw> wrote: > > Hi > > -----Original Message----- > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Sent: Friday, May 24, 2019 3:06 PM > To: 廖崇榮 > Cc: Dmitry Torokhov; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > Subject: Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value > > On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote: > > > > Hi Benjamin, > > > > Thanks so much for all you do for Elan touchpad. > > > > For the width_*, I have a question for it. > > Our antenna sensors fully occupied the whole touchpad PCB. > > > > The Gap between 2 sensors are 7.5 mil (0.19mm). > > That's why we did not minus one trace. > > So, with the P52 I have: > [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 > [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 > [ +0.000002] size: (98,55) drivers/input/mouse/elan_i2c_core.c:430 > [ +0.000001] res: (31,31) drivers/input/mouse/elan_i2c_core.c:431 > > calculated size (max/res): 98 x 56 mm > true size, as measured: 101 x 60 mm > > I list layout information of P52 touchpad as below. > Physical size : 99 x 58 mm > Active Area size : ~ 97 * 56 mm, (boarding is 1.008mm for each side) > > Sensor layout: > X Pitch : 4.0286 mm > Y Pitch : 4.0147 mm > > Which gives (without the minus 1): > width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm > > -> this gives a total size of the touchpad of: 96 x 57 mm (width_x * > 24, width_y * 14) > > With the minus 1: > width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm > > -> this gives a total size of the touchpad of: 102 x 60 mm (width_x * > 24, width_y * 14) > and considering traces-1: 98 x 56 mm > > Removing 1 to the number of traces gave a squarer values in rows and columns, and this is what is done in the PS/2 driver. > Also, going back to the size of the touchpad gives a better value when removing 1 on the *traces. > So maybe when forwarding the properties we should remove one there in the PS/2 driver? > > Removing 1 trace may be better for some of previous touchpad. (depending on sensor pattern) > mk_* indicate the number of trace which is touched, and it's not a precise value. > I think the usability won't change too much whether removing one trace. > PS/2 have supported plenty of touchpad. It's better to remain the same. > OK, so I guess I should just drop this patch from the series then. Cheers, Benjamin
On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > The width/height is actually in the same unit than X and Y. So we should > not tamper the data, but just set the proper resolution, so that userspace > can correctly detect which touch is a palm or a finger. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > -- > > new in v2 > --- > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c > index 7ff044c6cd11..6f4feedb7765 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -45,7 +45,6 @@ > #define DRIVER_NAME "elan_i2c" > #define ELAN_VENDOR_ID 0x04f3 > #define ETP_MAX_PRESSURE 255 > -#define ETP_FWIDTH_REDUCE 90 > #define ETP_FINGER_WIDTH 15 > #define ETP_RETRY_COUNT 3 > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > return; > } > > - /* > - * To avoid treating large finger as palm, let's reduce the > - * width x and y per trace. > - */ > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > + area_x = mk_x * data->width_x; > + area_y = mk_y * data->width_y; > > major = max(area_x, area_y); > minor = min(area_x, area_y); > @@ -1123,8 +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data) > ETP_MAX_PRESSURE, 0, 0); > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > ETP_FINGER_WIDTH * max_width, 0, 0); > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > ETP_FINGER_WIDTH * min_width, 0, 0); > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. TL;DR: I don't think we should export the resolution there :( KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? Cheers, Benjamin > > data->input = input; > > -- > 2.21.0 >
Hi -----Original Message----- From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] Sent: Friday, May 24, 2019 5:37 PM To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > The width/height is actually in the same unit than X and Y. So we > should not tamper the data, but just set the proper resolution, so > that userspace can correctly detect which touch is a palm or a finger. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > -- > > new in v2 > --- > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c > b/drivers/input/mouse/elan_i2c_core.c > index 7ff044c6cd11..6f4feedb7765 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -45,7 +45,6 @@ > #define DRIVER_NAME "elan_i2c" > #define ELAN_VENDOR_ID 0x04f3 > #define ETP_MAX_PRESSURE 255 > -#define ETP_FWIDTH_REDUCE 90 > #define ETP_FINGER_WIDTH 15 > #define ETP_RETRY_COUNT 3 > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > return; > } > > - /* > - * To avoid treating large finger as palm, let's reduce the > - * width x and y per trace. > - */ > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > + area_x = mk_x * data->width_x; > + area_y = mk_y * data->width_y; > > major = max(area_x, area_y); > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > static int elan_setup_input_device(struct elan_tp_data *data) > ETP_MAX_PRESSURE, 0, 0); > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > ETP_FINGER_WIDTH * max_width, 0, 0); > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > ETP_FINGER_WIDTH * min_width, 0, 0); > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. TL;DR: I don't think we should export the resolution there :( KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. Our FW team know nothing about ETP_FWIDTH_REDUCE ether. The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. Chrome's finger/palm threshold may be different from other Linux distribution. We will discuss it with Google once the patch picked by chrome and cause something wrong. Cheers, Benjamin > > data->input = input; > > -- > 2.21.0 >
Hi Benjamin, KT, On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > Hi > > -----Original Message----- > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Sent: Friday, May 24, 2019 5:37 PM > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > The width/height is actually in the same unit than X and Y. So we > > should not tamper the data, but just set the proper resolution, so > > that userspace can correctly detect which touch is a palm or a finger. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > -- > > > > new in v2 > > --- > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > b/drivers/input/mouse/elan_i2c_core.c > > index 7ff044c6cd11..6f4feedb7765 100644 > > --- a/drivers/input/mouse/elan_i2c_core.c > > +++ b/drivers/input/mouse/elan_i2c_core.c > > @@ -45,7 +45,6 @@ > > #define DRIVER_NAME "elan_i2c" > > #define ELAN_VENDOR_ID 0x04f3 > > #define ETP_MAX_PRESSURE 255 > > -#define ETP_FWIDTH_REDUCE 90 > > #define ETP_FINGER_WIDTH 15 > > #define ETP_RETRY_COUNT 3 > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > return; > > } > > > > - /* > > - * To avoid treating large finger as palm, let's reduce the > > - * width x and y per trace. > > - */ > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > + area_x = mk_x * data->width_x; > > + area_y = mk_y * data->width_y; > > > > major = max(area_x, area_y); > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > > static int elan_setup_input_device(struct elan_tp_data *data) > > ETP_MAX_PRESSURE, 0, 0); > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > ETP_FINGER_WIDTH * max_width, 0, 0); > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > ETP_FINGER_WIDTH * min_width, 0, 0); > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > TL;DR: I don't think we should export the resolution there :( > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > Chrome's finger/palm threshold may be different from other Linux distribution. > We will discuss it with Google once the patch picked by chrome and cause something wrong. Chrome has logic that contact with maximum major/minor is treated as a palm, so here the driver (which originally came from Chrome OS) artificially reduces the contact size to ensure that palm rejection logic does not trigger. I'm adding Harry to confirm whether we are still using this logic and to see if we can adjust it to be something else. Thanks.
On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Benjamin, KT, > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > Hi > > > > -----Original Message----- > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > Sent: Friday, May 24, 2019 5:37 PM > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > The width/height is actually in the same unit than X and Y. So we > > > should not tamper the data, but just set the proper resolution, so > > > that userspace can correctly detect which touch is a palm or a finger. > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > -- > > > > > > new in v2 > > > --- > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > b/drivers/input/mouse/elan_i2c_core.c > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > @@ -45,7 +45,6 @@ > > > #define DRIVER_NAME "elan_i2c" > > > #define ELAN_VENDOR_ID 0x04f3 > > > #define ETP_MAX_PRESSURE 255 > > > -#define ETP_FWIDTH_REDUCE 90 > > > #define ETP_FINGER_WIDTH 15 > > > #define ETP_RETRY_COUNT 3 > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > return; > > > } > > > > > > - /* > > > - * To avoid treating large finger as palm, let's reduce the > > > - * width x and y per trace. > > > - */ > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > + area_x = mk_x * data->width_x; > > > + area_y = mk_y * data->width_y; > > > > > > major = max(area_x, area_y); > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > > > static int elan_setup_input_device(struct elan_tp_data *data) > > > ETP_MAX_PRESSURE, 0, 0); > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > ETP_FINGER_WIDTH * max_width, 0, 0); > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > ETP_FINGER_WIDTH * min_width, 0, 0); > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > TL;DR: I don't think we should export the resolution there :( > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > Chrome's finger/palm threshold may be different from other Linux distribution. > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > Chrome has logic that contact with maximum major/minor is treated as a > palm, so here the driver (which originally came from Chrome OS) > artificially reduces the contact size to ensure that palm rejection > logic does not trigger. > > I'm adding Harry to confirm whether we are still using this logic and to > see if we can adjust it to be something else. I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
We do still use a maxed out major axis as a signal for a palm in the touchscreen logic, but I'm not too concerned because if that axis is maxed out, the contact should probably be treated as a palm anyway... I'm more concerned with this affecting our gesture detection for touchpad. It looks like this change would cause all contacts to reported as some percentage bigger than they are currently. Can you give me an idea of how big that percentage is? On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote: > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > Hi Benjamin, KT, > > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > > Hi > > > > > > -----Original Message----- > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > > Sent: Friday, May 24, 2019 5:37 PM > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > > The width/height is actually in the same unit than X and Y. So we > > > > should not tamper the data, but just set the proper resolution, so > > > > that userspace can correctly detect which touch is a palm or a finger. > > > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > -- > > > > > > > > new in v2 > > > > --- > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > > b/drivers/input/mouse/elan_i2c_core.c > > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > > @@ -45,7 +45,6 @@ > > > > #define DRIVER_NAME "elan_i2c" > > > > #define ELAN_VENDOR_ID 0x04f3 > > > > #define ETP_MAX_PRESSURE 255 > > > > -#define ETP_FWIDTH_REDUCE 90 > > > > #define ETP_FINGER_WIDTH 15 > > > > #define ETP_RETRY_COUNT 3 > > > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > > return; > > > > } > > > > > > > > - /* > > > > - * To avoid treating large finger as palm, let's reduce the > > > > - * width x and y per trace. > > > > - */ > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > > + area_x = mk_x * data->width_x; > > > > + area_y = mk_y * data->width_y; > > > > > > > > major = max(area_x, area_y); > > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > > > > static int elan_setup_input_device(struct elan_tp_data *data) > > > > ETP_MAX_PRESSURE, 0, 0); > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > > ETP_FINGER_WIDTH * max_width, 0, 0); > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > > ETP_FINGER_WIDTH * min_width, 0, 0); > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); > > > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > > > TL;DR: I don't think we should export the resolution there :( > > > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > > Chrome's finger/palm threshold may be different from other Linux distribution. > > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > > > Chrome has logic that contact with maximum major/minor is treated as a > > palm, so here the driver (which originally came from Chrome OS) > > artificially reduces the contact size to ensure that palm rejection > > logic does not trigger. > > > > I'm adding Harry to confirm whether we are still using this logic and to > > see if we can adjust it to be something else. > > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote: > > We do still use a maxed out major axis as a signal for a palm in the touchscreen > logic, but I'm not too concerned because if that axis is maxed out, the contact > should probably be treated as a palm anyway... > > I'm more concerned with this affecting our gesture detection for > touchpad. It looks > like this change would cause all contacts to reported as some percentage bigger > than they are currently. Can you give me an idea of how big that percentage is? On the P52, I currently have: [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 -> with the computation done in the kernel: width_ratio: 126 height_ratio: 123 For my average finger, the reported traces are between 4 and 6: With the ETP_FWIDTH_REDUCE: Major between 144 to 216 Minor between 132 to 198 Without: Major between 504 to 756 Minor between 492 to 738 So a rough augmentation of 350% For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range. Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand. Cheers, Benjamin > > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote: > > > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > Hi Benjamin, KT, > > > > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > > > Hi > > > > > > > > -----Original Message----- > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > > > Sent: Friday, May 24, 2019 5:37 PM > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > > > > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > The width/height is actually in the same unit than X and Y. So we > > > > > should not tamper the data, but just set the proper resolution, so > > > > > that userspace can correctly detect which touch is a palm or a finger. > > > > > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > > > -- > > > > > > > > > > new in v2 > > > > > --- > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > > > b/drivers/input/mouse/elan_i2c_core.c > > > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > > > @@ -45,7 +45,6 @@ > > > > > #define DRIVER_NAME "elan_i2c" > > > > > #define ELAN_VENDOR_ID 0x04f3 > > > > > #define ETP_MAX_PRESSURE 255 > > > > > -#define ETP_FWIDTH_REDUCE 90 > > > > > #define ETP_FINGER_WIDTH 15 > > > > > #define ETP_RETRY_COUNT 3 > > > > > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > > > return; > > > > > } > > > > > > > > > > - /* > > > > > - * To avoid treating large finger as palm, let's reduce the > > > > > - * width x and y per trace. > > > > > - */ > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > > > + area_x = mk_x * data->width_x; > > > > > + area_y = mk_y * data->width_y; > > > > > > > > > > major = max(area_x, area_y); > > > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > > > > > static int elan_setup_input_device(struct elan_tp_data *data) > > > > > ETP_MAX_PRESSURE, 0, 0); > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > > > ETP_FINGER_WIDTH * max_width, 0, 0); > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > > > ETP_FINGER_WIDTH * min_width, 0, 0); > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); > > > > > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > > > > > TL;DR: I don't think we should export the resolution there :( > > > > > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > > > Chrome's finger/palm threshold may be different from other Linux distribution. > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > > > > > Chrome has logic that contact with maximum major/minor is treated as a > > > palm, so here the driver (which originally came from Chrome OS) > > > artificially reduces the contact size to ensure that palm rejection > > > logic does not trigger. > > > > > > I'm adding Harry to confirm whether we are still using this logic and to > > > see if we can adjust it to be something else. > > > > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
-----Original Message----- From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] Sent: Wednesday, May 29, 2019 3:17 PM To: Sean O'Brien; Peter Hutterer Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote: > > We do still use a maxed out major axis as a signal for a palm in the > touchscreen logic, but I'm not too concerned because if that axis is > maxed out, the contact should probably be treated as a palm anyway... > > I'm more concerned with this affecting our gesture detection for > touchpad. It looks like this change would cause all contacts to > reported as some percentage bigger than they are currently. Can you > give me an idea of how big that percentage is? On the P52, I currently have: [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 -> with the computation done in the kernel: width_ratio: 126 height_ratio: 123 For my average finger, the reported traces are between 4 and 6: With the ETP_FWIDTH_REDUCE: Major between 144 to 216 Minor between 132 to 198 Without: Major between 504 to 756 Minor between 492 to 738 So a rough augmentation of 350% For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range. Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm mk_x = (finger_data[3] & 0x0f); mk_y = (finger_data[3] >> 4); Do we need to use mk_* for major/minor for keeping it consistent with other vendor? But this modification will impact Chromebook's usability and Chrome test suite. Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand. Cheers, Benjamin > > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote: > > > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > Hi Benjamin, KT, > > > > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > > > Hi > > > > > > > > -----Original Message----- > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > > > Sent: Friday, May 24, 2019 5:37 PM > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de > > > > Goede > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true > > > > width/height > > > > > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > The width/height is actually in the same unit than X and Y. So > > > > > we should not tamper the data, but just set the proper > > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger. > > > > > > > > > > Signed-off-by: Benjamin Tissoires > > > > > <benjamin.tissoires@redhat.com> > > > > > > > > > > -- > > > > > > > > > > new in v2 > > > > > --- > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > > > b/drivers/input/mouse/elan_i2c_core.c > > > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > > > @@ -45,7 +45,6 @@ > > > > > #define DRIVER_NAME "elan_i2c" > > > > > #define ELAN_VENDOR_ID 0x04f3 > > > > > #define ETP_MAX_PRESSURE 255 > > > > > -#define ETP_FWIDTH_REDUCE 90 > > > > > #define ETP_FINGER_WIDTH 15 > > > > > #define ETP_RETRY_COUNT 3 > > > > > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > > > return; > > > > > } > > > > > > > > > > - /* > > > > > - * To avoid treating large finger as palm, let's reduce the > > > > > - * width x and y per trace. > > > > > - */ > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > > > + area_x = mk_x * data->width_x; > > > > > + area_y = mk_y * data->width_y; > > > > > > > > > > major = max(area_x, area_y); > > > > > minor = min(area_x, area_y); @@ -1123,8 > > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data) > > > > > ETP_MAX_PRESSURE, 0, 0); > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > > > ETP_FINGER_WIDTH * max_width, 0, > > > > > 0); > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, > > > > > + data->x_res); > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > > > ETP_FINGER_WIDTH * min_width, 0, > > > > > 0); > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, > > > > > + data->y_res); > > > > > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > > > > > TL;DR: I don't think we should export the resolution there :( > > > > > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > > > Chrome's finger/palm threshold may be different from other Linux distribution. > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > > > > > Chrome has logic that contact with maximum major/minor is treated > > > as a palm, so here the driver (which originally came from Chrome > > > OS) artificially reduces the contact size to ensure that palm > > > rejection logic does not trigger. > > > > > > I'm adding Harry to confirm whether we are still using this logic > > > and to see if we can adjust it to be something else. > > > > I'm not very familiar with our touchpad code, so adding Sean > > O'Brien, who is.
On Wed, May 29, 2019 at 2:56 PM 廖崇榮 <kt.liao@emc.com.tw> wrote: > > > > -----Original Message----- > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Sent: Wednesday, May 29, 2019 3:17 PM > To: Sean O'Brien; Peter Hutterer > Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote: > > > > We do still use a maxed out major axis as a signal for a palm in the > > touchscreen logic, but I'm not too concerned because if that axis is > > maxed out, the contact should probably be treated as a palm anyway... > > > > I'm more concerned with this affecting our gesture detection for > > touchpad. It looks like this change would cause all contacts to > > reported as some percentage bigger than they are currently. Can you > > give me an idea of how big that percentage is? > > On the P52, I currently have: > [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 > [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 > > -> with the computation done in the kernel: > width_ratio: 126 > height_ratio: 123 > > For my average finger, the reported traces are between 4 and 6: > With the ETP_FWIDTH_REDUCE: > Major between 144 to 216 > Minor between 132 to 198 > > Without: > Major between 504 to 756 > Minor between 492 to 738 > > So a rough augmentation of 350% > > For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range. > > Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm > mk_x = (finger_data[3] & 0x0f); > mk_y = (finger_data[3] >> 4); > Do we need to use mk_* for major/minor for keeping it consistent with other vendor? IMO, no. It is better to send something closer to an actual unit instead of 12,5th of mm. However, the problem here is that major/minor can be swapped depending on how the finger is placed (horizontally or vertically), so unfortunately, if the axes and resolutions are not the same, then we are screwed, this would just be a value without unit. > But this modification will impact Chromebook's usability and Chrome test suite. Yeah, there is no point breaking things just for the fun of it. Cheers, Benjamin > > > > Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand. > > Cheers, > Benjamin > > > > > > > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote: > > > > > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > Hi Benjamin, KT, > > > > > > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > > > > Hi > > > > > > > > > > -----Original Message----- > > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > > > > Sent: Friday, May 24, 2019 5:37 PM > > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de > > > > > Goede > > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true > > > > > width/height > > > > > > > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > > > The width/height is actually in the same unit than X and Y. So > > > > > > we should not tamper the data, but just set the proper > > > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger. > > > > > > > > > > > > Signed-off-by: Benjamin Tissoires > > > > > > <benjamin.tissoires@redhat.com> > > > > > > > > > > > > -- > > > > > > > > > > > > new in v2 > > > > > > --- > > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > > > > b/drivers/input/mouse/elan_i2c_core.c > > > > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > > > > @@ -45,7 +45,6 @@ > > > > > > #define DRIVER_NAME "elan_i2c" > > > > > > #define ELAN_VENDOR_ID 0x04f3 > > > > > > #define ETP_MAX_PRESSURE 255 > > > > > > -#define ETP_FWIDTH_REDUCE 90 > > > > > > #define ETP_FINGER_WIDTH 15 > > > > > > #define ETP_RETRY_COUNT 3 > > > > > > > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > > > > return; > > > > > > } > > > > > > > > > > > > - /* > > > > > > - * To avoid treating large finger as palm, let's reduce the > > > > > > - * width x and y per trace. > > > > > > - */ > > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > > > > + area_x = mk_x * data->width_x; > > > > > > + area_y = mk_y * data->width_y; > > > > > > > > > > > > major = max(area_x, area_y); > > > > > > minor = min(area_x, area_y); @@ -1123,8 > > > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data) > > > > > > ETP_MAX_PRESSURE, 0, 0); > > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > > > > ETP_FINGER_WIDTH * max_width, 0, > > > > > > 0); > > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, > > > > > > + data->x_res); > > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > > > > ETP_FINGER_WIDTH * min_width, 0, > > > > > > 0); > > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, > > > > > > + data->y_res); > > > > > > > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > > > > > > > TL;DR: I don't think we should export the resolution there :( > > > > > > > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > > > > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > > > > Chrome's finger/palm threshold may be different from other Linux distribution. > > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > > > > > > > Chrome has logic that contact with maximum major/minor is treated > > > > as a palm, so here the driver (which originally came from Chrome > > > > OS) artificially reduces the contact size to ensure that palm > > > > rejection logic does not trigger. > > > > > > > > I'm adding Harry to confirm whether we are still using this logic > > > > and to see if we can adjust it to be something else. > > > > > > I'm not very familiar with our touchpad code, so adding Sean > > > O'Brien, who is. >
On Wed, May 29, 2019 at 09:16:38AM +0200, Benjamin Tissoires wrote: > On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote: > > > > We do still use a maxed out major axis as a signal for a palm in the touchscreen > > logic, but I'm not too concerned because if that axis is maxed out, the contact > > should probably be treated as a palm anyway... > > > > I'm more concerned with this affecting our gesture detection for > > touchpad. It looks > > like this change would cause all contacts to reported as some percentage bigger > > than they are currently. Can you give me an idea of how big that percentage is? > > On the P52, I currently have: > [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428 > [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429 > > -> with the computation done in the kernel: > width_ratio: 126 > height_ratio: 123 > > For my average finger, the reported traces are between 4 and 6: > With the ETP_FWIDTH_REDUCE: > Major between 144 to 216 > Minor between 132 to 198 > > Without: > Major between 504 to 756 > Minor between 492 to 738 > > So a rough augmentation of 350% > > For the Synaptics devices (over SMBus), they send the raw value of the > traces, so you will get a major/minor between 2 to 5. Max on these > axes is 15, so we should get the same percentage of value comparing to > the range. > Which is why libinput has a database of which device reports which > pressure/major/minor ranges as otherwise the values are just > impossible to understand. yeah, I've given up on trying to guess finger/thumb/palm sizes. git grep for these quirk names in libinput for the ranges: AttrTouchSizeRange AttrThumbSizeThreshold AttrPalmSizeThreshold There are also matching s/Size/Pressure/ entries for touchpads without major/minor. Looking at the database now, the palm size thresholds range entries are 5 (Wacom) and a set of 800-1600 for apple touchpads. So yeah, all this is really a bit random :) Feel free to steal those entries though and/or add to them where applicable. Cheers, Peter > > > > > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote: > > > > > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > Hi Benjamin, KT, > > > > > > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote: > > > > > Hi > > > > > > > > > > -----Original Message----- > > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > > > > > Sent: Friday, May 24, 2019 5:37 PM > > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede > > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org > > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height > > > > > > > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > > > The width/height is actually in the same unit than X and Y. So we > > > > > > should not tamper the data, but just set the proper resolution, so > > > > > > that userspace can correctly detect which touch is a palm or a finger. > > > > > > > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > > > > > -- > > > > > > > > > > > > new in v2 > > > > > > --- > > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++------- > > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c > > > > > > b/drivers/input/mouse/elan_i2c_core.c > > > > > > index 7ff044c6cd11..6f4feedb7765 100644 > > > > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > > > > @@ -45,7 +45,6 @@ > > > > > > #define DRIVER_NAME "elan_i2c" > > > > > > #define ELAN_VENDOR_ID 0x04f3 > > > > > > #define ETP_MAX_PRESSURE 255 > > > > > > -#define ETP_FWIDTH_REDUCE 90 > > > > > > #define ETP_FINGER_WIDTH 15 > > > > > > #define ETP_RETRY_COUNT 3 > > > > > > > > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data, > > > > > > return; > > > > > > } > > > > > > > > > > > > - /* > > > > > > - * To avoid treating large finger as palm, let's reduce the > > > > > > - * width x and y per trace. > > > > > > - */ > > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE); > > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE); > > > > > > + area_x = mk_x * data->width_x; > > > > > > + area_y = mk_y * data->width_y; > > > > > > > > > > > > major = max(area_x, area_y); > > > > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ > > > > > > static int elan_setup_input_device(struct elan_tp_data *data) > > > > > > ETP_MAX_PRESSURE, 0, 0); > > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, > > > > > > ETP_FINGER_WIDTH * max_width, 0, 0); > > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res); > > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, > > > > > > ETP_FINGER_WIDTH * min_width, 0, 0); > > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res); > > > > > > > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution. > > > > > > > > > > TL;DR: I don't think we should export the resolution there :( > > > > > > > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around? > > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. > > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether. > > > > > > > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years. > > > > > Chrome's finger/palm threshold may be different from other Linux distribution. > > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong. > > > > > > > > Chrome has logic that contact with maximum major/minor is treated as a > > > > palm, so here the driver (which originally came from Chrome OS) > > > > artificially reduces the contact size to ensure that palm rejection > > > > logic does not trigger. > > > > > > > > I'm adding Harry to confirm whether we are still using this logic and to > > > > see if we can adjust it to be something else. > > > > > > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.