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

Groups > linux.kernel > #1520223

Re: PM regression with LED changes in next-20161109

Show key headers only | View raw

Hi,

On 12-11-16 11:24, Jacek Anaszewski wrote:
> Hi,
>
> On 11/11/2016 08:28 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 11-11-16 18:03, Jacek Anaszewski wrote:
>>> On 11/11/2016 01:01 PM, Pavel Machek wrote:
>>>> On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/10/2016 09:29 PM, Pavel Machek wrote:
>>>>>> On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
>>>>>>> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for
>>>>>>>>>>>> poll()ing
>>>>>>>>>>>> the sysfs brightness attr for changes.") breaks runtime PM
>>>>>>>>>>>> for me.
>>>>>>>>>>>>
>>>>>>>>>>>> On my omap dm3730 based test system, idle power consumption
>>>>>>>>>>>> is over 70
>>>>>>>>>>>> times higher now with this patch! It goes from about 6mW for
>>>>>>>>>>>> the core
>>>>>>>>>>>> system to over 440mW during idle meaning there's some busy
>>>>>>>>>>>> timer now
>>>>>>>>>>>> active.
>>>>>>>>>>>>
>>>>>>>>>>>> Reverting this patch fixes the issue. Any ideas?
>>>>>>>>
>>>>>>>> Are you using any LED that toggles with high frequency? Like perhaps
>>>>>>>> LED that is lit when CPU is active?
>>>>>>>
>>>>>>> Yeah one of them seems to have cpu0 as the default trigger.
>>>>>>
>>>>>> Aha. Its quite obvious we don't want to notify sysfs each time that
>>>>>> one is toggled, right?
>>>>>>
>>>>>> IMO brightness should display max brightness for the trigger, as Hans
>>>>>> suggested, anything else is madness for trigger such as cpu activity.
>>>>>
>>>>> Are you suggesting that we should revert changes introduced
>>>>> by below patch?
>>>>>
>>>>> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
>>>>> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>>>>> Date:   Tue Mar 18 09:47:48 2008 +0000
>>>>>
>>>>>     leds: Add support to leds with readable status
>>>>>
>>>>>     Some led hardware allows drivers to query the led state, and
>>>>> this patch
>>>>>     adds a hook to let the led class take advantage of that
>>>>> information when
>>>>>     available.
>>>>>
>>>>>     Without this functionality, when access to the led hardware is not
>>>>>     exclusive (i.e. firmware or hardware might change its state
>>>>> behind the
>>>>>     kernel's back), reality goes out of sync with the led class'
>>>>> idea of
>>>>> what
>>>>>     the led is doing, which is annoying at best.
>>>>
>>>> Hmm. So userland can read the LED state, and it can get _some_ value
>>>> back, but it can not know if it is current state or not.
>>>>
>>>> I don't think that's a good interface. I see it is from 2008... is
>>>> someone using it? Maybe it is too late for revert.
>>>
>>> I can imagine it being used in flash LED use case. E.g. one
>>> could use oneshot trigger to trigger flash strobe, and then
>>> he could periodically read brightness file to check, for whatever
>>> reason, whether the flash is strobing.
>>>
>>>> But I'd certainly not extend it with poll.
>>>
>>> We could add a dedicated file e.g. hw_brightness_change for that
>>> (maybe someone will have a better candidate for the file name).
>>
>> Why a dedicated file? Are we going to mirror brightness here
>> wrt r/w (show/store) behavior ? If not userspace now needs
>> 2 open fds which is not really nice. If we are and we are
>> not going to use poll for something else on brightness itself
>> then why not just poll directly on brightness ?
>
> My main concern is that reporting only hw brightness changes
> wouldn't be consistent with general brightness file purpose.
> One could expect that brightness changes made by triggers
> should be also reported.

Ok, I agree that not notifying poll() while an actual
read() would result in a different value is not really good
semantics.

I don't like to call it hw_brightness_change though, as
mentioned before I believe that if we were to start with
a clean slate we would make the brightness file's read/write
behavior more a mirror of itself.

So I would like to propose creating a new read-write
user_brightness file.

The write behavior would be 100% identical to the brightness
file (in code terms it will call the same store function).

The the read behavior otoh will be different: it will shows
the last brightness as set by the user, this would show the
read behavior we really want of brightness: show the real
brightness when not blinking / triggers are active and show
the brightness used when on when blinking / triggers are active.

We could then add poll support on this new user_brightness
file, thus avoiding the problem with the extra cpu-load on
notifications on blinking / triggers.

> I'd make it only readable, so it wouldn't mirror brightness
> file behavior.

Then userspace which wants to be able to read + write + poll
the brightness again needs to open 2 fds, as suggested
above for the new user_brightness file it will be easy
to just make it mimic the brightness file write behavior
and then userspace only needs to open one fd.

Regards,

Hans




>
> Its purpose would be clear: notify hw brightness changes
> and provide the brightness value that was set by the hardware
> last time. It implies that this value could be different from
> the one the brightness file reports. E.g. hw could have changed
> brightness, which could be later updated through brightness
> file, but hw_brightness_change would still report brightness level
> that was set by the hardware last time. It could be useful
> e.g. in case of showing the difference between the desired
> value and the currently allowed configuration (e.g. if the
> firmware automatically adjusted the value set by the user).
>

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-09 20:30 +0100
  Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-09 21:50 +0100
    Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:50 +0100
      Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 14:00 +0100
        Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 14:10 +0100
          Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 15:00 +0100
            Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:40 +0100
        Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:30 +0100
          Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 17:50 +0100
            Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:50 +0100
              Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 09:30 +0100
          Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 19:10 +0100
            Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:30 +0100
              Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-10 22:40 +0100
                Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 13:10 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-11 18:10 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 20:30 +0100
                Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 23:20 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 09:10 +0100
                Three different LED brightnesses (was Re: PM regression with LED  changes in next-20161109) Pavel Machek <pavel@ucw.cz> - 2016-11-13 10:20 +0100
                Re: Three different LED brightnesses (was Re: PM regression with LED  changes in next-20161109) Hans de Goede <hdegoede@redhat.com> - 2016-11-13 10:50 +0100
                Re: Three different LED brightnesses (was Re: PM regression with LED  changes in next-20161109) Pavel Machek <pavel@ucw.cz> - 2016-11-13 21:50 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-12 11:30 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 11:50 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-12 20:20 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 22:20 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-13 12:50 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-13 15:00 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-14 10:20 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-14 14:00 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 11:10 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 11:10 +0100
                LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 11:40 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 12:00 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 12:20 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 12:20 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 12:30 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 12:50 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 13:10 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 13:20 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 14:30 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 14:50 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 15:10 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 15:40 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 15:50 +0100
                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-17 23:20 +0100
                Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-14 09:40 +0100
                Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 23:10 +0100
  Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:40 +0100
    Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 16:20 +0100

csiph-web