On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote: > Hi, > > On 01-03-17 12:15, Pali Rohár wrote: > >On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote: > >>1) These events do not happen 100s of times per second, they happen > >>almost never > > > >100 events per second is probably not happening, but on my Latitude I > >see that sometimes more events are delayed and delivered at once. > > > >>2) This is the best we can do given the firmware interface we have > > > >What about just dropping upcoming one event in interval of next 2-3 > >second? Instead of trying to remember last state in kernel and then > >trying to mach if received event was that which was caused by change? > > That is way more racy then the solution with the kernel remembering > the brightness. With my proposed solution there is basically no race, Is is racy. When you change keyboard backlight via LED sysfs and also in that time kernel receive hardware event about changed backlight, kernel do not know if that event comes as reaction to change via LED sysfs or as autonomous firmware decision or as reaction to pressing key for changing keyboard backlight (managed by firmware). I'm not talking here about userspace libsmbios. All is just by autonomous firmware, keyboard usage and LED sysfs API. And how is my idea about dropping exactly one event for one request for changing backlight via LED sysfs API more racy as yours? Received event from firmware has only the timestamp, no more information yet (*). Therefore state of events does is not ordered and any two events are basically same. In your provides patch you are adding additional information to event. It is currently read brightness level in time when was event delivered, not state of time when event was created. Which means you cannot swap any two received events anymore as they are part of current driver state. In my idea I do not add any additional information to event. Which means all received events are same in current driver state and I can swap them. So I can decide to drop one event when driver is changing backlight level. And because I can swap received events it does not matter if I drop "correct" one (that which was really generated by my backlight level change) or any other before or after (as they are same). The only important is that kernel use correct number of firmware events. Idea for time interval of 2-3 seconds is that if event is delivered later as after 3 seconds it is probably useless as driver application code can be already out-of-sync. But this time interval is just another idea and we do not need to use it. (*) Looks like for some machines in dell-wmi.c we know also which brightness level was set by firmware, but it is not used now and we still have machines without this information. > the only downside is the driver seeing a brightness change caused by > libsmbios as one caused by the hotkeys, but it will do so 100% > reliably and that is something which I believe we can live with > just fine. > > >This would allow us to reduce doing one SMM call for each received > >event and I think it would be same reliable as your approach. > > As I explained *already* we already have only one SMM call for reach > received event, we pass the read-back brightness into > led_classdev_notify_brightness_hw_changed and when userspace > reads the new brightness_hw_changed event in response to the wakeup > the led-core will use the value passed through > led_classdev_notify_brightness_hw_changed so only 1 SMM call happens > per event. > > Also again this does not happen 100 times per second you're really > trying to optimize something which does not need any optimization > at all here. We have reports that on some Dell notebooks is issuing SMM call causing freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for a longer time...). > Regards, > > Hans > -- Pali Rohár pali.rohar@xxxxxxxxx
