[PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness keypress

[PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness keypress

[Hans de Goede]

Hi All,

Here is v2 of my cleaned up version of Linus' patch to fix this. New this
version is that the module option is left as a boolean, and the keypresses
handling simply always gets delayed 100 ms.

Bjørn, can you please re-test to make sure I've not accidentally broken
anything ? Note this still applies on top of the revert.

Regards,

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

[PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness up/down keypress

[Hans de Goede]

From: Linus Torvalds <torvalds@linux-foundation.org>

In various scenarious userspace will respond to brightness up/down keypresses
by increasing/decreasing the backlight brightness itself. If the kernel then
also changes the brightness this results in the brightness having changed 2
steps for a single keypress which is undesirable. See e.g. :

https://bugs.launchpad.net/gnome-settings-daemon/+bug/527157
http://askubuntu.com/questions/173921/why-does-my-thinkpad-brightness-control-skip-steps

This commit delays responding to brightness up/down keypresses by 100 ms and
if userspace in that time responds by changing the backlight itself, cancels
the kernels own handling of these keypresses, fixing the 2 steps issue.

[hdegoede@redhat.com: Move the delayed_work struct into struct
 acpi_video_device instead of having it as a global]
[hdegoede@redhat.com: Keep brightness_switch_enabled as a boolean and always
 delay the keypress handling]
Tested-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>

--

Changes in v2:
-Keep brightness_switch_enabled as a boolean and always delay the handling
---
 drivers/acpi/video.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1a450c9..18c0e69 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -204,6 +204,8 @@ struct acpi_video_device {
 	struct acpi_video_device_flags flags;
 	struct acpi_video_device_cap cap;
 	struct list_head entry;
+	struct delayed_work switch_brightness_work;
+	int switch_brightness_event;
 	struct acpi_video_bus *video;
 	struct acpi_device *dev;
 	struct acpi_video_device_brightness *brightness;
@@ -230,8 +232,7 @@ static int acpi_video_device_lcd_get_level_current(
 			unsigned long long *level, bool raw);
 static int acpi_video_get_next_level(struct acpi_video_device *device,
 				     u32 level_current, u32 event);
-static int acpi_video_switch_brightness(struct acpi_video_device *device,
-					 int event);
+static void acpi_video_switch_brightness(struct work_struct *work);
 
 static bool acpi_video_use_native_backlight(void)
 {
@@ -275,6 +276,7 @@ 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(&vd->switch_brightness_work);
 	return acpi_video_device_lcd_set_level(vd,
 				vd->brightness->levels[request_level]);
 }
@@ -1252,6 +1254,8 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	data->device_id = device_id;
 	data->video = video;
 	data->dev = device;
+	INIT_DELAYED_WORK(&data->switch_brightness_work,
+			  acpi_video_switch_brightness);
 
 	attribute = acpi_video_get_device_attr(video, device_id);
 
@@ -1474,15 +1478,18 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	}
 }
 
-static int
-acpi_video_switch_brightness(struct acpi_video_device *device, int event)
+static void
+acpi_video_switch_brightness(struct work_struct *work)
 {
+	struct acpi_video_device *device = container_of(to_delayed_work(work),
+			     struct acpi_video_device, switch_brightness_work);
 	unsigned long long level_current, level_next;
+	int event = device->switch_brightness_event;
 	int result = -EINVAL;
 
 	/* no warning message if acpi_backlight=vendor or a quirk is used */
 	if (!acpi_video_verify_backlight_support())
-		return 0;
+		return;
 
 	if (!device->brightness)
 		goto out;
@@ -1504,8 +1511,6 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
 out:
 	if (result)
 		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
-
-	return result;
 }
 
 int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
@@ -1673,6 +1678,16 @@ 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;
+
+	video_device->switch_brightness_event = event;
+	schedule_delayed_work(&video_device->switch_brightness_work, HZ / 10);
+}
+
 static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_video_device *video_device = data;
@@ -1690,28 +1705,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:
-- 
2.0.1

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

Re: [PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness keypress

[Bjørn Mork]

Hans de Goede <hdegoede@redhat.com> writes:

> Hi All,
>
> Here is v2 of my cleaned up version of Linus' patch to fix this. New this
> version is that the module option is left as a boolean, and the keypresses
> handling simply always gets delayed 100 ms.
>
> Bjørn, can you please re-test to make sure I've not accidentally broken
> anything ? Note this still applies on top of the revert.

Still works fine for me.  Sorry about the testing delay.


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

Re: [PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness keypress

[Hans de Goede]

Hi,

On 07/22/2014 04:35 PM, Bjørn Mork wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Hi All,
>>
>> Here is v2 of my cleaned up version of Linus' patch to fix this. New this
>> version is that the module option is left as a boolean, and the keypresses
>> handling simply always gets delayed 100 ms.
>>
>> Bjørn, can you please re-test to make sure I've not accidentally broken
>> anything ? Note this still applies on top of the revert.
>
> Still works fine for me.  Sorry about the testing delay.

np. thanks for testing.

Regards,

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

Re: [PATCH v2] acpi-video: Fix backlight taking 2 steps on a brightness keypress

[Rafael J. Wysocki]

On Tuesday, July 22, 2014 05:10:55 PM Hans de Goede wrote:
> Hi,
> 
> On 07/22/2014 04:35 PM, Bjørn Mork wrote:
> > Hans de Goede <hdegoede@redhat.com> writes:
> >
> >> Hi All,
> >>
> >> Here is v2 of my cleaned up version of Linus' patch to fix this. New this
> >> version is that the module option is left as a boolean, and the keypresses
> >> handling simply always gets delayed 100 ms.
> >>
> >> Bjørn, can you please re-test to make sure I've not accidentally broken
> >> anything ? Note this still applies on top of the revert.
> >
> > Still works fine for me.  Sorry about the testing delay.
> 
> np. thanks for testing.

I've queued up this patch for 3.17, thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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