Hi Peter, Dmitry, Benjamin, Sean, On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer <peter.hutterer@xxxxxxxxx> 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@xxxxxxxxxxxx> wrote: > > > > > > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov > > > > <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> 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
