| Message ID | CA+55aFzHo4BEO8W0yza7g0YMgg-b=hPbk20-vpR3s==ydpmAOg@mail.gmail.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Mon, Jul 14, 2014 at 9:24 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Bjørn, what's your setup? Is this perhaps solvable some other way? Just to answer that: I don't use any particular desktop environment. I have acpid running to take care of the most basic power management stuff. My X session is simply WindowMaker (sic) running directly from lightdm. No session management or fancy policy daemons. So I don't have any daemon which would react on the brightness key codes. Now, it's not that I would mind adding a daemon to handle stuff like brightness control. I reported this as a bug because I was a bit surprised by the existing behaviour breaking like that, and I thought that other users might be as surprised as me. Some maybe even without the ability to track down the change and the setting that would restore the old behaviour. > For example, I wonder if we could fix the "dual brightness change" > problem automatically by making a new option for > 'brightness_switch_enabled'. > > Currently, there are two cases: > > - enabled: do the actual brightness change _and_ send the input > report keycode for a brightness change > > - disabled: just send the keycode, excpecting the desktop software to > handle it. > > and maybe we could have a new case (and make *that* the default): > > - delayed: send the keycode, and set up a delayed timer (say, one > tenth of a second) to do the actual brightness change. And if a > brightness change from user mode comes in during that delay, we cancel > the kernel-induced pending change. That sounds like a good solution to me FWIW. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 14, 2014 at 1:45 PM, Bjørn Mork <bjorn@mork.no> wrote: >> brightness change from user mode comes in during that delay, we cancel >> the kernel-induced pending change. > > That sounds like a good solution to me FWIW. Try the patch. It *might* work. I'm not saying it will, but it seemed to at least compile for me. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Mon, Jul 14, 2014 at 1:45 PM, Bjørn Mork <bjorn@mork.no> wrote: >>> brightness change from user mode comes in during that delay, we cancel >>> the kernel-induced pending change. >> >> That sounds like a good solution to me FWIW. > > Try the patch. It *might* work. I'm not saying it will, but it seemed > to at least compile for me. Yes, the patch works as advertised for me. Thanks. But this will break existing configs: > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -68,8 +68,8 @@ MODULE_AUTHOR("Bruno Ducrot"); > MODULE_DESCRIPTION("ACPI Video Driver"); > MODULE_LICENSE("GPL"); > > -static bool brightness_switch_enabled; > -module_param(brightness_switch_enabled, bool, 0644); > +static int brightness_switch_enabled = -1; > +module_param(brightness_switch_enabled, int, 0644); > > /* > * By default, we don't allow duplicate ACPI video bus devices Any setup using e.g "options video brightness_switch_enabled=Y" will barf on this, won't they? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 14, 2014 at 2:46 PM, Bjørn Mork <bjorn@mork.no> wrote: > > Yes, the patch works as advertised for me. Thanks. Ok, so it should probably be tested by people who see the "move by two" problem too. > But this will break existing configs: > > Any setup using e.g "options video brightness_switch_enabled=Y" will > barf on this, won't they? Hmm. Probably. That said, that kind of breakage I think we might want to live with, especially if it actually ends up causing them to test the new default (which might just work for them). But regardless, let's make sure that patch (or something _like_ it) works for people who saw the reverse problem for you. Then the exact syntax of whatever module parameter (if any) can be a separate discussion. Anyway, for 3.16 I think we should just do what we used to do (ie the revert that Rafael apparently already has queued up), so this isn't necessarily critical. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 071c1dfb93f3..9c4afface8e7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -68,8 +68,8 @@ MODULE_AUTHOR("Bruno Ducrot"); MODULE_DESCRIPTION("ACPI Video Driver"); MODULE_LICENSE("GPL"); -static bool brightness_switch_enabled; -module_param(brightness_switch_enabled, bool, 0644); +static int brightness_switch_enabled = -1; +module_param(brightness_switch_enabled, int, 0644); /* * By default, we don't allow duplicate ACPI video bus devices @@ -270,11 +270,21 @@ static int acpi_video_get_brightness(struct backlight_device *bd) return 0; } +static u32 acpi_brightness_event; +static struct acpi_video_device *acpi_brightness_device; +static void acpi_video_set_brighness_delayed(struct work_struct *work) +{ + acpi_video_switch_brightness(acpi_brightness_device, acpi_brightness_event); +} + +static DECLARE_DELAYED_WORK(acpi_brightness_work, acpi_video_set_brighness_delayed); + static int acpi_video_set_brightness(struct backlight_device *bd) { int request_level = bd->props.brightness + 2; struct acpi_video_device *vd = bl_get_data(bd); + cancel_delayed_work(&acpi_brightness_work); return acpi_video_device_lcd_set_level(vd, vd->brightness->levels[request_level]); } @@ -1601,6 +1611,19 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) return; } +static void brightness_switch_event(struct acpi_video_device *video_device, u32 event) +{ + if (!brightness_switch_enabled) + return; + if (brightness_switch_enabled > 0) { + acpi_video_switch_brightness(video_device, event); + return; + } + acpi_brightness_device = video_device; + acpi_brightness_event = event; + schedule_delayed_work(&acpi_brightness_work, HZ/10); +} + static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data) { struct acpi_video_device *video_device = data; @@ -1618,28 +1641,23 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data) switch (event) { case ACPI_VIDEO_NOTIFY_CYCLE_BRIGHTNESS: /* Cycle brightness */ - if (brightness_switch_enabled) - acpi_video_switch_brightness(video_device, event); + brightness_switch_event(video_device, event); keycode = KEY_BRIGHTNESS_CYCLE; break; case ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS: /* Increase brightness */ - if (brightness_switch_enabled) - acpi_video_switch_brightness(video_device, event); + brightness_switch_event(video_device, event); keycode = KEY_BRIGHTNESSUP; break; case ACPI_VIDEO_NOTIFY_DEC_BRIGHTNESS: /* Decrease brightness */ - if (brightness_switch_enabled) - acpi_video_switch_brightness(video_device, event); + brightness_switch_event(video_device, event); keycode = KEY_BRIGHTNESSDOWN; break; case ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS: /* zero brightness */ - if (brightness_switch_enabled) - acpi_video_switch_brightness(video_device, event); + brightness_switch_event(video_device, event); keycode = KEY_BRIGHTNESS_ZERO; break; case ACPI_VIDEO_NOTIFY_DISPLAY_OFF: /* display device off */ - if (brightness_switch_enabled) - acpi_video_switch_brightness(video_device, event); + brightness_switch_event(video_device, event); keycode = KEY_DISPLAY_OFF; break; default: