Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520219 > unrolled thread

Re: PM regression with LED changes in next-20161109

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-12 11:30 +0100
    Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 11:50 +0100
      Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-12 20:20 +0100
        Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 22:20 +0100
          Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-13 12:50 +0100
            Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-13 15:00 +0100
              Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-14 10:20 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-14 14:00 +0100
                  Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 11:10 +0100
                  Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 11:10 +0100
                  LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 11:40 +0100
                    Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 12:00 +0100
                      Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 12:20 +0100
                      Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 12:20 +0100
                        Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 12:30 +0100
                          Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 12:50 +0100
                            Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 13:10 +0100
                              Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-15 13:20 +0100
                              Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 14:30 +0100
                                Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 14:50 +0100
                                  Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 15:10 +0100
                                    Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-15 15:40 +0100
                                      Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-15 15:50 +0100
                              Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM  regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-17 23:20 +0100
          Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-14 09:40 +0100

Page 1 of 2  [1] 2  Next page →

#1520219 — Re: PM regression with LED changes in next-20161109

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

I'd make it only readable, so it wouldn't mirror brightness
file behavior.

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

-- 
Best regards,
Jacek Anaszewski

[toc] | [next] | [standalone]

#1520223

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

[toc] | [prev] | [next] | [standalone]

#1520360

Hi,

On 11/12/2016 11:33 AM, Hans de Goede wrote:
> 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@ucw.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@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 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. 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.

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.

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

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1520379

Hi,

On 12-11-16 20:14, Jacek Anaszewski wrote:

<snip>

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

> 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:

"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."

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.

> 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, which is completely
unheard of in any kernel API, so I still vote NACK for the
entire idea of having a different file purely for notifying
changes. The way unix defines poll to work means that the
poll() and read() must be on the same fd, anything else
does not make sense.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1520590

Hi,

On 11/12/2016 10:14 PM, Hans de Goede wrote:
> Hi,
>
> On 12-11-16 20:14, Jacek Anaszewski wrote:
>
> <snip>
>
>>>>> 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 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.

Unless you're able to give references to the kernel code which
contradict my judgement.

>
> "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.

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

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

> which is completely
> unheard of in any kernel API, so I still vote NACK for the
> entire idea of having a different file purely for notifying
> changes. The way unix defines poll to work means that the
> poll() and read() must be on the same fd, anything else
> does not make sense.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1520604

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.

>> "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.

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

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1521446

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

[toc] | [prev] | [next] | [standalone]

#1521625

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/

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1522515

Hi,

On 15-11-16 11:01, Jacek Anaszewski wrote:
> Hi,
>
> On 11/14/2016 01:51 PM, Hans de Goede wrote:

<snip>

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

That means that userspace is waking up frequently to write the
sysfs file, so in that case userspace is already draining
a lot of energy, so I don't think that is something we need to
worry about.

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

Ok, let me quote myself from another part of this thread:

We've 2 sorts of brightness really:

1) transient brightness, aka current brightness, when blinking or
triggers are used this will switch many times a second
between off and some on level.

2) non-transient brightness, for non blinking leds this is the
actual brightness, for blinking leds this is the brightness
level used when the led is on.

Now we want to have a sysfs attribute reflecting 2, so that
userspace can poll on that, both for my use-case as well as so
that userspace process a can detect changes made by writing to
the brightness file by process b.

So maybe we need to simply call the new attribute
non_transient_brightness instead of user_brightness?

non_transient_brightness certainly seems like a better name
then brightness2 ?

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1522516

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

[toc] | [prev] | [next] | [standalone]

#1522534 — LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >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/

Please, lets fix this properly.

The LED you are talking about _has_ a trigger, implemented in
hardware. That trigger can change LED brightness behind kernel's (and
userspace's) back. Don't pretend the trigger does not exist, it does.

And when you do that, you'll have nice place to report changes to
userspace -- trigger can now export that information, and offer poll()
interface.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1522578 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

On 11/15/2016 11:31 AM, Pavel Machek wrote:
> Hi!
>
>>> 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/
>
> Please, lets fix this properly.
>
> The LED you are talking about _has_ a trigger, implemented in
> hardware. That trigger can change LED brightness behind kernel's (and
> userspace's) back. Don't pretend the trigger does not exist, it does.
>
> And when you do that, you'll have nice place to report changes to
> userspace -- trigger can now export that information, and offer poll()
> interface.

