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

[Angela Czubak <acz@semihalf.com>]

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