diff mbox series

[13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

Message ID 20211221191743.1893185-14-acz@semihalf.com
State New
Headers show
Series *** Implement simple haptic HID support *** | expand

Commit Message

Angela Czubak Dec. 21, 2021, 7:17 p.m. UTC
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/input/input-mt.c | 18 ++++++++++++++++--
 include/linux/input/mt.h |  4 ++++
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Dmitry Torokhov Jan. 7, 2022, 10:07 p.m. UTC | #1
Hi Angela,

On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> Add a function to switch off ABS_PRESSURE generation if necessary.
> This may be helpful in case drivers want to generate ABS_PRESSURE events
> themselves from ABS_MT_PRESSURE.

This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().

Or did I misunderstand the point?

Thanks.
Angela Czubak Jan. 10, 2022, 7:43 p.m. UTC | #2
Hi Dmitry,

On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Angela,
>
> On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > Add a function to switch off ABS_PRESSURE generation if necessary.
> > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > themselves from ABS_MT_PRESSURE.
>
> This needs better explanation for why it is needed. I assume this is to
> use ABS_PRESSURE to report "true force" for devices. If this is correct
> then I believe we should define a new flag for input_mt_init_slots()
> and check it here and also use it to calculate the force across contacts
> in input_mt_sync_frame().
>
> Or did I misunderstand the point?
>
I would say you understood it correctly, though to my mind it is not a
static behaviour,
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode. Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
> Thanks.

>
> --
> Dmitry
Dmitry Torokhov Jan. 10, 2022, 9:02 p.m. UTC | #3
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> Hi Dmitry,
> 
> On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Angela,
> >
> > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > themselves from ABS_MT_PRESSURE.
> >
> > This needs better explanation for why it is needed. I assume this is to
> > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > then I believe we should define a new flag for input_mt_init_slots()
> > and check it here and also use it to calculate the force across contacts
> > in input_mt_sync_frame().
> >
> > Or did I misunderstand the point?
> >
> I would say you understood it correctly, though to my mind it is not a
> static behaviour,

It should be, otherwise how will userspace know the meaning of the
event?

> i.e. we may want to switch this kind of calculation on and off.
> Are flags intended to be modified at runtime?

No.

> For instance, if user decides to remove the release or press effect (previously
> uploaded by them) and there is no default one per device, then we should switch
> the haptic handling from kernel mode back to device mode.

Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.

In my mind we have following cases:

- OS does not know about these haptics devices (touchpads). They work in
  device (?) mode and provide haptic feedback on their own.

- OS does know about haptics devices (that includes having both kernel
  *and* userspace support for them. If one is missing then the other
  should not be enabled, it is up to the distro to make sure all pieces
  are there). In this case OS controls haptics effects all the time,
  except:

- OS supports haptics, but switched it to device mode to allow haptics
  effect playback when waking up.

> Currently it
> also means
> that the driver stops generating ABS_PRESSURE events on its own and so
> input-mt layer may/should be used again (i.e. mt report pointer emulation).
> Anyhow, if it would be actually better to calculate the true force in
> input_mt_sync_frame()/input_mt_report_pointer_emulation()

Thanks.
Angela Czubak Jan. 11, 2022, 5:06 p.m. UTC | #4
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > Hi Dmitry,
> >
> > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Angela,
> > >
> > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > themselves from ABS_MT_PRESSURE.
> > >
> > > This needs better explanation for why it is needed. I assume this is to
> > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > then I believe we should define a new flag for input_mt_init_slots()
> > > and check it here and also use it to calculate the force across contacts
> > > in input_mt_sync_frame().
> > >
> > > Or did I misunderstand the point?
> > >
> > I would say you understood it correctly, though to my mind it is not a
> > static behaviour,
>
> It should be, otherwise how will userspace know the meaning of the
> event?
>
Fair point.

> > i.e. we may want to switch this kind of calculation on and off.
> > Are flags intended to be modified at runtime?
>
> No.
>
> > For instance, if user decides to remove the release or press effect (previously
> > uploaded by them) and there is no default one per device, then we should switch
> > the haptic handling from kernel mode back to device mode.
>
> Why? I think if user removes effects then they do not want to have
> haptics effects. I am wondering if this whole thing made too complex.
>
> In my mind we have following cases:
>
> - OS does not know about these haptics devices (touchpads). They work in
>   device (?) mode and provide haptic feedback on their own.
>
> - OS does know about haptics devices (that includes having both kernel
>   *and* userspace support for them. If one is missing then the other
>   should not be enabled, it is up to the distro to make sure all pieces
>   are there). In this case OS controls haptics effects all the time,
>   except:
>
> - OS supports haptics, but switched it to device mode to allow haptics
>   effect playback when waking up.
>
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.

Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.

Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?

> > Currently it
> > also means
> > that the driver stops generating ABS_PRESSURE events on its own and so
> > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > Anyhow, if it would be actually better to calculate the true force in
> > input_mt_sync_frame()/input_mt_report_pointer_emulation()
>
(I suppose I wanted to say I would implement it in such case)

> Thanks.
>
> --
> Dmitry
Dmitry Torokhov Jan. 12, 2022, 2:52 a.m. UTC | #5
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > Hi Angela,
> > > > >
> > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > themselves from ABS_MT_PRESSURE.
> > > > >
> > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > and check it here and also use it to calculate the force across contacts
> > > > > in input_mt_sync_frame().
> > > > >
> > > > > Or did I misunderstand the point?
> > > > >
> > > > I would say you understood it correctly, though to my mind it is not a
> > > > static behaviour,
> > >
> > > It should be, otherwise how will userspace know the meaning of the
> > > event?
> > >
> > Fair point.
> >
> > > > i.e. we may want to switch this kind of calculation on and off.
> > > > Are flags intended to be modified at runtime?
> > >
> > > No.
> > >
> > > > For instance, if user decides to remove the release or press effect (previously
> > > > uploaded by them) and there is no default one per device, then we should switch
> > > > the haptic handling from kernel mode back to device mode.
> > >
> > > Why? I think if user removes effects then they do not want to have
> > > haptics effects. I am wondering if this whole thing made too complex.
> > >
> > > In my mind we have following cases:
> > >
> > > - OS does not know about these haptics devices (touchpads). They work in
> > >   device (?) mode and provide haptic feedback on their own.
> > >
> > > - OS does know about haptics devices (that includes having both kernel
> > >   *and* userspace support for them. If one is missing then the other
> > >   should not be enabled, it is up to the distro to make sure all pieces
> > >   are there). In this case OS controls haptics effects all the time,
> > >   except:
> > >
> > > - OS supports haptics, but switched it to device mode to allow haptics
> > >   effect playback when waking up.
> > >
> > Perhaps switching between modes should be a separate discussion.
> > Right now it seems to me that your suggestion could be that if
> > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > across all slots. Does it sound right?
> > If so, I suppose I will implement it. It should be completely independent from
> > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > the pressure sum still gets calculated.

I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).

