Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751511AbdI1Fna (ORCPT ); Thu, 28 Sep 2017 01:43:30 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:37256 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750946AbdI1Fn1 (ORCPT ); Thu, 28 Sep 2017 01:43:27 -0400 X-Google-Smtp-Source: AOwi7QCfp0NyfJL612Xv8MgC1AiTNUt+OkpwkYVJ9Sn3RuiHv1oMApz8hXHwn2vQ5PxVrgBtNBaLCg== Date: Wed, 27 Sep 2017 22:43:22 -0700 User-Agent: K-9 Mail for Android In-Reply-To: References: <20170913175400.42744-1-dtwlin@google.com> <20170913202032.GA30844@amd> <9c75c3a9-4123-c7f3-7725-45ba752d672a@gmail.com> <20170914205804.GA24339@amd> <7a611993-ebaa-08bb-b10c-ebe4fb9ca33a@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Subject: Re: Vibrations in input vs. LED was Re: [PATCH v2 0/3] led: ledtrig-transient: add support for hrtimer To: David Lin CC: Jacek Anaszewski , Pavel Machek , "linux-input@vger.kernel.org" , Jonathan Corbet , Richard Purdie , Hans de Goede , Greg Kroah-Hartman , Rob Herring , Rom Lemarchand , "linux-doc@vger.kernel.org" , lkml , "linux-leds@vger.kernel.org" From: Dmitry Torokhov Message-ID: <744568EA-8FEE-4A0E-B431-F054B0827261@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8S5hZ0r021680 Content-Length: 6928 Lines: 172 On September 27, 2017 10:03:28 PM PDT, David Lin wrote: >Dmitry, > >On Fri, Sep 15, 2017 at 3:30 PM, Dmitry Torokhov > wrote: >> On Fri, Sep 15, 2017 at 2:55 PM, Jacek Anaszewski >> wrote: >>> On 09/15/2017 08:34 PM, Dmitry Torokhov wrote: >>>> On Thu, Sep 14, 2017 at 1:58 PM, Pavel Machek 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