Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1518430 > unrolled thread
| Started by | Tony Lindgren <tony@atomide.com> |
|---|---|
| First post | 2016-11-09 20:30 +0100 |
| Last post | 2016-11-10 16:20 +0100 |
| Articles | 10 on this page of 50 — 5 participants |
Back to article view | Back to linux.kernel
PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-09 20:30 +0100
Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-09 21:50 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:50 +0100
Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 14:00 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 14:10 +0100
Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 15:00 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:40 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:30 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 17:50 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:50 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 09:30 +0100
Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 19:10 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:30 +0100
Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-10 22:40 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 13:10 +0100
Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-11 18:10 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 20:30 +0100
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 23:20 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-12 09:10 +0100
Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Pavel Machek <pavel@ucw.cz> - 2016-11-13 10:20 +0100
Re: Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Hans de Goede <hdegoede@redhat.com> - 2016-11-13 10:50 +0100
Re: Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Pavel Machek <pavel@ucw.cz> - 2016-11-13 21:50 +0100
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
Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 23:10 +0100
Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:40 +0100
Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 16:20 +0100
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Jacek Anaszewski <j.anaszewski@samsung.com> |
|---|---|
| Date | 2016-11-15 14:30 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sDLYe-5sD-39@gated-at.bofh.it> |
| In reply to | #1522620 |
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]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2016-11-15 14:50 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sDMhz-5yN-11@gated-at.bofh.it> |
| In reply to | #1522723 |
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]
| From | Jacek Anaszewski <j.anaszewski@samsung.com> |
|---|---|
| Date | 2016-11-15 15:10 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sDMAX-5VL-37@gated-at.bofh.it> |
| In reply to | #1522734 |
On 11/15/2016 02:48 PM, Hans de Goede wrote: > 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. If so then when software blinking enabled, it will return 0 on low blink cycle no matter what current brightness level is. -- Best regards, Jacek Anaszewski
[toc] | [prev] | [next] | [standalone]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2016-11-15 15:40 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sDN3X-69H-27@gated-at.bofh.it> |
| In reply to | #1522752 |
Hi, On 15-11-16 15:04, Jacek Anaszewski wrote: > On 11/15/2016 02:48 PM, Hans de Goede wrote: >> 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. > > If so then when software blinking enabled, it will return 0 on low > blink cycle no matter what current brightness level is. For software blinking there will not be a current_brightness atrribute, as we will only add that for LEDs with poll-able triggers. Also during the low cycle the LED is off, so its brightness at the moment of reading (iow currently) is 0. Regards, Hans
[toc] | [prev] | [next] | [standalone]
| From | Jacek Anaszewski <j.anaszewski@samsung.com> |
|---|---|
| Date | 2016-11-15 15:50 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sDNdD-6dg-9@gated-at.bofh.it> |
| In reply to | #1522779 |
On 11/15/2016 03:30 PM, Hans de Goede wrote: > Hi, > > On 15-11-16 15:04, Jacek Anaszewski wrote: >> On 11/15/2016 02:48 PM, Hans de Goede wrote: >>> 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. >> >> If so then when software blinking enabled, it will return 0 on low >> blink cycle no matter what current brightness level is. > > For software blinking there will not be a current_brightness atrribute, > as we will only add that for LEDs with poll-able triggers. > > Also during the low cycle the LED is off, so its brightness at the > moment of reading (iow currently) is 0. OK, I wanted to make sure that we know what we've agreed on. Earlier there were doubts raised about brightness during software blinking being periodically reported 0, but actually your reasoning seems to be the best answer to that. -- Best regards, Jacek Anaszewski
[toc] | [prev] | [next] | [standalone]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2016-11-17 23:20 +0100 |
| Subject | Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 |
| Message-ID | <sEDce-6KN-17@gated-at.bofh.it> |
| In reply to | #1522620 |
Hi, On 15-11-16 13:06, 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. Done, this actually turned out pretty nice, the trigger also helps in propagating the change events from dell-wmi to the led-classdev in dell-laptop without needing the ugly hacks I needed before. v5 coming up. 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]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2016-11-14 09:40 +0100 |
| Message-ID | <sDkY1-4yt-7@gated-at.bofh.it> |
| In reply to | #1520379 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > >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." That's not sane semantics. Userspace would have to read three files (racy) to find out about triggers etc. It also prevents modelling "hardware-changes-brightness-behind-kernel's-back" as a trigger. > 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. Stop obsessing about "ingle file". FDs are pretty cheap. This is sysfs. > >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 Actually you have turned "2 fds" into "3 fds", as userspace now needs to check for trigger _and_ blinking _and_ actuall brightness. fds are cheap, but that is still not nice design. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2016-11-11 23:10 +0100 |
| Message-ID | <sCsbg-1Q9-15@gated-at.bofh.it> |
| In reply to | #1519914 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > >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. I'm pretty sure nobody does that for flah strobe. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2016-11-10 09:40 +0100 |
| Message-ID | <sBT3Q-3a8-19@gated-at.bofh.it> |
| In reply to | #1518430 |
Hi,
On 09-11-16 20:23, Tony Lindgren wrote:
> 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.
Do you have any blinking LEDs or LED triggers defined on the system ?
> Reverting this patch fixes the issue. Any ideas?
All I can think of is something calling led_set_brightness quite often,
the patch in question makes led_set_brightness somewhat more expensive,
but it should not cause such a big difference unless something is
really calling led_set_brightness quite often maybe something is calling
it with the same value all the time ?
Regards,
Hans
[toc] | [prev] | [next] | [standalone]
| From | Tony Lindgren <tony@atomide.com> |
|---|---|
| Date | 2016-11-10 16:20 +0100 |
| Message-ID | <sBZj1-7Uk-19@gated-at.bofh.it> |
| In reply to | #1518740 |
* Hans de Goede <hdegoede@redhat.com> [161110 01:35]:
> Hi,
>
> On 09-11-16 20:23, Tony Lindgren wrote:
> > 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.
>
> Do you have any blinking LEDs or LED triggers defined on the system ?
There are some configured in the dts file:
$ grep -i led arch/arm/boot/dts/*torpedo*.dts*
And the gpio controlled led1 is configured to blink with
linux,default-trigger = "cpu0".
> > Reverting this patch fixes the issue. Any ideas?
>
> All I can think of is something calling led_set_brightness quite often,
> the patch in question makes led_set_brightness somewhat more expensive,
> but it should not cause such a big difference unless something is
> really calling led_set_brightness quite often maybe something is calling
> it with the same value all the time ?
I don't think this one has any brightness control.
Regards,
Tony
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web