> >
> > Sean, is it OK for the device to keep kernel mode in the event no
> > default press/release
> > waveform is defined in the waveform list and the user removes relevant effects
> > (after having uploaded them)? I think it was desired to remain in the
> > device mode
> > if no such waveforms/effects are defined and, thus, I assumed that removing
> > following effects (in case no press/release waveforms in the waveform
> > list) should
> > trigger coming back to device mode.
> > Right now it seems that switching back to device mode should be
> > allowed only when
> > suspending the device.
> 
> I agree that we should switch to device-controlled mode if press/release are
> not defined by the device, and userspace has not supplied alternative
> waveforms for either. If we kept it in kernel-controlled mode, there would be
> no effect for click/release. This can be achieved by userspace by emitting
> EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.

What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.

Benjamin, Jiri, Peter, I'd like you to chime in please.

> 
> This also allows for the case where userspace may want to send haptics for UX
> effects, while still relying on the device for traditional press and release
> haptics (in the case where the device doesn't define press/release
> waveforms).

Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...

> >
> > Now, the question would be where BTN_LEFT events should be generated.
> > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > Does anyone mind such behaviour?
> >
> > > > Currently it
> > > > also means
> > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > Anyhow, if it would be actually better to calculate the true force in
> > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > >
> > (I suppose I wanted to say I would implement it in such case)

