Re: PM regression with LED changes in next-20161109

[Hans de Goede]

Hi,

On 11-11-16 23:12, Pavel Machek wrote:
> Hi!
>
> Reason #1:
>
> > > > 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.

That is not correct, the current behavior for eading the brightness
atrribute is to always return the current state.

> > 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 ?
>
> Reason #1 is above.

See my reply above.

> Reason #2 is "if userspace sees brightness file, it can not know if
> the notifications on change actually work or not".

If it needs to know that it can simply check the kernel version.

> Reason #3 is that you broke Tony's system. Polling does not make sense
> when trigger such as "CPU in use" is active.

Have you seen v4 of my patch? It fixes this while keeping the
polling on the brightness attribute itself, it basically goes
back (more or less) to v1 of my patch which did not have this
problem. I never wanted notification of trigger / blinking
changes because I already feared Tony's problem would happen.

> Reason #4 is that there are really two brightnesses:
>
> 1) maximum brightness trigger is going to use
>
> 2) current brightness
>
> Currently writing to "brightness" file changes 1), but reading returns
> 2) when available.

Right and Jacek has already said that we cannot change the
reading behavior on the brightness file because of ABI concerns.

So if anything we need a new blink_brightness file or such,
which when read shows the maximum brightness when blinking or
triggers are active. Note that we already have a max_brightness
file which is the actual maximum brightness the led supports.

Since the existing ABI behavior is for the existing brightness
file to return the *current* brightness, please explain to me
how polling on say the new blink_brightness file would make
sense to detect changes in the current brightness ?

> So, feel free to propose better interface. One that solves #1..#4
> above.

Proposal 1:

v4 of my patch, see the list. It solves all but #4, which
is out of scope for my patch, feel free to submit a patch to
solve #4 (with a new sysfs attr).

Proposal 2:

Add a new "user_brightness" file, which 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, show the brightness used when on when
blinking / triggers are active.

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

Regards,

Hans