PM regression with LED changes in next-20161109

Tue Nov 15 02:01:26 PST 2016

Hi,

On 11/14/2016 01:51 PM, Hans de Goede wrote:
> Hi,
>
> On 14-11-16 10:12, Jacek Anaszewski wrote:
>> 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().
>
> Ugh, I see I accidentally send a v4 twice, instead of
> calling the version which dropped those called v5 as
> I should have, sorry.
>
> The v4 which I would like to see merged, the one with
> those calls dropped, is here:
>
> https://patchwork.kernel.org/patch/9423093/

Right I've had an impression that I've already seen something
different than "first" v4.

Regarding the patch - adding led_notify_brightness_change() to
brightness_store() can have similar power consumption related
implications if brightness is set frequently via sysfs. I'm leaning
towards adding a new brightness file similar to user_brightness
discussed in this thread.

It would cover shortcomings and read/write inconsistencies that
brightness file currently has, but without breaking existing users.

I'd not however go for "user_brightness" name due to the possible
brightness adjustments made autonomously by firmware. I'm afraid
that devising a meaningful name for the new file will be hard,
so the simplest would be just brighntess2. Dedicated section
in leds-class.txt should be devoted to it.

-- 
Best regards,
Jacek Anaszewski