Thanks.
Angela Czubak Jan. 12, 2022, 9:02 a.m. UTC | #6
On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Angela,
> > > > > >
> > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > >
> > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > in input_mt_sync_frame().
> > > > > >
> > > > > > Or did I misunderstand the point?
> > > > > >
> > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > static behaviour,
> > > >
> > > > It should be, otherwise how will userspace know the meaning of the
> > > > event?
> > > >
> > > Fair point.
> > >
> > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > Are flags intended to be modified at runtime?
> > > >
> > > > No.
> > > >
> > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > the haptic handling from kernel mode back to device mode.
> > > >
> > > > Why? I think if user removes effects then they do not want to have
> > > > haptics effects. I am wondering if this whole thing made too complex.
> > > >
> > > > In my mind we have following cases:
> > > >
> > > > - OS does not know about these haptics devices (touchpads). They work in
> > > >   device (?) mode and provide haptic feedback on their own.
> > > >
> > > > - OS does know about haptics devices (that includes having both kernel
> > > >   *and* userspace support for them. If one is missing then the other
> > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > >   are there). In this case OS controls haptics effects all the time,
> > > >   except:
> > > >
> > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > >   effect playback when waking up.
> > > >
> > > Perhaps switching between modes should be a separate discussion.
> > > Right now it seems to me that your suggestion could be that if
> > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > across all slots. Does it sound right?
> > > If so, I suppose I will implement it. It should be completely independent from
> > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > the pressure sum still gets calculated.
>
> I'd say that if hid_haptic_init() fails we should not say that the
> device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> the device instantiation, which we probably should not).
>
It would still be a haptic touchpad, I suppose; the fact that
initialization failed isn't going
to do anything with the device, really, rather some failure in memory
allocation etc.
If you are worried about how the userspace can be sure that
initialization succeeded,
if hid_haptic_init() succeeds then the device should report EV_FF
events, otherwise not.
Angela Czubak Jan. 14, 2022, 6:26 p.m. UTC | #7
On Wed, Jan 12, 2022 at 10:17 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Angela,
> > > > > > >
> > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > >
> > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > in input_mt_sync_frame().
> > > > > > >
> > > > > > > Or did I misunderstand the point?
> > > > > > >
> > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > static behaviour,
> > > > >
> > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > event?
> > > > >
> > > > Fair point.
> > > >
> > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > Are flags intended to be modified at runtime?
> > > > >
> > > > > No.
> > > > >
> > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > the haptic handling from kernel mode back to device mode.
> > > > >
> > > > > Why? I think if user removes effects then they do not want to have
> > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > >
> > > > > In my mind we have following cases:
> > > > >
> > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > >   device (?) mode and provide haptic feedback on their own.
> > > > >
> > > > > - OS does know about haptics devices (that includes having both kernel
> > > > >   *and* userspace support for them. If one is missing then the other
> > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > >   are there). In this case OS controls haptics effects all the time,
> > > > >   except:
> > > > >
> > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > >   effect playback when waking up.
> > > > >
> > > > Perhaps switching between modes should be a separate discussion.
> > > > Right now it seems to me that your suggestion could be that if
> > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > across all slots. Does it sound right?
> > > > If so, I suppose I will implement it. It should be completely independent from
> > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > the pressure sum still gets calculated.
> >
> > I'd say that if hid_haptic_init() fails we should not say that the
> > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > the device instantiation, which we probably should not).
>
> Agree. Userspace should know that the device is a pressure pad based
> on the unit provided in ABS_MT_PRESSURE IIRC.
> So setting the resolution is enough for userspace to emulate the
> button clicks based on the pressure. libinput already has code for
> that.
>

A quick glance [1] and it seems that libinput chooses to ignore ABS_MT_PRESSURE
it the resolution is non-zero (though I might have been looking in a
wrong place).
Mentioning just because someone might lead me to a proper place/library actually
using ABS_MT_PRESSURE as force.

> So basically, INPUT_PROP_HAPTIC_TOUCHPAD is only an indication that
> the haptic is configurable. And if haptic_init() fails, it should not
> expose that property.
>
> And BTW, why "TOUCHPAD" in INPUT_PROP_HAPTIC_TOUCHPAD? The Surface
> Dial could benefit from that implementation and it is not a
> touchpad...
>

Ok, so looking back at the old discussion it seems to me that the
property originally
suggested is INPUT_PROP_FORCEPAD and it was initially intended to mean that
ABS_MT_PRESSURE events should be interpreted as force and not area/pressure.
In case units are grams or newtons I calculate the resolution, but it
seems that Peter
has previously stated it is not enough:

>And we can't just assume "if resolution is set, units are $foo" because
>nothing written in the last decade or so will assume that. Some extra flag
>is needed, like INPUT_PROP_FORCEPAD.

I think Benjamin originally suggested this flag so that userspace knows
ABS_MT_PRESSURE should mean force.

However, it was a very long time ago. It seems that about a year ago it was
defined that non-zero pressure resolution means units/grams is used.

It seems to me that we could assume that reporting FF_HID events implicates
how ABS_MT_PRESSURE should be interpreted, so I could get rid of this
flag, if that is what you prefer.

> >
> > > >
> > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > default press/release
> > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > (after having uploaded them)? I think it was desired to remain in the
> > > > device mode
> > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > following effects (in case no press/release waveforms in the waveform
> > > > list) should
> > > > trigger coming back to device mode.
> > > > Right now it seems that switching back to device mode should be
> > > > allowed only when
> > > > suspending the device.
> > >
> > > I agree that we should switch to device-controlled mode if press/release are
> > > not defined by the device, and userspace has not supplied alternative
> > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > no effect for click/release. This can be achieved by userspace by emitting
> > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> >
> > What is wrong for not having effect for press/release if userspace did
> > not bother to set it up? I think this is reasonably to expect that if
> > user enabled support for haptic touchpad in kernel they should also have
> > userspace that knows how to handle it. If we go with this requirement I
> > think we will reduce a lot of complexity.
> >
> > Benjamin, Jiri, Peter, I'd like you to chime in please.
>
> [FWIW, lei saved me on this one for not being Cc-ed since the
> beginning of this thread]
>
> I think we should keep it simple:
> - the device configuration should be static (i.e.
> ABS_PRESSURE/ABS_MT_PRESSURE, pointer emulation, button emulation,
> ...) always present
> - userspace should pick up what it needs based on its own state:
>   if there is a need to compute a total pressure, userspace is capable
> of computing itself, and generates its own button press/release
> - the haptic is a global state of the device, so any decision you make
> is going to have corner cases with more than one userspace (or if the
> userspace daemon/lib is restarted)
>
> So to me, we should keep the kernel device emulation, export what
> needs to be for userspace to make its own decision and have the haptic
> side as a "nice to have" feature but distinct from the event
> processing.
>
> I didn't want to chime into this thread because I am currently working
> on 2 big series that might also be helpful here:
> - the first one, which is almost ready, consists in rethinking how the
> HID events are processed, meaning we can ensure that some events are
> always processed before others. The net benefit is that I can now
> express the Win8 multitouch protocol in hid-generic without too much
> pain, meaning that hid-haptic.c could be a leaf driver instead of
> being an API.
> The net benefit of not having hid-haptic.c as an API is that we can
> always rmmod it to disable the entire haptic system if there is
> something wrong.
>
> - the second one is the eBPF bindings for HID (see
> https://lore.kernel.org/all/20211215134220.1735144-1-tero.kristo@linux.intel.com/
> and the other versions for some more discussions)
> Basically BPF allows to avoid specific kernel APIs and userspace is in
> charge of loading the bridge between its API and the device. It
> definitely has the potential to solve many limitations we are seeing
> now in all the various input/ff protocols IMO.
>
> >
> > >
> > > This also allows for the case where userspace may want to send haptics for UX
> > > effects, while still relying on the device for traditional press and release
> > > haptics (in the case where the device doesn't define press/release
> > > waveforms).
> >
> > Again, what is the difference between press/release and other UX
> > effects? They seem to be the same to me...
> >
> > > >
> > > > Now, the question would be where BTN_LEFT events should be generated.
> > > > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > > > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > > > Does anyone mind such behaviour?
>
> Again, why is there a need to have some complex behavior there? Just
> let userspace do its own fancy computation and keep it simple in the
> kernel.

I thought it was requested based on the following discussion [2]:

