* PM regression with LED changes in next-20161109 @ 2016-11-09 19:23 Tony Lindgren 2016-11-09 20:45 ` Jacek Anaszewski 2016-11-10 8:34 ` Hans de Goede 0 siblings, 2 replies; 50+ messages in thread From: Tony Lindgren @ 2016-11-09 19:23 UTC (permalink / raw) To: Hans de Goede, Jacek Anaszewski Cc: linux-leds, linux-omap, linux-arm-kernel, linux-kernel 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? Regards, Tony ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-09 19:23 PM regression with LED changes in next-20161109 Tony Lindgren @ 2016-11-09 20:45 ` Jacek Anaszewski 2016-11-10 8:49 ` Hans de Goede 2016-11-10 8:34 ` Hans de Goede 1 sibling, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-09 20:45 UTC (permalink / raw) To: Tony Lindgren, Hans de Goede Cc: Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi Tony, On 11/09/2016 08:23 PM, 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. > > Reverting this patch fixes the issue. Any ideas? Thanks for the report. This is probably caused by sysfs_notify_dirent(). I'm afraid that we can't keep this feature in the current shape. Hans, I'm dropping the patch. We probably will have to delegate this call to a workqueue task. Think about use cases when the LED is blinked with high frequency e.g. from ledtrig-disk.c. Also, IMHO the notifications should be enabled only if explicitly selected in the kernel config. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-09 20:45 ` Jacek Anaszewski @ 2016-11-10 8:49 ` Hans de Goede 2016-11-10 12:56 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-10 8:49 UTC (permalink / raw) To: Jacek Anaszewski, Tony Lindgren Cc: Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi, On 09-11-16 21:45, Jacek Anaszewski wrote: > Hi Tony, > > On 11/09/2016 08:23 PM, 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. >> >> Reverting this patch fixes the issue. Any ideas? > > Thanks for the report. This is probably caused by sysfs_notify_dirent(). > I'm afraid that we can't keep this feature in the current shape. > Hans, I'm dropping the patch. We probably will have to delegate this > call to a workqueue task. Think about use cases when the LED is blinked > with high frequency e.g. from ledtrig-disk.c. sysfs_notify_dirent() already uses a workqueue, here is the actual implementation of it (from fs/kernfs/file.c) : void kernfs_notify(struct kernfs_node *kn) { static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn); unsigned long flags; if (WARN_ON(kernfs_type(kn) != KERNFS_FILE)) return; spin_lock_irqsave(&kernfs_notify_lock, flags); if (!kn->attr.notify_next) { kernfs_get(kn); kn->attr.notify_next = kernfs_notify_list; kernfs_notify_list = kn; schedule_work(&kernfs_notify_work); } spin_unlock_irqrestore(&kernfs_notify_lock, flags); } So using a workqueue is not going to help. Note that I already feared this, which is why my initial implementation only called sysfs_notify_dirent() for user initiated changes and not for triggers / blinking. I think we may need to reconsider what getting the brightness sysfs atrribute actually returns. Currently when a LED is blinking it will return 0 resp. the actual brightness depending on when in the blink cycle the user reads the brightness sysfs atrribute. So a user can do "echo 128 > brightness && cat brightness" and get out 0, or 128, depending purely on timing. This seems to contradict what Documentation/ABI/testing/sysfs-class-led has to say: What: /sys/class/leds/<led>/brightness Date: March 2006 KernelVersion: 2.6.17 Contact: Richard Purdie <rpurdie@rpsys.net> Description: Set the brightness of the LED. Most LEDs don't have hardware brightness support, so will just be turned on for non-zero brightness settings. The value is between 0 and /sys/class/leds/<led>/max_brightness. Writing 0 to this file clears active trigger. Writing non-zero to this file while trigger is active changes the top brightness trigger is going to use. Even though it only talks about writing, the logical thing would be for reading to be the exact opposite of writing, so we would get: Reading from this file while a trigger is active returns the top brightness trigger is going to use. The current docs say not about (sw) blinking, but that should be treated just like a trigger IMHO. If we can get consensus on what the read behavior for the brightness attribute should be, then I think that a better poll() behavior will automatically follow from that. Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 8:49 ` Hans de Goede @ 2016-11-10 12:56 ` Jacek Anaszewski 2016-11-10 13:04 ` Hans de Goede 2016-11-10 16:29 ` Pavel Machek 0 siblings, 2 replies; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-10 12:56 UTC (permalink / raw) To: Hans de Goede, Jacek Anaszewski, Tony Lindgren Cc: linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart, Pavel Machek Hi, On 11/10/2016 09:49 AM, Hans de Goede wrote: > Hi, > > On 09-11-16 21:45, Jacek Anaszewski wrote: >> Hi Tony, >> >> On 11/09/2016 08:23 PM, 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. >>> >>> Reverting this patch fixes the issue. Any ideas? >> >> Thanks for the report. This is probably caused by sysfs_notify_dirent(). >> I'm afraid that we can't keep this feature in the current shape. >> Hans, I'm dropping the patch. We probably will have to delegate this >> call to a workqueue task. Think about use cases when the LED is blinked >> with high frequency e.g. from ledtrig-disk.c. > > sysfs_notify_dirent() already uses a workqueue, here is the actual > implementation of it (from fs/kernfs/file.c) : > > void kernfs_notify(struct kernfs_node *kn) > { > static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn); > unsigned long flags; > > if (WARN_ON(kernfs_type(kn) != KERNFS_FILE)) > return; > > spin_lock_irqsave(&kernfs_notify_lock, flags); > if (!kn->attr.notify_next) { > kernfs_get(kn); > kn->attr.notify_next = kernfs_notify_list; > kernfs_notify_list = kn; > schedule_work(&kernfs_notify_work); > } > spin_unlock_irqrestore(&kernfs_notify_lock, flags); > } Indeed. As a next step of this investigation Tony could disable particular calls made in kernfs_notify_workfn to check what exactly causes excessive power consumption. > So using a workqueue is not going to help. Note that I already > feared this, which is why my initial implementation only called > sysfs_notify_dirent() for user initiated changes and not for > triggers / blinking. AFAIR there were no calls to led_notify_brightness_change() in the initial implementation and it was entirely predestined for being called by LED class drivers on brightness changes made by firmware. > I think we may need to reconsider what getting the brightness > sysfs atrribute actually returns. Currently when a LED is > blinking it will return 0 resp. the actual brightness depending > on when in the blink cycle the user reads the brightness > sysfs atrribute. > > So a user can do "echo 128 > brightness && cat brightness" and > get out 0, or 128, depending purely on timing. > > This seems to contradict what Documentation/ABI/testing/sysfs-class-led > has to say: > > What: /sys/class/leds/<led>/brightness > Date: March 2006 > KernelVersion: 2.6.17 > Contact: Richard Purdie <rpurdie@rpsys.net> > Description: > Set the brightness of the LED. Most LEDs don't > have hardware brightness support, so will just be turned > on for > non-zero brightness settings. The value is between 0 and > /sys/class/leds/<led>/max_brightness. > > Writing 0 to this file clears active trigger. > > Writing non-zero to this file while trigger is active > changes the > top brightness trigger is going to use. > > Even though it only talks about writing, the logical thing would be for > reading to be the exact opposite of writing, so we would get: > > Reading from this file while a trigger is active returns > the > top brightness trigger is going to use. > > The current docs say not about (sw) blinking, but that should be treated > just > like a trigger IMHO. You'r right, we should describe the semantics on reading, but it would have to be as follows: Reading from this file returns LED brightness at given moment, i.e. even though LED class device brightness setting is greater than 0, the momentary brightness can be 0 if the readout occurred during low phase of blink cycle. > If we can get consensus on what the read behavior for the brightness > attribute > should be, then I think that a better poll() behavior will automatically > follow > from that. It seems that we should get back to your initial approach. i.e. only brightness changes caused by hardware should be reported. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 12:56 ` Jacek Anaszewski @ 2016-11-10 13:04 ` Hans de Goede 2016-11-10 13:55 ` Jacek Anaszewski 2016-11-10 16:29 ` Pavel Machek 1 sibling, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-10 13:04 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren Cc: linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart, Pavel Machek Hi, On 10-11-16 13:56, Jacek Anaszewski wrote: > Hi, > > On 11/10/2016 09:49 AM, Hans de Goede wrote: >> Hi, >> >> On 09-11-16 21:45, Jacek Anaszewski wrote: >>> Hi Tony, >>> >>> On 11/09/2016 08:23 PM, 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. >>>> >>>> Reverting this patch fixes the issue. Any ideas? >>> >>> Thanks for the report. This is probably caused by sysfs_notify_dirent(). >>> I'm afraid that we can't keep this feature in the current shape. >>> Hans, I'm dropping the patch. We probably will have to delegate this >>> call to a workqueue task. Think about use cases when the LED is blinked >>> with high frequency e.g. from ledtrig-disk.c. >> >> sysfs_notify_dirent() already uses a workqueue, here is the actual >> implementation of it (from fs/kernfs/file.c) : >> >> void kernfs_notify(struct kernfs_node *kn) >> { >> static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn); >> unsigned long flags; >> >> if (WARN_ON(kernfs_type(kn) != KERNFS_FILE)) >> return; >> >> spin_lock_irqsave(&kernfs_notify_lock, flags); >> if (!kn->attr.notify_next) { >> kernfs_get(kn); >> kn->attr.notify_next = kernfs_notify_list; >> kernfs_notify_list = kn; >> schedule_work(&kernfs_notify_work); >> } >> spin_unlock_irqrestore(&kernfs_notify_lock, flags); >> } > > Indeed. As a next step of this investigation Tony could disable > particular calls made in kernfs_notify_workfn to check what > exactly causes excessive power consumption. > >> So using a workqueue is not going to help. Note that I already >> feared this, which is why my initial implementation only called >> sysfs_notify_dirent() for user initiated changes and not for >> triggers / blinking. > > AFAIR there were no calls to led_notify_brightness_change() in > the initial implementation and it was entirely predestined for > being called by LED class drivers on brightness changes made > by firmware. > >> I think we may need to reconsider what getting the brightness >> sysfs atrribute actually returns. Currently when a LED is >> blinking it will return 0 resp. the actual brightness depending >> on when in the blink cycle the user reads the brightness >> sysfs atrribute. >> >> So a user can do "echo 128 > brightness && cat brightness" and >> get out 0, or 128, depending purely on timing. >> >> This seems to contradict what Documentation/ABI/testing/sysfs-class-led >> has to say: >> >> What: /sys/class/leds/<led>/brightness >> Date: March 2006 >> KernelVersion: 2.6.17 >> Contact: Richard Purdie <rpurdie@rpsys.net> >> Description: >> Set the brightness of the LED. Most LEDs don't >> have hardware brightness support, so will just be turned >> on for >> non-zero brightness settings. The value is between 0 and >> /sys/class/leds/<led>/max_brightness. >> >> Writing 0 to this file clears active trigger. >> >> Writing non-zero to this file while trigger is active >> changes the >> top brightness trigger is going to use. >> >> Even though it only talks about writing, the logical thing would be for >> reading to be the exact opposite of writing, so we would get: >> >> Reading from this file while a trigger is active returns >> the >> top brightness trigger is going to use. >> >> The current docs say not about (sw) blinking, but that should be treated >> just >> like a trigger IMHO. > > You'r right, we should describe the semantics on reading, but it would > have to be as follows: > > Reading from this file returns LED brightness at given moment, i.e. > even though LED class device brightness setting is greater than 0, the > momentary brightness can be 0 if the readout occurred during low phase > of blink cycle. Why would it need to read like this, because this is the current behavior ? I doubt anyone is relying on this current behavior because it is really unpredictable which value one can get. I believe it would be better to change the read semantics to follow the write semantics, this would be much more consistent. Making the read behavior match the write behavior should be easy I would be happy to write a patch for this. >> If we can get consensus on what the read behavior for the brightness >> attribute >> should be, then I think that a better poll() behavior will automatically >> follow >> from that. > > It seems that we should get back to your initial approach. i.e. only > brightness changes caused by hardware should be reported. Ok, if you really want to keep the read behavior as is, I can provide an updated patch for this. Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 13:04 ` Hans de Goede @ 2016-11-10 13:55 ` Jacek Anaszewski 2016-11-10 16:36 ` Pavel Machek 0 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-10 13:55 UTC (permalink / raw) To: Hans de Goede, Jacek Anaszewski, Tony Lindgren Cc: linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart, Pavel Machek Hi, On 11/10/2016 02:04 PM, Hans de Goede wrote: > Hi, > > On 10-11-16 13:56, Jacek Anaszewski wrote: >> Hi, >> >> On 11/10/2016 09:49 AM, Hans de Goede wrote: >>> Hi, >>> >>> On 09-11-16 21:45, Jacek Anaszewski wrote: >>>> Hi Tony, >>>> >>>> On 11/09/2016 08:23 PM, 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. >>>>> >>>>> Reverting this patch fixes the issue. Any ideas? >>>> >>>> Thanks for the report. This is probably caused by >>>> sysfs_notify_dirent(). >>>> I'm afraid that we can't keep this feature in the current shape. >>>> Hans, I'm dropping the patch. We probably will have to delegate this >>>> call to a workqueue task. Think about use cases when the LED is blinked >>>> with high frequency e.g. from ledtrig-disk.c. >>> >>> sysfs_notify_dirent() already uses a workqueue, here is the actual >>> implementation of it (from fs/kernfs/file.c) : >>> >>> void kernfs_notify(struct kernfs_node *kn) >>> { >>> static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn); >>> unsigned long flags; >>> >>> if (WARN_ON(kernfs_type(kn) != KERNFS_FILE)) >>> return; >>> >>> spin_lock_irqsave(&kernfs_notify_lock, flags); >>> if (!kn->attr.notify_next) { >>> kernfs_get(kn); >>> kn->attr.notify_next = kernfs_notify_list; >>> kernfs_notify_list = kn; >>> schedule_work(&kernfs_notify_work); >>> } >>> spin_unlock_irqrestore(&kernfs_notify_lock, flags); >>> } >> >> Indeed. As a next step of this investigation Tony could disable >> particular calls made in kernfs_notify_workfn to check what >> exactly causes excessive power consumption. >> >>> So using a workqueue is not going to help. Note that I already >>> feared this, which is why my initial implementation only called >>> sysfs_notify_dirent() for user initiated changes and not for >>> triggers / blinking. >> >> AFAIR there were no calls to led_notify_brightness_change() in >> the initial implementation and it was entirely predestined for >> being called by LED class drivers on brightness changes made >> by firmware. >> >>> I think we may need to reconsider what getting the brightness >>> sysfs atrribute actually returns. Currently when a LED is >>> blinking it will return 0 resp. the actual brightness depending >>> on when in the blink cycle the user reads the brightness >>> sysfs atrribute. >>> >>> So a user can do "echo 128 > brightness && cat brightness" and >>> get out 0, or 128, depending purely on timing. >>> >>> This seems to contradict what Documentation/ABI/testing/sysfs-class-led >>> has to say: >>> >>> What: /sys/class/leds/<led>/brightness >>> Date: March 2006 >>> KernelVersion: 2.6.17 >>> Contact: Richard Purdie <rpurdie@rpsys.net> >>> Description: >>> Set the brightness of the LED. Most LEDs don't >>> have hardware brightness support, so will just be turned >>> on for >>> non-zero brightness settings. The value is between 0 and >>> /sys/class/leds/<led>/max_brightness. >>> >>> Writing 0 to this file clears active trigger. >>> >>> Writing non-zero to this file while trigger is active >>> changes the >>> top brightness trigger is going to use. >>> >>> Even though it only talks about writing, the logical thing would be for >>> reading to be the exact opposite of writing, so we would get: >>> >>> Reading from this file while a trigger is active returns >>> the >>> top brightness trigger is going to use. >>> >>> The current docs say not about (sw) blinking, but that should be treated >>> just >>> like a trigger IMHO. >> >> You'r right, we should describe the semantics on reading, but it would >> have to be as follows: >> >> Reading from this file returns LED brightness at given moment, i.e. >> even though LED class device brightness setting is greater than 0, the >> momentary brightness can be 0 if the readout occurred during low phase >> of blink cycle. > > Why would it need to read like this, because this is the current behavior ? We have led_update_brightness() which was introduced long time ago and is used in brightness_show(). Note that if LED controller changed actual LED brightness e.g. due to battery voltage dropping below certain threshold, we wouldn't be able to find it out otherwise (except if we added separate sysfs file for that). > > I doubt anyone is relying on this current behavior because it is really > unpredictable which value one can get. > > I believe it would be better to change the read semantics to follow > the write semantics, this would be much more consistent. > > Making the read behavior match the write behavior should be easy I would > be happy to write a patch for this. Let's better agree on the description of the current semantics. It has been around for a long time. >>> If we can get consensus on what the read behavior for the brightness >>> attribute >>> should be, then I think that a better poll() behavior will automatically >>> follow >>> from that. >> >> It seems that we should get back to your initial approach. i.e. only >> brightness changes caused by hardware should be reported. > > Ok, if you really want to keep the read behavior as is, I can provide > an updated patch for this. Yes please. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 13:55 ` Jacek Anaszewski @ 2016-11-10 16:36 ` Pavel Machek 0 siblings, 0 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-10 16:36 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hans de Goede, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1361 bytes --] Hi! > >>>The current docs say not about (sw) blinking, but that should be treated > >>>just > >>>like a trigger IMHO. > >> > >>You'r right, we should describe the semantics on reading, but it would > >>have to be as follows: > >> > >>Reading from this file returns LED brightness at given moment, i.e. > >>even though LED class device brightness setting is greater than 0, the > >>momentary brightness can be 0 if the readout occurred during low phase > >>of blink cycle. > > > >Why would it need to read like this, because this is the current behavior ? > > We have led_update_brightness() which was introduced long time ago and > is used in brightness_show(). Note that if LED controller changed > actual LED brightness e.g. due to battery voltage dropping below > certain threshold, we wouldn't be able to find it out otherwise > (except if we added separate sysfs file for that). And we should have a separate sysfs file for that. Note that on some hardware leds, you are able to let hardware control them, but if you do, you can't really tell the current state. Examples are n900 and thinkpad-acpi. So it is better we don't pretend we can get that value for userspace. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 12:56 ` Jacek Anaszewski 2016-11-10 13:04 ` Hans de Goede @ 2016-11-10 16:29 ` Pavel Machek 2016-11-10 16:44 ` Hans de Goede 2016-11-10 17:55 ` Tony Lindgren 1 sibling, 2 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-10 16:29 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hans de Goede, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1374 bytes --] 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? > >So a user can do "echo 128 > brightness && cat brightness" and > >get out 0, or 128, depending purely on timing. ... > > Reading from this file while a trigger is active returns > >the > > top brightness trigger is going to use. Yes, that sounds sane. > It seems that we should get back to your initial approach. i.e. only > brightness changes caused by hardware should be reported. I don't think enabling poll() here is good idea. Some hardware won't be able to tell you that it changed the state. Returning maximum brightness trigger is going to use seems easier/better. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 16:29 ` Pavel Machek @ 2016-11-10 16:44 ` Hans de Goede 2016-11-10 20:48 ` Pavel Machek 2016-11-10 17:55 ` Tony Lindgren 1 sibling, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-10 16:44 UTC (permalink / raw) To: Pavel Machek, Jacek Anaszewski Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi, On 10-11-16 17:29, Pavel Machek 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. >>>>> >>>>> 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? > >>> So a user can do "echo 128 > brightness && cat brightness" and >>> get out 0, or 128, depending purely on timing. > ... >>> Reading from this file while a trigger is active returns >>> the >>> top brightness trigger is going to use. > > Yes, that sounds sane. > >> It seems that we should get back to your initial approach. i.e. only >> brightness changes caused by hardware should be reported. > > I don't think enabling poll() here is good idea. Some hardware won't > be able to tell you that it changed the state. Returning maximum > brightness trigger is going to use seems easier/better. The idea here is to allow userspace to poll() on the brightness sysfs atrribute to detect changes autonomously done by the hardware, such as e.g. happens on both Dell and Thinkpad laptops when pressing the keyboard backlight cycle hotkey. Note that these keys do not generate key-press events, the cycling through the brightness levels (including off) is done entirely in firmware. But we do get other ACPI events for this which we can use to let userspace know this happens, which is something which user- interfaces which allow control over the kbd backlight want to know. I understand that we will not always be able to do this, here is the Documentation/ABI/testing/sysfs-class-led text I have in mind: The file supports poll() to detect changes, changes are only signalled when this file is written or when the hardware / firmware changes the brightness itself and the driver can detect this. Changes done by kernel triggers / software blinking are not signalled. Note the "and the driver can detect this" language, that has been there since v1 of the poll() notification patch since I already expected not all hardware to be able to signal this. Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 16:44 ` Hans de Goede @ 2016-11-10 20:48 ` Pavel Machek 2016-11-11 8:25 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-10 20:48 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 2557 bytes --] Hi! > >>It seems that we should get back to your initial approach. i.e. only > >>brightness changes caused by hardware should be reported. > > > >I don't think enabling poll() here is good idea. Some hardware won't > >be able to tell you that it changed the state. Returning maximum > >brightness trigger is going to use seems easier/better. > > The idea here is to allow userspace to poll() on the brightness > sysfs atrribute to detect changes autonomously done by the hardware, > such as e.g. happens on both Dell and Thinkpad laptops when pressing > the keyboard backlight cycle hotkey. Note that these keys do not > generate key-press events, the cycling through the brightness levels > (including off) is done entirely in firmware. Ok, so you can do that for keyboard backlight on thinkpad... I guess you handle that as a special trigger on the keyboard leds? Can other triggers, such as heartbeat, be assigned to that "led"? > But we do get other ACPI events for this which we can use to let > userspace know this happens, which is something which user- > interfaces which allow control over the kbd backlight want to know. Yes, you can do that for keyboard backlight... but on thinkpads there are more leds, such as battery led. That can blink on battery low, and I don't think you can read the current status from hardware. Getting current state of led blinking with cpu trigger is also not quite a good idea. So IMO this should not be done in generic code. Instead, kbd-backlight trigger should have special attribute, and that one should be pollable. > I understand that we will not always be able to do this, here is the > Documentation/ABI/testing/sysfs-class-led text I have in mind: > > The file supports poll() to detect changes, changes are only > signalled when this file is written or when the hardware / > firmware changes the brightness itself and the driver can detect > this. Changes done by kernel triggers / software blinking are > not signalled. > > Note the "and the driver can detect this" language, that has been there > since v1 of the poll() notification patch since I already expected not > all hardware to be able to signal this. Lets move it to separate attribute, for triggers that can do that, please. We do want a way to read maximum brightness for the heartbeat trigger, for example.. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 20:48 ` Pavel Machek @ 2016-11-11 8:25 ` Hans de Goede 0 siblings, 0 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-11 8:25 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi, On 10-11-16 21:48, Pavel Machek wrote: > Hi! > >>>> It seems that we should get back to your initial approach. i.e. only >>>> brightness changes caused by hardware should be reported. >>> >>> I don't think enabling poll() here is good idea. Some hardware won't >>> be able to tell you that it changed the state. Returning maximum >>> brightness trigger is going to use seems easier/better. >> >> The idea here is to allow userspace to poll() on the brightness >> sysfs atrribute to detect changes autonomously done by the hardware, >> such as e.g. happens on both Dell and Thinkpad laptops when pressing >> the keyboard backlight cycle hotkey. Note that these keys do not >> generate key-press events, the cycling through the brightness levels >> (including off) is done entirely in firmware. > > Ok, so you can do that for keyboard backlight on thinkpad... I guess > you handle that as a special trigger on the keyboard leds? No, as said this is all done in firmware, as in this is all dealt with by (presumably) the acpi-ec (acpi-embedded-controller) the kernel does not do anything here, the key is "hardwired" to control the keyboard backlight from the kernels pov. > Can other > triggers, such as heartbeat, be assigned to that "led"? > >> But we do get other ACPI events for this which we can use to let >> userspace know this happens, which is something which user- >> interfaces which allow control over the kbd backlight want to know. > > Yes, you can do that for keyboard backlight... but on thinkpads there > are more leds, such as battery led. That can blink on battery low, and > I don't think you can read the current status from hardware. Well the battery LED does not show up under /sys/class/led so that is not relevant for this situation, anyways ... > Getting current state of led blinking with cpu trigger is also not > quite a good idea. I agree with you that it would be better if reading the brightness sysfs attribute would always return the max brightness for LEDs which are blinking or have a trigger set. But it seems that Jacek disagrees, I will leave further discussion of this up to you and Jacek. > So IMO this should not be done in generic code. Instead, > kbd-backlight trigger should have special attribute, and that one > should be pollable. Again there is no kbd-backlight trigger. >> I understand that we will not always be able to do this, here is the >> Documentation/ABI/testing/sysfs-class-led text I have in mind: >> >> The file supports poll() to detect changes, changes are only >> signalled when this file is written or when the hardware / >> firmware changes the brightness itself and the driver can detect >> this. Changes done by kernel triggers / software blinking are >> not signalled. >> >> Note the "and the driver can detect this" language, that has been there >> since v1 of the poll() notification patch since I already expected not >> all hardware to be able to signal this. > > Lets move it to separate attribute, for triggers that can do that, > please. As explained above this has nothing to do with triggers... Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 16:29 ` Pavel Machek 2016-11-10 16:44 ` Hans de Goede @ 2016-11-10 17:55 ` Tony Lindgren 2016-11-10 20:29 ` Pavel Machek 1 sibling, 1 reply; 50+ messages in thread From: Tony Lindgren @ 2016-11-10 17:55 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Hans de Goede, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart * 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. Tony ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 17:55 ` Tony Lindgren @ 2016-11-10 20:29 ` Pavel Machek 2016-11-10 21:34 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-10 20:29 UTC (permalink / raw) To: Tony Lindgren Cc: Jacek Anaszewski, Hans de Goede, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1222 bytes --] 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. Thanks and best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 20:29 ` Pavel Machek @ 2016-11-10 21:34 ` Jacek Anaszewski 2016-11-11 12:01 ` Pavel Machek 0 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-10 21:34 UTC (permalink / raw) To: Pavel Machek, Tony Lindgren Cc: Jacek Anaszewski, Hans de Goede, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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. Behaviour for drivers that do not or cannot read the led status is unchanged. Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> Signed-off-by: Richard Purdie <rpurdie@rpsys.net> -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 21:34 ` Jacek Anaszewski @ 2016-11-11 12:01 ` Pavel Machek 2016-11-11 17:03 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-11 12:01 UTC (permalink / raw) To: Jacek Anaszewski Cc: Tony Lindgren, Jacek Anaszewski, Hans de Goede, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 2619 bytes --] 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. But I'd certainly not extend it with poll. IMO reading/polling should only be available with some triggers. It does not make sense with "CPU load" trigger. It makes sense with "keyboard light changeable by hardware" trigger. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 12:01 ` Pavel Machek @ 2016-11-11 17:03 ` Jacek Anaszewski 2016-11-11 19:28 ` Hans de Goede 2016-11-11 22:06 ` Pavel Machek 0 siblings, 2 replies; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-11 17:03 UTC (permalink / raw) To: Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, Hans de Goede, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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). This way it would be semantically consistent to report only hardware originating brightness changes on it, which was the initial reason for adding the brightness change notification feature. Moreover, LED class drivers could report on this file the brightness level which was set by the firmware, which wouldn't affect current LED class device brightness setting, unless brightness file is read (and brightness_get op is supported). > IMO reading/polling should only be available with some triggers. It > does not make sense with "CPU load" trigger. It makes sense with > "keyboard light changeable by hardware" trigger. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 17:03 ` Jacek Anaszewski @ 2016-11-11 19:28 ` Hans de Goede 2016-11-11 22:12 ` Pavel Machek 2016-11-12 10:24 ` PM regression with LED changes in next-20161109 Jacek Anaszewski 2016-11-11 22:06 ` Pavel Machek 1 sibling, 2 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-11 19:28 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ? Thinking more about this, I'm strongly against having to do poll on /sys/.../bar to detect changes on /sys/.../foo that is something which no other interface does. So my vote on this is NACK for the having a separate file for this. Regards, Hans > > This way it would be semantically consistent to report only > hardware originating brightness changes on it, which was the > initial reason for adding the brightness change notification > feature. > > Moreover, LED class drivers could report on this file the > brightness level which was set by the firmware, which wouldn't > affect current LED class device brightness setting, unless > brightness file is read (and brightness_get op is supported). > >> IMO reading/polling should only be available with some triggers. It >> does not make sense with "CPU load" trigger. It makes sense with >> "keyboard light changeable by hardware" trigger. > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 19:28 ` Hans de Goede @ 2016-11-11 22:12 ` Pavel Machek 2016-11-12 8:03 ` Hans de Goede 2016-11-12 10:24 ` PM regression with LED changes in next-20161109 Jacek Anaszewski 1 sibling, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-11 22:12 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1194 bytes --] Hi! Reason #1: > >>Hmm. So userland can read the LED state, and it can get _some_ value > >>back, but it can not know if it is current state or not. > Why a dedicated file? Are we going to mirror brightness here > wrt r/w (show/store) behavior ? If not userspace now needs > 2 open fds which is not really nice. If we are and we are > not going to use poll for something else on brightness itself > then why not just poll directly on brightness ? Reason #1 is above. Reason #2 is "if userspace sees brightness file, it can not know if the notifications on change actually work or not". Reason #3 is that you broke Tony's system. Polling does not make sense when trigger such as "CPU in use" is active. Reason #4 is that there are really two brightnesses: 1) maximum brightness trigger is going to use 2) current brightness Currently writing to "brightness" file changes 1), but reading returns 2) when available. So, feel free to propose better interface. One that solves #1..#4 above. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 22:12 ` Pavel Machek @ 2016-11-12 8:03 ` Hans de Goede 2016-11-13 9:10 ` Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Pavel Machek 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-12 8:03 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi, On 11-11-16 23:12, Pavel Machek wrote: > Hi! > > Reason #1: > >>>> Hmm. So userland can read the LED state, and it can get _some_ value >>>> back, but it can not know if it is current state or not. That is not correct, the current behavior for eading the brightness atrribute is to always return the current state. >> Why a dedicated file? Are we going to mirror brightness here >> wrt r/w (show/store) behavior ? If not userspace now needs >> 2 open fds which is not really nice. If we are and we are >> not going to use poll for something else on brightness itself >> then why not just poll directly on brightness ? > > Reason #1 is above. See my reply above. > Reason #2 is "if userspace sees brightness file, it can not know if > the notifications on change actually work or not". If it needs to know that it can simply check the kernel version. > Reason #3 is that you broke Tony's system. Polling does not make sense > when trigger such as "CPU in use" is active. Have you seen v4 of my patch? It fixes this while keeping the polling on the brightness attribute itself, it basically goes back (more or less) to v1 of my patch which did not have this problem. I never wanted notification of trigger / blinking changes because I already feared Tony's problem would happen. > Reason #4 is that there are really two brightnesses: > > 1) maximum brightness trigger is going to use > > 2) current brightness > > Currently writing to "brightness" file changes 1), but reading returns > 2) when available. Right and Jacek has already said that we cannot change the reading behavior on the brightness file because of ABI concerns. So if anything we need a new blink_brightness file or such, which when read shows the maximum brightness when blinking or triggers are active. Note that we already have a max_brightness file which is the actual maximum brightness the led supports. Since the existing ABI behavior is for the existing brightness file to return the *current* brightness, please explain to me how polling on say the new blink_brightness file would make sense to detect changes in the current brightness ? > So, feel free to propose better interface. One that solves #1..#4 > above. Proposal 1: v4 of my patch, see the list. It solves all but #4, which is out of scope for my patch, feel free to submit a patch to solve #4 (with a new sysfs attr). Proposal 2: Add a new "user_brightness" file, which shows the last brightness as set by the user, this would show the read behavior we really want of brightness: show the real brightness when not blinking / triggers are active, show the brightness used when on when blinking / triggers are active. And then we could add poll support on this new user_brightness file, thus avoiding the problem with the extra cpu-load on notifications on blinking / triggers. Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) 2016-11-12 8:03 ` Hans de Goede @ 2016-11-13 9:10 ` Pavel Machek 2016-11-13 9:44 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-13 9:10 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 4433 bytes --] On Sat 2016-11-12 09:03:42, Hans de Goede wrote: > Hi, > > On 11-11-16 23:12, Pavel Machek wrote: > >Hi! > > > >Reason #1: > > > >>>>Hmm. So userland can read the LED state, and it can get _some_ value > >>>>back, but it can not know if it is current state or not. > > That is not correct, the current behavior for eading the brightness > atrribute is to always return the current state. No. (Because some hardware can't get back current state of hardware-controlled leds, and because of blinking). > >>Why a dedicated file? Are we going to mirror brightness here > >>wrt r/w (show/store) behavior ? If not userspace now needs > >>2 open fds which is not really nice. If we are and we are > >>not going to use poll for something else on brightness itself > >>then why not just poll directly on brightness ? > > > >Reason #1 is above. > > See my reply above. > > >Reason #2 is "if userspace sees brightness file, it can not know if > >the notifications on change actually work or not". > > If it needs to know that it can simply check the kernel version. No. Because in case of hardware blinking we can't provide poll() functionality. Plus, saying "simply check the kernel version" simply means you should not be submitting patches to kernel... at all. (Hint... it also does not work.) > >Reason #3 is that you broke Tony's system. Polling does not make sense > >when trigger such as "CPU in use" is active. > > Have you seen v4 of my patch? It fixes this while keeping the > polling on the brightness attribute itself, it basically goes > back (more or less) to v1 of my patch which did not have this > problem. I never wanted notification of trigger / blinking > changes because I already feared Tony's problem would happen. Have you seen v67123 of my latest facebook post? It explains why you are completely wrong. > >Reason #4 is that there are really two brightnesses: > > > >1) maximum brightness trigger is going to use > > > >2) current brightness > > > >Currently writing to "brightness" file changes 1), but reading returns > >2) when available. > > Right and Jacek has already said that we cannot change the > reading behavior on the brightness file because of ABI concerns. Until there's user that actually reads that, ABI can be fixed. Given that it basically returns random value, > >So, feel free to propose better interface. One that solves #1..#4 > >above. > > Proposal 1: > > v4 of my patch, see the list. It solves all but #4, which > is out of scope for my patch, feel free to submit a patch to > solve #4 (with a new sysfs attr). NAK on that. (And it does not solve #1 and #2 at least.) > Proposal 2: > > Add a new "user_brightness" file, which shows the last brightness > as set by the user, this would show the read behavior we really > want of brightness: show the real brightness when not blinking / > triggers are active, show the brightness used when on when > blinking / triggers are active. No, that's just adding more mess on the system. Here's better proposal: brightness (write): what we do today. (Mess, but too late to change it) (read): -Esomething or what we do today (if someone acutally uses it) (poll): -Esomething current_brightness (write): -Esomething, or maybe change brightness for triggers that can work with that (read, poll): if the current trigger can get current state of led, do it, otherwise -Esomething... or maybe file should be simply hidden from sysfs. trigger_max_brightness (read,write): change the maximum brightness for a trigger. (poll): -Esomething If you have hardware changing the brightness behind kernel's back, that should be modelled as a trigger. Userspace should know there's hardware changing it autonomously ... there should be "hardware-keylight-brightness" trigger, probably impossible to change (depends on hardware behaviour). On thinkpad, for example, for many LEDs kernel can select either "hardware drives the LED", but then current_brightness is unavailable, or "kernel drives the LED", but then hardware does not touch the led at all. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) 2016-11-13 9:10 ` Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Pavel Machek @ 2016-11-13 9:44 ` Hans de Goede 2016-11-13 20:45 ` Pavel Machek 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-13 9:44 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart Hi, On 13-11-16 10:10, Pavel Machek wrote: > On Sat 2016-11-12 09:03:42, Hans de Goede wrote: >> Hi, >> >> On 11-11-16 23:12, Pavel Machek wrote: >>> Hi! >>> >>> Reason #1: >>> >>>>>> Hmm. So userland can read the LED state, and it can get _some_ value >>>>>> back, but it can not know if it is current state or not. >> >> That is not correct, the current behavior for eading the brightness >> atrribute is to always return the current state. > > No. (Because some hardware can't get back current state of > hardware-controlled leds, and because of blinking). > >>>> Why a dedicated file? Are we going to mirror brightness here >>>> wrt r/w (show/store) behavior ? If not userspace now needs >>>> 2 open fds which is not really nice. If we are and we are >>>> not going to use poll for something else on brightness itself >>>> then why not just poll directly on brightness ? >>> >>> Reason #1 is above. >> >> See my reply above. >> >>> Reason #2 is "if userspace sees brightness file, it can not know if >>> the notifications on change actually work or not". >> >> If it needs to know that it can simply check the kernel version. > > No. Because in case of hardware blinking we can't provide poll() > functionality. We have already decided that we do not want to wakeup poll() on blinking because it causes to many wakeups, and there is no use-case for waking poll for blinking (or for triggers). > Plus, saying "simply check the kernel version" simply means you should > not be submitting patches to kernel... at all. (Hint... it also does > not work.) Lets keep things civil please. >>> Reason #3 is that you broke Tony's system. Polling does not make sense >>> when trigger such as "CPU in use" is active. >> >> Have you seen v4 of my patch? It fixes this while keeping the >> polling on the brightness attribute itself, it basically goes >> back (more or less) to v1 of my patch which did not have this >> problem. I never wanted notification of trigger / blinking >> changes because I already feared Tony's problem would happen. > > Have you seen v67123 of my latest facebook post? It explains why you > are completely wrong. Lets keep things civil please. >>> Reason #4 is that there are really two brightnesses: >>> >>> 1) maximum brightness trigger is going to use >>> >>> 2) current brightness >>> >>> Currently writing to "brightness" file changes 1), but reading returns >>> 2) when available. >> >> Right and Jacek has already said that we cannot change the >> reading behavior on the brightness file because of ABI concerns. > > Until there's user that actually reads that, ABI can be fixed. Given > that it basically returns random value, Jacek has pretty much nacked fixing this because of ABI concerns, so I see no use in further discussing changing the read behavior of the existing brightness sysfs attribute. >>> So, feel free to propose better interface. One that solves #1..#4 >>> above. >> >> Proposal 1: >> >> v4 of my patch, see the list. It solves all but #4, which >> is out of scope for my patch, feel free to submit a patch to >> solve #4 (with a new sysfs attr). > > NAK on that. (And it does not solve #1 and #2 at least.) > >> Proposal 2: >> >> Add a new "user_brightness" file, which shows the last brightness >> as set by the user, this would show the read behavior we really >> want of brightness: show the real brightness when not blinking / >> triggers are active, show the brightness used when on when >> blinking / triggers are active. > > No, that's just adding more mess on the system. > > Here's better proposal: > > brightness (write): what we do today. (Mess, but too late to change it) > (read): -Esomething or what we do today (if someone > acutally uses it) As said Jacek has already nacked any changes to read behavior. > (poll): -Esomething Making poll() on sysfs attributes return -Esomething is not possible, the internal sysfs API does not allow this. > current_brightness (write): -Esomething, or maybe change brightness > for triggers that can work with that > (read, poll): if the current trigger can get current > state of led, do it, otherwise -Esomething... > or maybe file should be simply hidden from sysfs. So write is -EINVAL and read is the same as what brightness currently does, so I see no use in introducing this new file. Also this reintroduces all the issues of v2 of my poll() patch since the CPU load problem is not actually in waking up userspace, it is in even checking if there are any userspace waiters. TLDR we simply cannot have poll behavior where we need to try and wakeup userspace on every blink / every time a trigger triggers. > trigger_max_brightness (read,write): change the maximum brightness for > a trigger. > (poll): -Esomething The write behavior here is the same as what brightness currently does and the read behavior is that of what I suggested for user_brightness, minus that you've not definied the read behavior for when no trigger / blinking is active. In itself I would be fine with this file to work around the read behavior of trigger_max_brightness, but you've not solved the polling problem. 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? > If you have hardware changing the brightness behind kernel's back, > that should be modelled as a trigger. Userspace should know > there's hardware changing it autonomously ... there should be > "hardware-keylight-brightness" trigger, probably impossible to change > (depends on hardware behaviour). > > On thinkpad, for example, for many LEDs kernel can select either > "hardware drives the LED", but then current_brightness is unavailable, > or "kernel drives the LED", but then hardware does not touch the led > at all. Whether or not the hardware changing the brightness behind kernel's should be modeled as a trigger is really outside of the scope of this discussion, as it is not related to the issues with the brightness atrribute / polling on the brightness attribute. Regards, Hans ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) 2016-11-13 9:44 ` Hans de Goede @ 2016-11-13 20:45 ` Pavel Machek 0 siblings, 0 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-13 20:45 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 2171 bytes --] Hi! > >No, that's just adding more mess on the system. > > > >Here's better proposal: > > > >brightness (write): what we do today. (Mess, but too late to change it) > > (read): -Esomething or what we do today (if someone > > acutally uses it) > > As said Jacek has already nacked any changes to read behavior. > > > (poll): -Esomething > > Making poll() on sysfs attributes return -Esomething is not possible, > the internal sysfs API does not allow this. > > >current_brightness (write): -Esomething, or maybe change brightness > > for triggers that can work with that > > (read, poll): if the current trigger can get current > > state of led, do it, otherwise -Esomething... > > or maybe file should be simply hidden from sysfs. > > So write is -EINVAL and read is the same as what brightness currently does, > so I see no use in introducing this new file. No, read is not same as current brightness file. Currently, reading brightness file returns either current brightness or maximum brightness, and userspace can't tell which is which. > >trigger_max_brightness (read,write): change the maximum brightness for > > a trigger. > > (poll): -Esomething > > The write behavior here is the same as what brightness currently does > and the read behavior is that of what I suggested for No, it is not. "brightness" behaviour is currently broken, as it sets either current brightness or maximum trigger brightness. > 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. Yes, and we want reasonable interface where userspace sees those two brightnesses. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 19:28 ` Hans de Goede 2016-11-11 22:12 ` Pavel Machek @ 2016-11-12 10:24 ` Jacek Anaszewski 2016-11-12 10:33 ` Hans de Goede 1 sibling, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-12 10:24 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-12 10:24 ` PM regression with LED changes in next-20161109 Jacek Anaszewski @ 2016-11-12 10:33 ` Hans de Goede 2016-11-12 19:14 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-12 10:33 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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). > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-12 10:33 ` Hans de Goede @ 2016-11-12 19:14 ` Jacek Anaszewski 2016-11-12 21:14 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-12 19:14 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-12 19:14 ` Jacek Anaszewski @ 2016-11-12 21:14 ` Hans de Goede 2016-11-13 11:44 ` Jacek Anaszewski 2016-11-14 8:31 ` Pavel Machek 0 siblings, 2 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-12 21:14 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-12 21:14 ` Hans de Goede @ 2016-11-13 11:44 ` Jacek Anaszewski 2016-11-13 13:52 ` Hans de Goede 2016-11-14 8:31 ` Pavel Machek 1 sibling, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-13 11:44 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-13 11:44 ` Jacek Anaszewski @ 2016-11-13 13:52 ` Hans de Goede 2016-11-14 9:12 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-13 13:52 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-13 13:52 ` Hans de Goede @ 2016-11-14 9:12 ` Jacek Anaszewski 2016-11-14 12:51 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-14 9:12 UTC (permalink / raw) To: Hans de Goede, Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-14 9:12 ` Jacek Anaszewski @ 2016-11-14 12:51 ` Hans de Goede 2016-11-15 10:01 ` Jacek Anaszewski 2016-11-15 10:31 ` LEDs that change brightness "itself" -- that's a trigger. " Pavel Machek 0 siblings, 2 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-14 12:51 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-14 12:51 ` Hans de Goede @ 2016-11-15 10:01 ` Jacek Anaszewski 2016-11-15 10:09 ` Hans de Goede 2016-11-15 10:31 ` LEDs that change brightness "itself" -- that's a trigger. " Pavel Machek 1 sibling, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-15 10:01 UTC (permalink / raw) To: Hans de Goede, Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-15 10:01 ` Jacek Anaszewski @ 2016-11-15 10:09 ` Hans de Goede 0 siblings, 0 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-15 10:09 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski, Pavel Machek Cc: Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-14 12:51 ` Hans de Goede 2016-11-15 10:01 ` Jacek Anaszewski @ 2016-11-15 10:31 ` Pavel Machek 2016-11-15 10:58 ` Jacek Anaszewski 1 sibling, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-15 10:31 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 994 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 10:31 ` LEDs that change brightness "itself" -- that's a trigger. " Pavel Machek @ 2016-11-15 10:58 ` Jacek Anaszewski 2016-11-15 11:11 ` Pavel Machek 2016-11-15 11:17 ` Hans de Goede 0 siblings, 2 replies; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-15 10:58 UTC (permalink / raw) To: Pavel Machek, Hans de Goede Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 10:58 ` Jacek Anaszewski @ 2016-11-15 11:11 ` Pavel Machek 2016-11-15 11:21 ` Hans de Goede 2016-11-15 11:17 ` Hans de Goede 1 sibling, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-15 11:11 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hans de Goede, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1708 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 11:11 ` Pavel Machek @ 2016-11-15 11:21 ` Hans de Goede 2016-11-15 11:48 ` Pavel Machek 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-15 11:21 UTC (permalink / raw) To: Pavel Machek, Jacek Anaszewski Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 11:21 ` Hans de Goede @ 2016-11-15 11:48 ` Pavel Machek 2016-11-15 12:06 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Pavel Machek @ 2016-11-15 11:48 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 2081 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 11:48 ` Pavel Machek @ 2016-11-15 12:06 ` Hans de Goede 2016-11-15 12:11 ` Pavel Machek ` (2 more replies) 0 siblings, 3 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-15 12:06 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart, Hans de Goede 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 > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 12:06 ` Hans de Goede @ 2016-11-15 12:11 ` Pavel Machek 2016-11-15 13:28 ` Jacek Anaszewski 2016-11-17 22:12 ` Hans de Goede 2 siblings, 0 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-15 12:11 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 2390 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 12:06 ` Hans de Goede 2016-11-15 12:11 ` Pavel Machek @ 2016-11-15 13:28 ` Jacek Anaszewski 2016-11-15 13:48 ` Hans de Goede 2016-11-17 22:12 ` Hans de Goede 2 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-15 13:28 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 13:28 ` Jacek Anaszewski @ 2016-11-15 13:48 ` Hans de Goede 2016-11-15 14:04 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-15 13:48 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 13:48 ` Hans de Goede @ 2016-11-15 14:04 ` Jacek Anaszewski 2016-11-15 14:30 ` Hans de Goede 0 siblings, 1 reply; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-15 14:04 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 14:04 ` Jacek Anaszewski @ 2016-11-15 14:30 ` Hans de Goede 2016-11-15 14:41 ` Jacek Anaszewski 0 siblings, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-15 14:30 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 14:30 ` Hans de Goede @ 2016-11-15 14:41 ` Jacek Anaszewski 0 siblings, 0 replies; 50+ messages in thread From: Jacek Anaszewski @ 2016-11-15 14:41 UTC (permalink / raw) To: Hans de Goede, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 12:06 ` Hans de Goede 2016-11-15 12:11 ` Pavel Machek 2016-11-15 13:28 ` Jacek Anaszewski @ 2016-11-17 22:12 ` Hans de Goede 2 siblings, 0 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-17 22:12 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 >> ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 2016-11-15 10:58 ` Jacek Anaszewski 2016-11-15 11:11 ` Pavel Machek @ 2016-11-15 11:17 ` Hans de Goede 1 sibling, 0 replies; 50+ messages in thread From: Hans de Goede @ 2016-11-15 11:17 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Jacek Anaszewski, Tony Lindgren, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-12 21:14 ` Hans de Goede 2016-11-13 11:44 ` Jacek Anaszewski @ 2016-11-14 8:31 ` Pavel Machek 1 sibling, 0 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-14 8:31 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, Tony Lindgren, Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 1568 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-11 17:03 ` Jacek Anaszewski 2016-11-11 19:28 ` Hans de Goede @ 2016-11-11 22:06 ` Pavel Machek 1 sibling, 0 replies; 50+ messages in thread From: Pavel Machek @ 2016-11-11 22:06 UTC (permalink / raw) To: Jacek Anaszewski Cc: Tony Lindgren, Jacek Anaszewski, Hans de Goede, linux-leds, linux-omap, linux-arm-kernel, linux-kernel, Darren Hart [-- Attachment #1: Type: text/plain, Size: 723 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-09 19:23 PM regression with LED changes in next-20161109 Tony Lindgren 2016-11-09 20:45 ` Jacek Anaszewski @ 2016-11-10 8:34 ` Hans de Goede 2016-11-10 15:11 ` Tony Lindgren 1 sibling, 1 reply; 50+ messages in thread From: Hans de Goede @ 2016-11-10 8:34 UTC (permalink / raw) To: Tony Lindgren, Jacek Anaszewski Cc: linux-leds, linux-omap, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: PM regression with LED changes in next-20161109 2016-11-10 8:34 ` Hans de Goede @ 2016-11-10 15:11 ` Tony Lindgren 0 siblings, 0 replies; 50+ messages in thread From: Tony Lindgren @ 2016-11-10 15:11 UTC (permalink / raw) To: Hans de Goede Cc: Jacek Anaszewski, linux-leds, linux-omap, linux-arm-kernel, linux-kernel * 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2016-11-17 22:13 UTC | newest] Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-09 19:23 PM regression with LED changes in next-20161109 Tony Lindgren 2016-11-09 20:45 ` Jacek Anaszewski 2016-11-10 8:49 ` Hans de Goede 2016-11-10 12:56 ` Jacek Anaszewski 2016-11-10 13:04 ` Hans de Goede 2016-11-10 13:55 ` Jacek Anaszewski 2016-11-10 16:36 ` Pavel Machek 2016-11-10 16:29 ` Pavel Machek 2016-11-10 16:44 ` Hans de Goede 2016-11-10 20:48 ` Pavel Machek 2016-11-11 8:25 ` Hans de Goede 2016-11-10 17:55 ` Tony Lindgren 2016-11-10 20:29 ` Pavel Machek 2016-11-10 21:34 ` Jacek Anaszewski 2016-11-11 12:01 ` Pavel Machek 2016-11-11 17:03 ` Jacek Anaszewski 2016-11-11 19:28 ` Hans de Goede 2016-11-11 22:12 ` Pavel Machek 2016-11-12 8:03 ` Hans de Goede 2016-11-13 9:10 ` Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109) Pavel Machek 2016-11-13 9:44 ` Hans de Goede 2016-11-13 20:45 ` Pavel Machek 2016-11-12 10:24 ` PM regression with LED changes in next-20161109 Jacek Anaszewski 2016-11-12 10:33 ` Hans de Goede 2016-11-12 19:14 ` Jacek Anaszewski 2016-11-12 21:14 ` Hans de Goede 2016-11-13 11:44 ` Jacek Anaszewski 2016-11-13 13:52 ` Hans de Goede 2016-11-14 9:12 ` Jacek Anaszewski 2016-11-14 12:51 ` Hans de Goede 2016-11-15 10:01 ` Jacek Anaszewski 2016-11-15 10:09 ` Hans de Goede 2016-11-15 10:31 ` LEDs that change brightness "itself" -- that's a trigger. " Pavel Machek 2016-11-15 10:58 ` Jacek Anaszewski 2016-11-15 11:11 ` Pavel Machek 2016-11-15 11:21 ` Hans de Goede 2016-11-15 11:48 ` Pavel Machek 2016-11-15 12:06 ` Hans de Goede 2016-11-15 12:11 ` Pavel Machek 2016-11-15 13:28 ` Jacek Anaszewski 2016-11-15 13:48 ` Hans de Goede 2016-11-15 14:04 ` Jacek Anaszewski 2016-11-15 14:30 ` Hans de Goede 2016-11-15 14:41 ` Jacek Anaszewski 2016-11-17 22:12 ` Hans de Goede 2016-11-15 11:17 ` Hans de Goede 2016-11-14 8:31 ` Pavel Machek 2016-11-11 22:06 ` Pavel Machek 2016-11-10 8:34 ` Hans de Goede 2016-11-10 15:11 ` Tony Lindgren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).