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

From: Angela Czubak <acz@semihalf.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>, seobrien@chromium.org
Cc: linux-input@vger.kernel.org, upstream@semihalf.com
Subject: Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation
Date: Tue, 11 Jan 2022 18:06:57 +0100	[thread overview]
Message-ID: <CAB4aORU35k1zuMPaKsa09Qr34wgurYw1F-Mpyn2WZN77+5R+7A@mail.gmail.com> (raw)
In-Reply-To: <Ydye8C1MjxpHHF5P@google.com>

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