>>
>> ABS_PRESSURE may be optionally reported as the total force applied to the
>> forcepad.
>>
>> The device/driver shouldn’t detect button clicks, this is left to the userspace
>> gesture library. Accordingly, the driver should not sent BTN_* events to
>> userspace in normal operating mode. However it should still report the ability
>> to produce such events, for use in autonomous mode.
>
> For backward compatibility, and to be able to debug it properly, you
> should keep the BTN_* events emulated in all cases.
> The userspace can ignore the events it doesn't want this way, but you
> will be able to debug the btn emulations on your current session
> without having to kill your compositor.
> There shouldn't be much of a head over forwarding those events, as it
> will never come alone, and will always be with an other one at least
> (pressure being 0 or less).
>
> Also, not sending BTN_TOUCH and BTN_LEFT might give some headaches to
> legacy applications.

I can remove such behaviour if I misunderstood or it is no longer valid.

> Well, with eBPF, you could let userspace put the BTN_LEFT emulation in
> the kernel by loading a specific program, but that would be in charge
> of the userspace to make this choice, not the kernel.
>
> Cheers,
> Benjamin
>
> > > >
> > > > > > Currently it
> > > > > > also means
> > > > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > > > Anyhow, if it would be actually better to calculate the true force in
> > > > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > > > >
> > > > (I suppose I wanted to say I would implement it in such case)
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
>
Regards,
Angela

[1] https://gitlab.freedesktop.org/libinput/libinput/-/issues/562
[2] https://www.spinics.net/lists/linux-input/msg60983.html
Peter Hutterer Jan. 21, 2022, 6:10 a.m. UTC | #8
On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Angela,
> > > > > >
> > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > >
> > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > in input_mt_sync_frame().
> > > > > >
> > > > > > Or did I misunderstand the point?
> > > > > >
> > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > static behaviour,
> > > >
> > > > It should be, otherwise how will userspace know the meaning of the
> > > > event?
> > > >
> > > Fair point.
> > >
> > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > Are flags intended to be modified at runtime?
> > > >
> > > > No.
> > > >
> > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > the haptic handling from kernel mode back to device mode.
> > > >
> > > > Why? I think if user removes effects then they do not want to have
> > > > haptics effects. I am wondering if this whole thing made too complex.
> > > >
> > > > In my mind we have following cases:
> > > >
> > > > - OS does not know about these haptics devices (touchpads). They work in
> > > >   device (?) mode and provide haptic feedback on their own.
> > > >
> > > > - OS does know about haptics devices (that includes having both kernel
> > > >   *and* userspace support for them. If one is missing then the other
> > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > >   are there). In this case OS controls haptics effects all the time,
> > > >   except:
> > > >
> > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > >   effect playback when waking up.
> > > >
> > > Perhaps switching between modes should be a separate discussion.
> > > Right now it seems to me that your suggestion could be that if
> > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > across all slots. Does it sound right?
> > > If so, I suppose I will implement it. It should be completely independent from
> > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > the pressure sum still gets calculated.
> 
> I'd say that if hid_haptic_init() fails we should not say that the
> device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> the device instantiation, which we probably should not).
> 
> > >
> > > Sean, is it OK for the device to keep kernel mode in the event no
> > > default press/release
> > > waveform is defined in the waveform list and the user removes relevant effects
> > > (after having uploaded them)? I think it was desired to remain in the
> > > device mode
> > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > following effects (in case no press/release waveforms in the waveform
> > > list) should
> > > trigger coming back to device mode.
> > > Right now it seems that switching back to device mode should be
> > > allowed only when
> > > suspending the device.
> > 
> > I agree that we should switch to device-controlled mode if press/release are
> > not defined by the device, and userspace has not supplied alternative
> > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > no effect for click/release. This can be achieved by userspace by emitting
> > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> 
> What is wrong for not having effect for press/release if userspace did
> not bother to set it up? I think this is reasonably to expect that if
> user enabled support for haptic touchpad in kernel they should also have
> userspace that knows how to handle it. If we go with this requirement I
> think we will reduce a lot of complexity.
> 
> Benjamin, Jiri, Peter, I'd like you to chime in please.
> 
> > 
> > This also allows for the case where userspace may want to send haptics for UX
> > effects, while still relying on the device for traditional press and release
> > haptics (in the case where the device doesn't define press/release
> > waveforms).
> 
> Again, what is the difference between press/release and other UX
> effects? They seem to be the same to me...

Agree with Dmitry here - have a sensible default in the kernel and if
userspace changes it, it's now userspace's problem to do it right. Anything
more complex is just making things more complicated for niche cases that may
never happen.

Cheers,
  Peter
