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

[David Lin]

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.

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.

David