Re: PM regression with LED changes in next-20161109

[Hans de Goede]

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@xxxxxx> [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@xxxxxxxxxx>
> > > > > 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).