Angela Czubak Jan. 25, 2022, 4:56 p.m. UTC | #9
Hi Peter, Dmitry, Benjamin, Sean,

On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Angela,
> > > > > > >
> > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > >
> > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > in input_mt_sync_frame().
> > > > > > >
> > > > > > > Or did I misunderstand the point?
> > > > > > >
> > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > static behaviour,
> > > > >
> > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > event?
> > > > >
> > > > Fair point.
> > > >
> > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > Are flags intended to be modified at runtime?
> > > > >
> > > > > No.
> > > > >
> > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > the haptic handling from kernel mode back to device mode.
> > > > >
> > > > > Why? I think if user removes effects then they do not want to have
> > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > >
> > > > > In my mind we have following cases:
> > > > >
> > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > >   device (?) mode and provide haptic feedback on their own.
> > > > >
> > > > > - OS does know about haptics devices (that includes having both kernel
> > > > >   *and* userspace support for them. If one is missing then the other
> > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > >   are there). In this case OS controls haptics effects all the time,
> > > > >   except:
> > > > >
> > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > >   effect playback when waking up.
> > > > >
> > > > Perhaps switching between modes should be a separate discussion.
> > > > Right now it seems to me that your suggestion could be that if
> > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > across all slots. Does it sound right?
> > > > If so, I suppose I will implement it. It should be completely independent from
> > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > the pressure sum still gets calculated.
> >
> > I'd say that if hid_haptic_init() fails we should not say that the
> > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > the device instantiation, which we probably should not).
> >
> > > >
> > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > default press/release
> > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > (after having uploaded them)? I think it was desired to remain in the
> > > > device mode
> > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > following effects (in case no press/release waveforms in the waveform
> > > > list) should
> > > > trigger coming back to device mode.
> > > > Right now it seems that switching back to device mode should be
> > > > allowed only when
> > > > suspending the device.
> > >
> > > I agree that we should switch to device-controlled mode if press/release are
> > > not defined by the device, and userspace has not supplied alternative
> > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > no effect for click/release. This can be achieved by userspace by emitting
> > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> >
> > What is wrong for not having effect for press/release if userspace did
> > not bother to set it up? I think this is reasonably to expect that if
> > user enabled support for haptic touchpad in kernel they should also have
> > userspace that knows how to handle it. If we go with this requirement I
> > think we will reduce a lot of complexity.
> >
> > Benjamin, Jiri, Peter, I'd like you to chime in please.
> >
> > >
> > > This also allows for the case where userspace may want to send haptics for UX
> > > effects, while still relying on the device for traditional press and release
> > > haptics (in the case where the device doesn't define press/release
> > > waveforms).
> >
> > Again, what is the difference between press/release and other UX
> > effects? They seem to be the same to me...
>
> Agree with Dmitry here - have a sensible default in the kernel and if
> userspace changes it, it's now userspace's problem to do it right. Anything
> more complex is just making things more complicated for niche cases that may
> never happen.
>

Could you please relate to the following statements/questions? I would like to
make sure I am nearer to your understanding of how the things should be.
I wouldn't say they constitute my plan, I am just wondering if shared effects
are acceptable at all since their handling seems questionable.

1. Kernel mode - is it OK to have any default at all? Or would you rather say
   it's userspace's responsibility to issue force feedback entirely? I am just
   wondering how much simplification you would actually prefer to have.
   In the current patchset the kernel can issue haptic feedback itself
   (based on the pressure/force sums calculated).
2. The patches introduce shared effects. This allows userspace to modify
   kernel mode behaviour, i.e. the waveforms it issues when press/release
   has been detected, which means both uploading and erasing those
   effects is possible.
   On the other hand, closing event fd triggers removing effects uploaded for
   that fd. I would assume removing shared effects is allowed as well
   since we can update them with upload. Should it be disallowed/prohibited?
   I mean that perhaps erasing shared effects should never really take place
   as we may end up removing something that has not been altered by
   userspace.
   I am worried since simply opening and closing the event file could possibly
   cause a change in behaviour if we actually let effects be completely
   removed.
3. Switching to kernel mode should happen at the instantiation and then only
   during suspend/resume cycle. If the shared press/release effect gets
   removed (even caused by input device flush), then we don't want any haptic
   feedback in kernel mode anyway.
