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

[Peter Hutterer <peter.hutterer@who-t.net>]

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