Re: PM regression with LED changes in next-20161109
From: Jacek Anaszewski
Date: Mon Nov 14 2016 - 04:14:06 EST
Hi,
On 11/13/2016 02:52 PM, Hans de Goede wrote:
Hi,
On 13-11-16 12:44, Jacek Anaszewski wrote:
Hi,
On 11/12/2016 10:14 PM, Hans de Goede wrote:
<snip>
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 agree that user_brightness allows to solve the issues you raised
about inconsistent write and read brightness' semantics
(which is not that painful IMHO).
Reporting non-user brightness changes on user_brightness file
doesn't sound reasonable though.
The changes I'm interested in are user brightness changes they
are just not done through sysfs, but through a hardwired hotkey,
they are however very much done by the user.
Ah, so this file name would be misleading especially taking into account
the context in which "user" is used in kernel, which predominantly
means "userspace", e.g. copy_to_user(), copy_from_user().
Also, how would we read the
brightness set by the firmware? We'd have to read brightness
file, so still two files would have to be opened which is
a second drawback of this approach.
No, look carefully at the definition of the read behavior
I plan to put in the ABI doc:
OK, "user" was what confused me. So in this case changes made
by the firmware even if in a result of user activity
(pressing hardware key) obviously cannot be treated similarly
to the changes made from the userspace context.
In the end both result on the brightness of the device
changing, so any userspace process interested in monitoring
the brightness will want to know about both type of changes.
Unless you're able to give references to the kernel code which
contradict my judgement.
AFAIK the audio code will signal volume changes done by
hardwired buttons the same way as audio changes done
by userspace calling into the kernel. This also makes
sense because in the end, what is interesting for a
mixer app, is that the volume changed, and what the
new volume is.
OK, so it is indeed similar to your LED use case. Nonetheless
in case of LED controllers it is also possible that hardware
adjusts LED brightness in case of low battery voltage.
If a device is able e.g. to generate an interrupt to notify this
kind of event, then we would like also to be able to notify the client
about that. It wouldn't be user generated brightness change though.
"Reading this file will return the actual led brightness
when not blinking and no triggers are active; reading this
file will return the brightness used when the led is on
when blinking or triggers are active."
This is unnecessarily entangled. Blinking means timer trigger
is active.
Ok.
So for e.g. the backlit keyboard case reading this single
file will return the actual brightness of the backlight,
since this does not involve blinking or triggers.
Basically the idea is that the user_brightness file
will have the semantics which IMHO the brightness file
itself should have had from the beginning, but which
we can't change now due to ABI reasons.
And in fact introducing user_brightness file would indeed
fix that shortcoming. However without providing notifications
of hw brightness changes on it.
See above, I believe such a file should report any
changes in brightness, except those caused by triggers,
so it would report hw brightness changes.
Anyways if you're not interested in fixing the
shortcomings of the current read behavior on the
brightness file (I'm fine with that, I can live
with the shortcomings) I suggest that we simply go
with v2 of my poll() patch.
v2 entails power consumption related issues.
Generally I think that we could add the file you proposed,
however it would be good to devise a name which will cover
also the cases when brightness is changed by firmware without
user interaction.
Having no difference in this area between the two approaches
I'm still in favour of the read-only file for notifying
brightness changes procured by hardware.
That brings back the needing 2 fds problem; and does
not solve userspace not being able to reliably read
the led on brightness when blinking or using triggers.
And this also has the issue that one is doing poll() on
one fd to detect changes on another fd,
It is not necessarily true. We can treat the polling on
hw_brightness_change file as a means to detect brightness
changes procured by hardware and we can read that brightness
by executing read on this same fd. It could return -ENODATA
if no such an event has occurred so far.
That would still require 2 fds as userspace also wants to
be able to set the keyboard backlight, but allowing read()
on the hw_brightness_change file at least fixes the weirdness
where userspace gets woken from poll() without being able to
read. So if you insist on going the hw_brightness_change file
route, then I can live with that (and upower will simply
need to open 2 fds, that is doable).
But, BUT, I would greatly prefer to just go for v4 of my
patch, which fixes the only real problem we've seen with
my patch as original merged without adding a new, somewhat
convoluted sysfs attribute.
Hmm, v4 still calls led_notify_brightness_change(led_cdev)
from both __led_set_brightness() and __led_set_brightness_blocking().
--
Best regards,
Jacek Anaszewski