Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630159 > unrolled thread

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer David Lin <dtwlin@google.com> - 2017-04-25 05:10 +0200
    Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-25 22:20 +0200
      Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer David Lin <dtwlin@google.com> - 2017-04-27 05:50 +0200
        Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-27 20:40 +0200
    Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer Pavel Machek <pavel@ucw.cz> - 2017-04-26 00:40 +0200
      Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-26 22:00 +0200

#1630159 — Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

Hi Jacek,

On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> Hi David,
>
> Thanks for the patch.
>
> Unfortunately we cannot switch to using hr timers just like that
> without introducing side effects for many devices. We had similar
> attempt of increasing timer tirgger accuracy two years ago [0].
>
> In short words, for drivers that can sleep while setting brightness
> and/or are using a bus like I2C you will not be able to enforce
> 1ms delay period.
>
> I recommend you to go through the thread [0] so that we had
> a well defined ground for the discussion on how to address this
> issue properly.
>

I think I understand the background now, and would agree that not all
the LED driver require hrtimer as human eye can't probably tell
there's a 10ms variation in a blink. However, there's a need to
support hrtimer if the LED subsystem claims support the use case of
vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
a 5ms of variation is perceivable to the user. I'm thinking if a
better interim solution is to introduce a
LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
compile time. Would you agree?

David

[toc] | [next] | [standalone]

#1630967

Hi David,

On 04/25/2017 05:05 AM, David Lin wrote:
> Hi Jacek,
> 
> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>>
>> Hi David,
>>
>> Thanks for the patch.
>>
>> Unfortunately we cannot switch to using hr timers just like that
>> without introducing side effects for many devices. We had similar
>> attempt of increasing timer tirgger accuracy two years ago [0].
>>
>> In short words, for drivers that can sleep while setting brightness
>> and/or are using a bus like I2C you will not be able to enforce
>> 1ms delay period.
>>
>> I recommend you to go through the thread [0] so that we had
>> a well defined ground for the discussion on how to address this
>> issue properly.
>>
> 
> I think I understand the background now, and would agree that not all
> the LED driver require hrtimer as human eye can't probably tell
> there's a 10ms variation in a blink.

The main problem are side effects occurring when an event
scheduled by hrtimer can't finish before the next one begins.
We get warnings like in the example below (copied from [0]) then,
and they have probably negative impact on the whole system performance.

echo "timer" > trigger
echo 1 > delay_on
echo 1 > delay_off
echo usec > delay_unit
[  178.584433] hrtimer: interrupt took 300747 ns

> However, there's a need to
> support hrtimer if the LED subsystem claims support the use case of
> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
> a 5ms of variation is perceivable to the user. I'm thinking if a
> better interim solution is to introduce a
> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
> compile time. Would you agree?

I think that it would be better if LED class driver set a flag
marking itself as capable of setting brightness with high rate.
I'd limit that only to leds-gpio and devices driven through
memory mapped registers.

Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
hr timers also to ledtrig-timer.

You can try also the other option mentioned by Pavel in [1].

[1] https://lkml.org/lkml/2017/4/24/881

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1631846

On Tue, Apr 25, 2017 at 1:15 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>> However, there's a need to
>> support hrtimer if the LED subsystem claims support the use case of
>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>> a 5ms of variation is perceivable to the user. I'm thinking if a
>> better interim solution is to introduce a
>> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
>> compile time. Would you agree?
>
> I think that it would be better if LED class driver set a flag
> marking itself as capable of setting brightness with high rate.
> I'd limit that only to leds-gpio and devices driven through
> memory mapped registers.
>
> Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
> hr timers also to ledtrig-timer.

Can I resubmit the patch implementing LED_BRIGHTNESS_FAST using hrtimer?

>
> You can try also the other option mentioned by Pavel in [1].

Thanks, Pavel. It does look like that input-ff is a more appropriate
subsystem for implementing a vibrator/haptics driver. It also seems
that it's relying on the userspace to control the timing of the
play/stop events which is likely to be less accurate than a hrtimer in
the kernel. But it provides more effect control than the LED
subsystem.

[toc] | [prev] | [next] | [standalone]

#1632329

On 04/27/2017 05:48 AM, David Lin wrote:
> On Tue, Apr 25, 2017 at 1:15 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>>> However, there's a need to
>>> support hrtimer if the LED subsystem claims support the use case of
>>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>>> a 5ms of variation is perceivable to the user. I'm thinking if a
>>> better interim solution is to introduce a
>>> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
>>> compile time. Would you agree?
>>
>> I think that it would be better if LED class driver set a flag
>> marking itself as capable of setting brightness with high rate.
>> I'd limit that only to leds-gpio and devices driven through
>> memory mapped registers.
>>
>> Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
>> hr timers also to ledtrig-timer.
> 
> Can I resubmit the patch implementing LED_BRIGHTNESS_FAST using hrtimer?

Yeah, but please split the changes into two patches:

1/2 - addition of a flag to linux/leds.h and corresponding update of
      Documentation/leds/leds-class.txt