Well, that sounds interesting. It is logically justifiable.
I initially proposed exactly this solution, with recently
added userspace LED being a trigger listener. It seems a bit
awkward though. How would you listen to the trigger events?

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1522585 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

HI,

On 15-11-16 11:58, Jacek Anaszewski wrote:
> On 11/15/2016 11:31 AM, Pavel Machek wrote:
>> Hi!
>>
>>>> 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/
>>
>> Please, lets fix this properly.
>>
>> The LED you are talking about _has_ a trigger, implemented in
>> hardware. That trigger can change LED brightness behind kernel's (and
>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>
>> And when you do that, you'll have nice place to report changes to
>> userspace -- trigger can now export that information, and offer poll()
>> interface.
>
> Well, that sounds interesting. It is logically justifiable.
> I initially proposed exactly this solution, with recently
> added userspace LED being a trigger listener. It seems a bit
> awkward though. How would you listen to the trigger events?

We could make the trigger sysfs attribute poll()-able, but only
for select triggers, e.g.:

Documentation/ABI/testing/sysfs-class-led

What:           /sys/class/leds/<led>/trigger
Date:           March 2006
KernelVersion:  2.6.17
Contact:        Richard Purdie <rpurdie@rpsys.net>
Description:
                 Set the trigger for this LED. A trigger is a kernel based source
                 of led events.
                 You can change triggers in a similar manner to the way an IO
                 scheduler is chosen. Trigger specific parameters can appear in
                 /sys/class/leds/<led> once a given trigger is selected. For
                 their documentation see sysfs-class-led-trigger-*.
+
+		For some triggers userspace my poll() this file, watching for
+		POLL_PRI to detect when the trigger triggers. This is only
+		supported if this is explicitly mentioned as supported in
+		sysfs-class-led-trigger-* for the selected trigger.

The reason for making this only supported for select triggers is to
avoid getting the whole power-consumption issue from triggers which fire
frequently again.

And then we could add a new:

Documentation/ABI/testing/sysfs-class-led-trigger-kbd-backlight-change

File which documents that the new to be added kbd-backlight-change
trigger is poll-able.

We would also need:

