Re: Vibrations in input vs. LED was Re: [PATCH v2 0/3] led: ledtrig-transient: add support for hrtimer

[Dmitry Torokhov]

On September 27, 2017 10:03:28 PM PDT, David Lin <dtwlin@xxxxxxxxxx> wrote:
>Dmitry,
>
>On Fri, Sep 15, 2017 at 3:30 PM, Dmitry Torokhov
><dmitry.torokhov@xxxxxxxxx> wrote:
>> On Fri, Sep 15, 2017 at 2:55 PM, Jacek Anaszewski
>> <jacek.anaszewski@xxxxxxxxx> wrote:
>>> On 09/15/2017 08:34 PM, Dmitry Torokhov wrote:
>>>> On Thu, Sep 14, 2017 at 1:58 PM, Pavel Machek <pavel@xxxxxx> wrote:
>>>>> On Thu 2017-09-14 21:31:31, Jacek Anaszewski wrote:
>>>>>> Hi David and Pavel,
>>>>>>
>>>>>> On 09/13/2017 10:20 PM, Pavel Machek wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>>> These patch series add the LED_BRIGHTNESS_FAST flag support for
>>>>>>>> ledtrig-transient to use hrtimer so that platforms with
>high-resolution timer
>>>>>>>> support can have better accuracy in the trigger duration
>timing. The need for
>>>>>>>> this support is driven by the fact that Android has removed the
>timed_ouput [1]
>>>>>>>> and is now using led-trigger for handling vibrator control
>which requires the
>>>>>>>> timer to be accurate up to a millisecond. However, this flag
>support would also
>>>>>>>> allow hrtimer to co-exist with the ktimer without causing
>warning to the
>>>>>>>> existing drivers [2].
>>>>>>>
>>>>>>> NAK.
>>>>>>>
>>>>>>> LEDs do not need extra overhead, and vibrator control should not
>go
>>>>>>> through LED subsystem.
>>>>>>>
>>>>>>> Input subsystem includes support for vibrations and force
>>>>>>> feedback. Please use that instead.
>>>>>>
>>>>>> I think that most vital criterion here is the usability of the
>>>>>> interface. If it can be harnessed for doing the work seemingly
>>>>>> unrelated to the primary subsystem's purpose, that's fine.
>>>>>> Moreover, it is extremely easy to use in comparison to the force
>>>>>> feedback one.
>>>>>
>>>>> Well, no.
>>>>>
>>>>> Kernel is supposed to provide hardware abstraction, that means it
>>>>> should hide differences between different devices.
>>>>>
>>>>> And we already have devices using input as designed. We don't want
>to
>>>>> have situation where "on phones A, D and E, vibrations are handled
>via
>>>>> input, while on B, C and F, they are handled via /sys/class/leds".
>>>>>
>>>>> If we want to have discussion "how to make vibrations in input
>>>>> easier to use", well that's fair. But I don't think it is
>particulary hard.
>>>>>
>>>>
>>>> I would like to know more about why you find the FF interface hard,
>>>
>>> led-transient trigger can be activated using only following bash
>>> commands:
>>>
>>> # echo 1 > state
>>> # echo 1000 > duration
>>> # while [ 1 ]; do echo 1 > activate; sleep 3; done
>>>
>>> Could you share sample sequence of commands to use ff driver?
>>
>> Cut what you need from this:
>> https://github.com/flosse/linuxconsole/blob/master/utils/fftest.c
>>
>> If your objection is that FF is not easily engaged from the shell -
>> yes, but I do not think that actual users who want to do vibration do
>> that via shell either. On the other hand, can you drop privileges and
>> still allow a certain process control your vibrator via LED
>interface?
>> With FF you can pass an FD to whoever you deem worthy and later
>revoke
>> access.
>>
>> IOW sysfs interfaces are nice for quick hacks, but when you want to
>> use them in real frameworks, where you need to think about proper
>> namespaces, isolation, etc, etc, other kinds of interfaces might suit
>> better.
>>
>>>
>>>> given that for rumble you need calls - one ioctl to set up rumble
>>>> parameters, and a write to start the playback. The FF core should
>take
>>>> care of handling duration of the effect, ramping it up and
>decaying,
>>>> if desired, and we make sure to automatically stop effects when
>>>> userspace closes the fd (because of ordinary exit or crash or FD
>being
>>>> revoked).
>>>>
>>>>> If we want to say "lets move all vibrations from input to LED
>>>>> subsystem"... I don't think that is good idea, but its a valid
>>>>> discussion. Some good reasons would be needed.
>>>>>
>>>>> But having half devices use one interface and half use different
>one
>>>>> is just bad...
>>>>
>>>> Completely agree here. I just merged PWM vibra driver from
>Sebastian
>>>> Reichel, we already had regulator-haptic driver, do we need
>gpio-based
>>>> one? Or make regulator-based one working with fixed regulators?
>>>
>>> Just to clarify: the background of this discussion is the question
>>> whether we should remove the following lines from
>>> Documentation/leds/ledtrig-transient.txt:
>>>
>>> -As a specific example of this use-case, let's look at vibrate
>feature on
>>> -phones. Vibrate function on phones is implemented using PWM pins on
>SoC or
>>> -PMIC. There is a need to activate one shot timer to control the
>vibrate
>>> -feature, to prevent user space crashes leaving the phone in vibrate
>mode
>>> -permanently causing the battery to drain.
>>> whether we should remove the following use case example from
>>>
>>> In effect Pavel has objections to increasing ledtrig-transient
>>> interval accuracy by adding hr_timer support to it, because vibrate
>>> devices, as one of the use cases, can benefit from it.
>>>
>>> So there are two issues:
>>> 1. Addition of hr_timer support to LED trigger.
>>> 2. Removal of vibrate devices use case from ledtrig-transient doc.
>>>
>>> I am in favour of 1. and against 2. since we're not gaining anything
>>> by hiding information about some kernel functionality when it will
>>> still be there. It also doesn't define the location of any vibrate
>>> device drivers, since sheer leds-gpio driver can be used for that
>>> purpose.
>>
>> I would say that while leds-gpio can be used to do whatever (you can
>> wire PS/2 mouse to a set of gpios and probably manage to drive it
>> through a set of leds-gpio devices) it does not make it right
>> interface for everything. We do have a standard interface for haptic
>> (via rumble FF), at this moment I see no reason why we need another
>> one. It just confuses userspace as it now needs to implement multiple
>> interfaces, depending on the system it runs. Android expects that HAL
>> will take care of hiding all of that from their upper layers and does
>> not really pay close attention to kernel ABIs, but I think we should.
>
>One thing I noticed is that input_ff_create_memless() also does not
>use high-resolution timer hence it also does not have the stop-time
>precision that I'm looking for.

I'll take patches for high resolution timers in ff memless, they should also help with more accurate scheduling of ramping up and fading of events.

>
>Another thing is that ff_reply duration value is maxed-out at 32767 ms
>(u16) while the Android/userspace requires size long for performing
>long notification alert.
>

You need unattended vibrations lasting longer than 30 seconds? Anyway, replay.length == 0 means endless IIRC, so you only need to schedule stopping. We still will clean up properly if your process crashes or exits or closes the fd.


Thanks.

-- 
Dmitry