2/2 - addition of hr timer support to ledtrig-transient.c

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1631042

[Multipart message — attachments visible in raw view] — view raw

On Mon 2017-04-24 20:05:59, David Lin wrote:
> Hi Jacek,
> 
> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
> >
> > Hi David,
> >
> > Thanks for the patch.
> >
> > Unfortunately we cannot switch to using hr timers just like that
> > without introducing side effects for many devices. We had similar
> > attempt of increasing timer tirgger accuracy two years ago [0].
> >
> > In short words, for drivers that can sleep while setting brightness
> > and/or are using a bus like I2C you will not be able to enforce
> > 1ms delay period.
> >
> > I recommend you to go through the thread [0] so that we had
> > a well defined ground for the discussion on how to address this
> > issue properly.
> >
> 
> I think I understand the background now, and would agree that not all
> the LED driver require hrtimer as human eye can't probably tell
> there's a 10ms variation in a blink. However, there's a need to
> support hrtimer if the LED subsystem claims support the use case of
> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
> a 5ms of variation is perceivable to the user. I'm thinking if a

I believe we should fix the documentation. It is LED subsystem,
requirements are different, and we _already_ have haptic feedback
subsystem.
								Pavel

IOW, I suggest this: (hmm, and more led->LED is needed, and more
english fixes. Oh well.)

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
index 3bd38b4..c5cf475 100644
--- a/Documentation/leds/ledtrig-transient.txt
+++ b/Documentation/leds/ledtrig-transient.txt
@@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
 goes away without deactivating the timer, the hardware will be left in that
 state permanently.
 
-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.
-
 Transient trigger addresses the need for one shot timer activation. The
 transient trigger can be enabled and disabled just like the other leds
 triggers.
 
-When an led class device driver registers itself, it can specify all leds
+When an LED class device driver registers itself, it can specify all leds
 triggers it supports and a default trigger. During registration, activation
 routine for the default trigger gets called. During registration of an led
 class device, the LED state does not change.
@@ -144,7 +138,6 @@ repeat the following step as needed:
 	echo none > trigger
 
 This trigger is intended to be used for for the following example use cases:
- - Control of vibrate (phones, tablets etc.) hardware by user space app.
  - Use of LED by user space app as activity indicator.
  - Use of LED by user space app as a kind of watchdog indicator -- as
        long as the app is alive, it can keep the LED illuminated, if it dies


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1631697

On 04/26/2017 12:34 AM, Pavel Machek wrote:
> On Mon 2017-04-24 20:05:59, David Lin wrote:
>> Hi Jacek,
>>
>> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>>
>>> Hi David,
>>>
>>> Thanks for the patch.
>>>
>>> Unfortunately we cannot switch to using hr timers just like that
>>> without introducing side effects for many devices. We had similar
>>> attempt of increasing timer tirgger accuracy two years ago [0].
>>>
>>> In short words, for drivers that can sleep while setting brightness
>>> and/or are using a bus like I2C you will not be able to enforce
>>> 1ms delay period.
>>>
>>> I recommend you to go through the thread [0] so that we had
>>> a well defined ground for the discussion on how to address this
>>> issue properly.
>>>
>>
>> I think I understand the background now, and would agree that not all
>> the LED driver require hrtimer as human eye can't probably tell
>> there's a 10ms variation in a blink. However, there's a need to
>> support hrtimer if the LED subsystem claims support the use case of
>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>> a 5ms of variation is perceivable to the user. I'm thinking if a
> 
> I believe we should fix the documentation. It is LED subsystem,
> requirements are different, and we _already_ have haptic feedback
> subsystem.
> 								Pavel
> 
> IOW, I suggest this: (hmm, and more led->LED is needed, and more
> english fixes. Oh well.)
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
> index 3bd38b4..c5cf475 100644
> --- a/Documentation/leds/ledtrig-transient.txt
> +++ b/Documentation/leds/ledtrig-transient.txt
> @@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
>  goes away without deactivating the timer, the hardware will be left in that
>  state permanently.
>  
> -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.
> -
>  Transient trigger addresses the need for one shot timer activation. The
>  transient trigger can be enabled and disabled just like the other leds
>  triggers.
>  
> -When an led class device driver registers itself, it can specify all leds
> +When an LED class device driver registers itself, it can specify all leds

Also: s/leds/LEDs/ :-)


>  triggers it supports and a default trigger. During registration, activation
>  routine for the default trigger gets called. During registration of an led
>  class device, the LED state does not change.
> @@ -144,7 +138,6 @@ repeat the following step as needed:
>  	echo none > trigger
>  
>  This trigger is intended to be used for for the following example use cases:
> - - Control of vibrate (phones, tablets etc.) hardware by user space app.
>   - Use of LED by user space app as activity indicator.
>   - Use of LED by user space app as a kind of watchdog indicator -- as
>         long as the app is alive, it can keep the LED illuminated, if it dies
> 
> 

Ack. Will you submit an official patch?

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web