4. Should I just not care and not sum the pressures across all slots? It just
   seemed to me there was a reason to choose one slot and pass it as
   ABS_PRESSURE in input-mt.c, and I just suspected it would be more
   logical to pass the sum of forces if the unit suggests it is force.

> Cheers,
>   Peter
Peter Hutterer Jan. 28, 2022, 5:25 a.m. UTC | #10
On Tue, Jan 25, 2022 at 05:56:17PM +0100, Angela Czubak wrote:
> Hi Peter, Dmitry, Benjamin, Sean,
> 
> On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >
> > On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> > > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Angela,
> > > > > > > >
> > > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > > >
> > > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > > in input_mt_sync_frame().
> > > > > > > >
> > > > > > > > Or did I misunderstand the point?
> > > > > > > >
> > > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > > static behaviour,
> > > > > >
> > > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > > event?
> > > > > >
> > > > > Fair point.
> > > > >
> > > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > > Are flags intended to be modified at runtime?
> > > > > >
> > > > > > No.
> > > > > >
> > > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > > the haptic handling from kernel mode back to device mode.
> > > > > >
> > > > > > Why? I think if user removes effects then they do not want to have
> > > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > > >
> > > > > > In my mind we have following cases:
> > > > > >
> > > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > > >   device (?) mode and provide haptic feedback on their own.
> > > > > >
> > > > > > - OS does know about haptics devices (that includes having both kernel
> > > > > >   *and* userspace support for them. If one is missing then the other
> > > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > > >   are there). In this case OS controls haptics effects all the time,
> > > > > >   except:
> > > > > >
> > > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > > >   effect playback when waking up.
> > > > > >
> > > > > Perhaps switching between modes should be a separate discussion.
> > > > > Right now it seems to me that your suggestion could be that if
> > > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > > across all slots. Does it sound right?
> > > > > If so, I suppose I will implement it. It should be completely independent from
> > > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > > the pressure sum still gets calculated.
> > >
> > > I'd say that if hid_haptic_init() fails we should not say that the
> > > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > > the device instantiation, which we probably should not).
> > >
> > > > >
> > > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > > default press/release
> > > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > > (after having uploaded them)? I think it was desired to remain in the
> > > > > device mode
> > > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > > following effects (in case no press/release waveforms in the waveform
> > > > > list) should
> > > > > trigger coming back to device mode.
> > > > > Right now it seems that switching back to device mode should be
> > > > > allowed only when
> > > > > suspending the device.
> > > >
> > > > I agree that we should switch to device-controlled mode if press/release are
> > > > not defined by the device, and userspace has not supplied alternative
> > > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > > no effect for click/release. This can be achieved by userspace by emitting
> > > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> > >
> > > What is wrong for not having effect for press/release if userspace did
> > > not bother to set it up? I think this is reasonably to expect that if
> > > user enabled support for haptic touchpad in kernel they should also have
> > > userspace that knows how to handle it. If we go with this requirement I
> > > think we will reduce a lot of complexity.
> > >
> > > Benjamin, Jiri, Peter, I'd like you to chime in please.
> > >
> > > >
> > > > This also allows for the case where userspace may want to send haptics for UX
> > > > effects, while still relying on the device for traditional press and release
> > > > haptics (in the case where the device doesn't define press/release
> > > > waveforms).
> > >
> > > Again, what is the difference between press/release and other UX
> > > effects? They seem to be the same to me...
> >
> > Agree with Dmitry here - have a sensible default in the kernel and if
> > userspace changes it, it's now userspace's problem to do it right. Anything
> > more complex is just making things more complicated for niche cases that may
> > never happen.
> >
> 
> Could you please relate to the following statements/questions? I would like to
> make sure I am nearer to your understanding of how the things should be.
> I wouldn't say they constitute my plan, I am just wondering if shared effects
> are acceptable at all since their handling seems questionable.
> 
> 1. Kernel mode - is it OK to have any default at all? Or would you rather say
>    it's userspace's responsibility to issue force feedback entirely? I am just
>    wondering how much simplification you would actually prefer to have.
>    In the current patchset the kernel can issue haptic feedback itself
>    (based on the pressure/force sums calculated).

IMO the kernel should have a default. without any force feedback the devices
are going to be difficult to use.

