Message-ID: <3eb60c78-d891-27e5-6b7b-a54a5b547a1c@redhat.com>
Date:   Sat, 12 Nov 2016 11:33:26 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>
Cc:     Tony Lindgren <tony@...mide.com>,
        Jacek Anaszewski <j.anaszewski@...sung.com>,
        linux-leds@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Darren Hart <dvhart@...radead.org>
Subject: Re: PM regression with LED changes in next-20161109

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@....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@....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).
>

Powered by blists - more mailing lists