--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -47,6 +47,7 @@ struct led_classdev {
  #define LED_DEV_CAP_FLASH      (1 << 18)
  #define LED_HW_PLUGGABLE       (1 << 19)
  #define LED_PANIC_INDICATOR    (1 << 20)
+#define LED_TRIGGER_READ_ONLY   (1 << 21)

         /* set_brightness_work / blink_timer flags, atomic, private. */
         unsigned long           work_flags;

To allow led drivers to indicate that there trigger is hardwired.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1522586 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

[Multipart message — attachments visible in raw view] — view raw

On Tue 2016-11-15 11:58:06, Jacek Anaszewski wrote:
> On 11/15/2016 11:31 AM, Pavel Machek wrote:
> >Hi!
> >
> >>>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/
> >
> >Please, lets fix this properly.
> >
> >The LED you are talking about _has_ a trigger, implemented in
> >hardware. That trigger can change LED brightness behind kernel's (and
> >userspace's) back. Don't pretend the trigger does not exist, it does.
> >
> >And when you do that, you'll have nice place to report changes to
> >userspace -- trigger can now export that information, and offer poll()
> >interface.
> 
> Well, that sounds interesting. It is logically justifiable.

Thanks.

> I initially proposed exactly this solution, with recently
> added userspace LED being a trigger listener. It seems a bit
> awkward though. How would you listen to the trigger events?

Trigger exposes a file in sysfs, with poll() working on that file (and
probably read exposing the current brightness).

Key difference is that only triggers where this makes sense (keyboard
backlight) expose it and carry the overhead. CPU trigger would
definitely not do this.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1522598 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

Hi,

On 15-11-16 12:11, Pavel Machek wrote:
> On Tue 2016-11-15 11:58:06, Jacek Anaszewski wrote:
>> On 11/15/2016 11:31 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> 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/
>>>
>>> Please, lets fix this properly.
>>>
>>> The LED you are talking about _has_ a trigger, implemented in
>>> hardware. That trigger can change LED brightness behind kernel's (and
>>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>>
>>> And when you do that, you'll have nice place to report changes to
>>> userspace -- trigger can now export that information, and offer poll()
>>> interface.
>>
>> Well, that sounds interesting. It is logically justifiable.
>
> Thanks.
>
>> I initially proposed exactly this solution, with recently
>> added userspace LED being a trigger listener. It seems a bit
>> awkward though. How would you listen to the trigger events?
>
> Trigger exposes a file in sysfs, with poll() working on that file

Hmm, a new file would give the advantage of making it easy for
userspace to see if the trigger is poll-able, this is likely
better then my own proposal I just send.

> (and
> probably read exposing the current brightness).

If we do this, can we please make it mirror brightness, iow
also make it writable, that will make it easier for userspace
to deal with it. We can simply re-use the existing show / store
methods for brightness for this.

I suggest we call it:

trigger_brightness

And only register it when a poll-able trigger is present.


> Key difference is that only triggers where this makes sense (keyboard
> backlight) expose it and carry the overhead. CPU trigger would
> definitely not do this.

Ack only having some triggers pollable is important.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1522606 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>>The LED you are talking about _has_ a trigger, implemented in
> >>>hardware. That trigger can change LED brightness behind kernel's (and
> >>>userspace's) back. Don't pretend the trigger does not exist, it does.
> >>>
> >>>And when you do that, you'll have nice place to report changes to
> >>>userspace -- trigger can now export that information, and offer poll()
> >>>interface.
> >>
> >>Well, that sounds interesting. It is logically justifiable.
> >
> >Thanks.
> >
> >>I initially proposed exactly this solution, with recently
> >>added userspace LED being a trigger listener. It seems a bit
> >>awkward though. How would you listen to the trigger events?
> >
> >Trigger exposes a file in sysfs, with poll() working on that file
> 
> Hmm, a new file would give the advantage of making it easy for
> userspace to see if the trigger is poll-able, this is likely
> better then my own proposal I just send.

Good.

> >(and
> >probably read exposing the current brightness).
> 
> If we do this, can we please make it mirror brightness, iow
> also make it writable, that will make it easier for userspace
> to deal with it. We can simply re-use the existing show / store
> methods for brightness for this.

Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
that here, you want to be able to turn off the backlight but still
keep the trigger (and be notified of future changes).

> I suggest we call it:
> 
> trigger_brightness
> 
> And only register it when a poll-able trigger is present.

I'd call it 'current_brightness', but that's no big deal. Yes, only
registering it for poll-able triggers makes sense.

> >Key difference is that only triggers where this makes sense (keyboard
> >backlight) expose it and carry the overhead. CPU trigger would
> >definitely not do this.
> 
> Ack only having some triggers pollable is important.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1522620 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

Hi,

On 15-11-16 12:48, Pavel Machek wrote:
> Hi!
>
>>>>> The LED you are talking about _has_ a trigger, implemented in
>>>>> hardware. That trigger can change LED brightness behind kernel's (and
>>>>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>>>>
>>>>> And when you do that, you'll have nice place to report changes to
>>>>> userspace -- trigger can now export that information, and offer poll()
>>>>> interface.
>>>>
>>>> Well, that sounds interesting. It is logically justifiable.
>>>
>>> Thanks.
>>>
>>>> I initially proposed exactly this solution, with recently
>>>> added userspace LED being a trigger listener. It seems a bit
>>>> awkward though. How would you listen to the trigger events?
>>>
>>> Trigger exposes a file in sysfs, with poll() working on that file
>>
>> Hmm, a new file would give the advantage of making it easy for
>> userspace to see if the trigger is poll-able, this is likely
>> better then my own proposal I just send.
>
> Good.
>
>>> (and
>>> probably read exposing the current brightness).
>>
>> If we do this, can we please make it mirror brightness, iow
>> also make it writable, that will make it easier for userspace
>> to deal with it. We can simply re-use the existing show / store
>> methods for brightness for this.
>
> Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
> that here, you want to be able to turn off the backlight but still
> keep the trigger (and be notified of future changes).

True, that is easy to do the store method will just need to call
led_set_brightness_nosleep instead of led_set_brightness, this
will skip the checks to stop blinking in led_set_brightness and
otherwise is equivalent.

>> I suggest we call it:
>>
>> trigger_brightness
>>
>> And only register it when a poll-able trigger is present.
>
> I'd call it 'current_brightness', but that's no big deal. Yes, only
> registering it for poll-able triggers makes sense.

current_brightness works for me. I will take a shot a patch-set
implementing this.

Regards,

Hans



>
>>> Key difference is that only triggers where this makes sense (keyboard
>>> backlight) expose it and carry the overhead. CPU trigger would
>>> definitely not do this.
>>
>> Ack only having some triggers pollable is important.
>
> Thanks,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

#1522629 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

[Multipart message — attachments visible in raw view] — view raw

On Tue 2016-11-15 13:06:14, Hans de Goede wrote:
> Hi,
> 
> On 15-11-16 12:48, Pavel Machek wrote:
> >Hi!
> >
> >>>>>The LED you are talking about _has_ a trigger, implemented in
> >>>>>hardware. That trigger can change LED brightness behind kernel's (and
> >>>>>userspace's) back. Don't pretend the trigger does not exist, it does.
> >>>>>
> >>>>>And when you do that, you'll have nice place to report changes to
> >>>>>userspace -- trigger can now export that information, and offer poll()
> >>>>>interface.
> >>>>
> >>>>Well, that sounds interesting. It is logically justifiable.
> >>>
> >>>Thanks.
> >>>
> >>>>I initially proposed exactly this solution, with recently
> >>>>added userspace LED being a trigger listener. It seems a bit
> >>>>awkward though. How would you listen to the trigger events?
> >>>
> >>>Trigger exposes a file in sysfs, with poll() working on that file
> >>
> >>Hmm, a new file would give the advantage of making it easy for
> >>userspace to see if the trigger is poll-able, this is likely
> >>better then my own proposal I just send.
> >
> >Good.
> >
> >>>(and
> >>>probably read exposing the current brightness).
> >>
> >>If we do this, can we please make it mirror brightness, iow
> >>also make it writable, that will make it easier for userspace
> >>to deal with it. We can simply re-use the existing show / store
> >>methods for brightness for this.
> >
> >Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
> >that here, you want to be able to turn off the backlight but still
> >keep the trigger (and be notified of future changes).
> 
> True, that is easy to do the store method will just need to call
> led_set_brightness_nosleep instead of led_set_brightness, this
> will skip the checks to stop blinking in led_set_brightness and
> otherwise is equivalent.
> 
> >>I suggest we call it:
> >>
> >>trigger_brightness
> >>
> >>And only register it when a poll-able trigger is present.
> >
> >I'd call it 'current_brightness', but that's no big deal. Yes, only
> >registering it for poll-able triggers makes sense.
> 
> current_brightness works for me. I will take a shot a patch-set
> implementing this.

Thanks!
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1522723 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

On 11/15/2016 01:06 PM, Hans de Goede wrote:
> Hi,
>
> On 15-11-16 12:48, Pavel Machek wrote:
>> Hi!
>>
>>>>>> The LED you are talking about _has_ a trigger, implemented in
>>>>>> hardware. That trigger can change LED brightness behind kernel's (and
>>>>>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>>>>>
>>>>>> And when you do that, you'll have nice place to report changes to
>>>>>> userspace -- trigger can now export that information, and offer
>>>>>> poll()
>>>>>> interface.
>>>>>
>>>>> Well, that sounds interesting. It is logically justifiable.
>>>>
>>>> Thanks.
>>>>
>>>>> I initially proposed exactly this solution, with recently
>>>>> added userspace LED being a trigger listener. It seems a bit
>>>>> awkward though. How would you listen to the trigger events?
>>>>
>>>> Trigger exposes a file in sysfs, with poll() working on that file
>>>
>>> Hmm, a new file would give the advantage of making it easy for
>>> userspace to see if the trigger is poll-able, this is likely
>>> better then my own proposal I just send.
>>
>> Good.
>>
>>>> (and
>>>> probably read exposing the current brightness).
>>>
>>> If we do this, can we please make it mirror brightness, iow
>>> also make it writable, that will make it easier for userspace
>>> to deal with it. We can simply re-use the existing show / store
>>> methods for brightness for this.
>>
>> Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
>> that here, you want to be able to turn off the backlight but still
>> keep the trigger (and be notified of future changes).
>
> True, that is easy to do the store method will just need to call
> led_set_brightness_nosleep instead of led_set_brightness, this
> will skip the checks to stop blinking in led_set_brightness and
> otherwise is equivalent.
>
>>> I suggest we call it:
>>>
>>> trigger_brightness
>>>
>>> And only register it when a poll-able trigger is present.
>>
>> I'd call it 'current_brightness', but that's no big deal. Yes, only
>> registering it for poll-able triggers makes sense.
>
> current_brightness works for me. I will take a shot a patch-set
> implementing this.

Word "current" is not precise here.

It can be thought of as either last brightness set by the
user or the brightness currently written to the device
(returned by brightness file).

There is a semantic discrepancy in our requirements -
we want the file representing both permanent brightness
set by the user and brightness set by the hardware.

The two stand in contradiction to each other since
brightness set by the user can be adjusted by the hardware.

Reading the file shouldn't update brightness property of
struct led_classdev, so it shouldn't call led_update_brightness()
but it still should allow reading brightness set by the
hardware, as a result of each POLLPRI event. So in fact in
the same time it should report both according to our requirements
which is impossible. Do we need three brightness files ?

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1522734 — Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

Hi,

On 15-11-16 14:28, Jacek Anaszewski wrote:
> On 11/15/2016 01:06 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 15-11-16 12:48, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>> The LED you are talking about _has_ a trigger, implemented in
>>>>>>> hardware. That trigger can change LED brightness behind kernel's (and
>>>>>>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>>>>>>
>>>>>>> And when you do that, you'll have nice place to report changes to
>>>>>>> userspace -- trigger can now export that information, and offer
>>>>>>> poll()
>>>>>>> interface.
>>>>>>
>>>>>> Well, that sounds interesting. It is logically justifiable.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>> I initially proposed exactly this solution, with recently
>>>>>> added userspace LED being a trigger listener. It seems a bit
>>>>>> awkward though. How would you listen to the trigger events?
>>>>>
>>>>> Trigger exposes a file in sysfs, with poll() working on that file
>>>>
>>>> Hmm, a new file would give the advantage of making it easy for
>>>> userspace to see if the trigger is poll-able, this is likely
>>>> better then my own proposal I just send.
>>>
>>> Good.
>>>
>>>>> (and
>>>>> probably read exposing the current brightness).
>>>>
>>>> If we do this, can we please make it mirror brightness, iow
>>>> also make it writable, that will make it easier for userspace
>>>> to deal with it. We can simply re-use the existing show / store
>>>> methods for brightness for this.
>>>
>>> Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
>>> that here, you want to be able to turn off the backlight but still
>>> keep the trigger (and be notified of future changes).
>>
>> True, that is easy to do the store method will just need to call
>> led_set_brightness_nosleep instead of led_set_brightness, this
>> will skip the checks to stop blinking in led_set_brightness and
>> otherwise is equivalent.
>>
>>>> I suggest we call it:
>>>>
>>>> trigger_brightness
>>>>
>>>> And only register it when a poll-able trigger is present.
>>>
>>> I'd call it 'current_brightness', but that's no big deal. Yes, only
>>> registering it for poll-able triggers makes sense.
>>
>> current_brightness works for me. I will take a shot a patch-set
>> implementing this.
>
> Word "current" is not precise here.
>
> It can be thought of as either last brightness set by the
> user or the brightness currently written to the device
> (returned by brightness file).
>
> There is a semantic discrepancy in our requirements -
> we want the file representing both permanent brightness
> set by the user and brightness set by the hardware.
>
> The two stand in contradiction to each other since
> brightness set by the user can be adjusted by the hardware.
>
> Reading the file shouldn't update brightness property of
> struct led_classdev, so it shouldn't call led_update_brightness()
> but it still should allow reading brightness set by the
> hardware, as a result of each POLLPRI event. So in fact in
> the same time it should report both according to our requirements
> which is impossible. Do we need three brightness files ?

I don't think so, current_brightness actually is an accurate
name, if the brightness was last changed by writing from
sysfs, the keyboard backlight will honor that and the current_brightness
attribute will show the brightness last set through writing it,
which matches the actual current brightness of the keyboard backlight.

Likewise if it was changed with the hotkey last then the keyboard
backlight brightness will be changed and reading from current_brightness
will return the new actual brightness. Basically reading from this
file will be no different then reading from the normal "brightness"
file the difference will be in that it is poll-able and that
writing 0 turns off the LED without stopping blinking.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web