> 2. The patches introduce shared effects. This allows userspace to modify
>    kernel mode behaviour, i.e. the waveforms it issues when press/release
>    has been detected, which means both uploading and erasing those
>    effects is possible.
>    On the other hand, closing event fd triggers removing effects uploaded for
>    that fd. I would assume removing shared effects is allowed as well
>    since we can update them with upload. Should it be disallowed/prohibited?
>    I mean that perhaps erasing shared effects should never really take place
>    as we may end up removing something that has not been altered by
>    userspace.
>    I am worried since simply opening and closing the event file could possibly
>    cause a change in behaviour if we actually let effects be completely
>    removed.

if you remove effects on fd close, you're effectively enforcing that some
process always needs to keep the fd to this device open just to have it
respond correctly. It may also prevent the device from going to sleep, right?

So I think there's an argument to leaving the configured waveforms there after
closing the effect - similar things already happen with other evdev ioctls.

> 3. Switching to kernel mode should happen at the instantiation and then only
>    during suspend/resume cycle. If the shared press/release effect gets
>    removed (even caused by input device flush), then we don't want any haptic
>    feedback in kernel mode anyway.
> 4. Should I just not care and not sum the pressures across all slots? It just
>    seemed to me there was a reason to choose one slot and pass it as
>    ABS_PRESSURE in input-mt.c, and I just suspected it would be more
>    logical to pass the sum of forces if the unit suggests it is force.

I'm not quite keeping up with the details in the patches but - whatever is the
most accurate physical measurement? :)

Cheers,
  Peter
diff mbox series

Patch

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 44fe6f2f063c..e0bf5917a8b5 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -52,6 +52,7 @@  int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 
 	mt->num_slots = num_slots;
 	mt->flags = flags;
+	mt->abs_pressure_gen = true;
 	input_set_abs_params(dev, ABS_MT_SLOT, 0, num_slots - 1, 0, 0);
 	input_set_abs_params(dev, ABS_MT_TRACKING_ID, 0, TRKID_MAX, 0, 0);
 
@@ -244,12 +245,14 @@  void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count)
 		input_event(dev, EV_ABS, ABS_X, x);
 		input_event(dev, EV_ABS, ABS_Y, y);
 
-		if (test_bit(ABS_MT_PRESSURE, dev->absbit)) {
+		if (test_bit(ABS_MT_PRESSURE, dev->absbit) &&
+		    mt->abs_pressure_gen) {
 			int p = input_mt_get_value(oldest, ABS_MT_PRESSURE);
 			input_event(dev, EV_ABS, ABS_PRESSURE, p);
 		}
 	} else {
-		if (test_bit(ABS_MT_PRESSURE, dev->absbit))
+		if (test_bit(ABS_MT_PRESSURE, dev->absbit) &&
+		    mt->abs_pressure_gen)
 			input_event(dev, EV_ABS, ABS_PRESSURE, 0);
 	}
 }
@@ -312,6 +315,17 @@  void input_mt_sync_frame(struct input_dev *dev)
 }
 EXPORT_SYMBOL(input_mt_sync_frame);
 
+void input_mt_pressure_toggle(struct input_dev *dev, bool toggle)
+{
+	struct input_mt *mt = dev->mt;
+
+	if (!mt)
+		return;
+
+	mt->abs_pressure_gen = toggle;
+}
+EXPORT_SYMBOL(input_mt_pressure_toggle);
+
 static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
 {
 	int f, *p, s, c;
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index 3b8580bd33c1..c870a513bde1 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -38,6 +38,7 @@  struct input_mt_slot {
  * @flags: input_mt operation flags
  * @frame: increases every time input_mt_sync_frame() is called
  * @red: reduced cost matrix for in-kernel tracking
+ * @abs_pressure_gen: emulate pointer pressure
  * @slots: array of slots holding current values of tracked contacts
  */
 struct input_mt {
@@ -47,6 +48,7 @@  struct input_mt {
 	unsigned int flags;
 	unsigned int frame;
 	int *red;
+	bool abs_pressure_gen;
 	struct input_mt_slot slots[];
 };
 
@@ -111,6 +113,8 @@  void input_mt_drop_unused(struct input_dev *dev);
 
 void input_mt_sync_frame(struct input_dev *dev);
 
+void input_mt_pressure_toggle(struct input_dev *dev, bool toggle);
+
 /**
  * struct input_mt_pos - contact position
  * @x: horizontal coordinate