Hi Ben,
On 7/13/22 15:29, Hans de Goede wrote:
> Hi,
>
> On 7/13/22 15:08, Ben Greening wrote:
>> Hi Hans, thanks for getting back to me.
>>
>> evemu-record shows events for both "Video Bus" and "Dell WMI hotkeys":
>>
>> Video Bus
>> E: 0.000001 0001 00e0 0001 # EV_KEY / KEY_BRIGHTNESSDOWN 1
>> E: 0.000001 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +0ms
>> E: 0.000020 0001 00e0 0000 # EV_KEY / KEY_BRIGHTNESSDOWN 0
>> E: 0.000020 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +0ms
>>
>> Dell WMI hotkeys
>> E: 0.000001 0004 0004 57349 # EV_MSC / MSC_SCAN 57349
>> E: 0.000001 0001 00e0 0001 # EV_KEY / KEY_BRIGHTNESSDOWN 1
>> E: 0.000001 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +0ms
>> E: 0.000020 0001 00e0 0000 # EV_KEY / KEY_BRIGHTNESSDOWN 0
>> E: 0.000020 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +0ms
>>
>> Adding video.report_key_events=1 with acpi_backlight=video makes
>> things work like you said it would.
>>
>>
>> With acpi_backlight=video just has intel_backlight.
>>
>> Without acpi_backlight=video:
>> intel_backlight:
>> max_brightness: 4882
>> backlight control works with echo
>> brightness keys make no change to brightness value
>>
>> dell_backlight:
>> max_brightness: 15
>> backlight control doesn't work immediately, but does on reboot
>> to set brightness at POST.
>> brightness keys change brightness value, but you don't see the
>> change until reboot.
>
> Ok, so your system lacks ACPI video backlight control, yet still reports
> brightness keypresses through the ACPI Video Bus. Interesting (weird)...
>
> I think I believe I know how to fix the regression, 1 patch coming up.
Can you please give the attached patch a try, with
video.report_key_events=1 *removed* from the commandline ?
It would also be interesting if you can start evemu-record on the
Dell WMI Hotkeys device before pressing any of the brightness keys.
There might still be a single duplicate event reported there on
the first press. I don't really see a way around that (without causing
all brightness key presses on some panasonic models to be duplicated),
but I'm curious if it is a problem at all...
Regards,
Hans
From 12da051beea6de3e2fd495fd8b82acce9dfb3eb5 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Wed, 13 Jul 2022 15:31:14 +0200
Subject: [PATCH] ACPI: video: Change how we determine if brightness
key-presses are handled again
Commit 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness
key-presses are handled") made acpi_video_handles_brightness_key_presses()
report false when none of the ACPI Video Devices support backlight control.
But it turns out that at least on a Dell Inspiron n4010 there is no ACPI
backlight control, yet brightness hotkeys are still reported through
the ACPI Video Bus; and since acpi_video_handles_brightness_key_presses()
now returns false, brightness keypresses are now reported twice.
To fix this rename the has_backlight flag to may_report_brightness_keys and
also set it the first time a brightness key press event is received.
Depending on the delivery of the other ACPI (WMI) event vs the ACPI Video
Bus event this means that the first brightness key press might still get
reported twice, but all further keypresses will be filtered as before.
Note that this relies on other drivers reporting brightness key events
calling acpi_video_handles_brightness_key_presses() when delivering
the events (rather then once during driver probe). This is already
required and documented in include/acpi/video.h:
/*
* Note: The value returned by acpi_video_handles_brightness_key_presses()
* may change over time and should not be cached.
*/
Fixes: 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness key-presses are handled")
Link: https://lore.kernel.org/regressions/CALF=6jEe5G8+r1Wo0vvz4GjNQQhdkLT5p8uCHn6ZXhg4nsOWow@xxxxxxxxxxxxxx/
Reported-and-tested-by: Ben Greening <bgreening@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/acpi/acpi_video.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index dc3c037d6313..1a350543a6bf 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -79,7 +79,7 @@ module_param(device_id_scheme, bool, 0444);
static int only_lcd = -1;
module_param(only_lcd, int, 0444);
-static bool has_backlight;
+static bool may_report_brightness_keys;
static int register_count;
static DEFINE_MUTEX(register_count_mutex);
static DEFINE_MUTEX(video_list_lock);
@@ -1232,7 +1232,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
acpi_video_device_find_cap(data);
if (data->cap._BCM && data->cap._BCL)
- has_backlight = true;
+ may_report_brightness_keys = true;
mutex_lock(&video->device_list_lock);
list_add_tail(&data->entry, &video->video_device_list);
@@ -1701,6 +1701,9 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
break;
}
+ if (keycode)
+ may_report_brightness_keys = true;
+
acpi_notifier_call_chain(device, event, 0);
if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) {
@@ -2280,7 +2283,7 @@ void acpi_video_unregister(void)
cancel_delayed_work_sync(&video_bus_register_backlight_work);
acpi_bus_unregister_driver(&acpi_video_bus);
register_count = 0;
- has_backlight = false;
+ may_report_brightness_keys = false;
}
mutex_unlock(®ister_count_mutex);
}
@@ -2299,7 +2302,7 @@ EXPORT_SYMBOL(acpi_video_register_backlight);
bool acpi_video_handles_brightness_key_presses(void)
{
- return has_backlight &&
+ return may_report_brightness_keys &&
(report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
}
EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
--
2.36.0
