[Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Ben Greening]

(resending because of HTML formatting)
Hi, I'm on Arch Linux and upgraded from kernel 5.18.9.arch1-1 to
5.18.10.arch1-1. The brightness keys don't work as well as before.
Gnome had 20 degrees of brightness, now it's 10, and Xfce went from 10
to 5. Additionally, on Gnome the brightness keys are a little slow to
respond and there's a bit of a stutter. Don't know why Xfce doesn't
stutter, but halving the degrees of brightness for both makes me
wonder if each press is being counted twice.

Reverting commit 3a0cf7ab8d in acpi_video.c and rebuilding
5.18.10.arch1-1 fixed it.
The laptop is a Dell Inspiron n4010 and I use "acpi_backlight=video"
to make the brightness keys work. Please let me know if there's any
hardware info you need.

#regzbot introduced: 3a0cf7ab8d

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Thorsten Leemhuis]

[CCing Hans, who authored 3a0cf7ab8d ("ACPI: video: Change how we
determine if brightness key-presses are handled")]

On 13.07.22 07:27, Ben Greening wrote:
> (resending because of HTML formatting)
> Hi, I'm on Arch Linux and upgraded from kernel 5.18.9.arch1-1 to
> 5.18.10.arch1-1. The brightness keys don't work as well as before.
> Gnome had 20 degrees of brightness, now it's 10, and Xfce went from 10
> to 5. Additionally, on Gnome the brightness keys are a little slow to
> respond and there's a bit of a stutter. Don't know why Xfce doesn't
> stutter, but halving the degrees of brightness for both makes me
> wonder if each press is being counted twice.
> 
> Reverting commit 3a0cf7ab8d in acpi_video.c and rebuilding
> 5.18.10.arch1-1 fixed it.

BTW, in case someone gets slightly confused like I was here:
3a0cf7ab8d is a mainline commit that was backported to 5.18.y as
1ed81b354d8c recently.

> The laptop is a Dell Inspiron n4010 and I use "acpi_backlight=video"
> to make the brightness keys work. Please let me know if there's any
> hardware info you need.
> 
> #regzbot introduced: 3a0cf7ab8d

BTW, thx for the report!

Ciao, Thorsten

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

Hi Ben,

On 7/13/22 07:27, Ben Greening wrote:
> (resending because of HTML formatting)
> Hi, I'm on Arch Linux and upgraded from kernel 5.18.9.arch1-1 to
> 5.18.10.arch1-1. The brightness keys don't work as well as before.
> Gnome had 20 degrees of brightness, now it's 10, and Xfce went from 10
> to 5. Additionally, on Gnome the brightness keys are a little slow to
> respond and there's a bit of a stutter. Don't know why Xfce doesn't
> stutter, but halving the degrees of brightness for both makes me
> wonder if each press is being counted twice.

Author of the troublesome patch here, sorry that this broke things
for you.

So this sounds like you are getting duplicate key-events reported,
causing the brightness to take 2 steps on each key-press which is
likely also causing the perceived stutter.

This suggests that acpi_video_handles_brightness_key_presses()
was returning true on your system and is now returning false.

Lets confirm this theory, please run either evtest or evemu-record
as root and then record events from the "Video Bus" device and then
press the brightness up/down keys. Press CTRL+C to exit. After this
repeat selecting the "Dell WMI hotkeys" device as input device.

I expect both tests/recordings to show brightness key events with
the troublesome kernel, showing that you are getting duplicate events.

If this is the case then as a workaround you can add:

video.report_key_events=1

to the kernel commandline. This should silence the "Video Bus"
events. Also can you provide the output of:

ls /sys/class/backlight

please?


> Reverting commit 3a0cf7ab8d in acpi_video.c and rebuilding
> 5.18.10.arch1-1 fixed it.

> The laptop is a Dell Inspiron n4010 and I use "acpi_backlight=video"
> to make the brightness keys work. Please let me know if there's any
> hardware info you need.

Note needing to add a commandline argument like this to get things
to work is something which really should always be reported upstream,
so that we can either adjust our heuristics; or add a quirk for your
laptop-model so that things will just work when another user tries
Linux on the same model.

So while at it lets look into fixing this properly to.

When you do not pass anything on the kernel commandline, what
is then the output of:

ls /sys/class/backlight

And for each directory under there, please cd into the dir
and then (as root) do:

cat max_brightness # this gives you the range of this backlight intf.
echo $some-value > brightness

picking some-value in a range of 0-max_brightness, repeating the
echo with different values (e.g. half-range + max) and see if
the screens brightness changes. Please let me know which directories
under /sys/class/backlight result in working backlight control
and which ones do not.

Also what is the output of "ls /sys/class/backlight" when
"acpi_backlight=video" is present on the kernel commandline ?

Regards,

Hans

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Ben Greening]

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.

Thanks again,

Ben

On Wed, Jul 13, 2022 at 2:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ben,
>
> On 7/13/22 07:27, Ben Greening wrote:
> > (resending because of HTML formatting)
> > Hi, I'm on Arch Linux and upgraded from kernel 5.18.9.arch1-1 to
> > 5.18.10.arch1-1. The brightness keys don't work as well as before.
> > Gnome had 20 degrees of brightness, now it's 10, and Xfce went from 10
> > to 5. Additionally, on Gnome the brightness keys are a little slow to
> > respond and there's a bit of a stutter. Don't know why Xfce doesn't
> > stutter, but halving the degrees of brightness for both makes me
> > wonder if each press is being counted twice.
>
> Author of the troublesome patch here, sorry that this broke things
> for you.
>
> So this sounds like you are getting duplicate key-events reported,
> causing the brightness to take 2 steps on each key-press which is
> likely also causing the perceived stutter.
>
> This suggests that acpi_video_handles_brightness_key_presses()
> was returning true on your system and is now returning false.
>
> Lets confirm this theory, please run either evtest or evemu-record
> as root and then record events from the "Video Bus" device and then
> press the brightness up/down keys. Press CTRL+C to exit. After this
> repeat selecting the "Dell WMI hotkeys" device as input device.
>
> I expect both tests/recordings to show brightness key events with
> the troublesome kernel, showing that you are getting duplicate events.
>
> If this is the case then as a workaround you can add:
>
> video.report_key_events=1
>
> to the kernel commandline. This should silence the "Video Bus"
> events. Also can you provide the output of:
>
> ls /sys/class/backlight
>
> please?
>
>
> > Reverting commit 3a0cf7ab8d in acpi_video.c and rebuilding
> > 5.18.10.arch1-1 fixed it.
>
> > The laptop is a Dell Inspiron n4010 and I use "acpi_backlight=video"
> > to make the brightness keys work. Please let me know if there's any
> > hardware info you need.
>
> Note needing to add a commandline argument like this to get things
> to work is something which really should always be reported upstream,
> so that we can either adjust our heuristics; or add a quirk for your
> laptop-model so that things will just work when another user tries
> Linux on the same model.
>
> So while at it lets look into fixing this properly to.
>
> When you do not pass anything on the kernel commandline, what
> is then the output of:
>
> ls /sys/class/backlight
>
> And for each directory under there, please cd into the dir
> and then (as root) do:
>
> cat max_brightness # this gives you the range of this backlight intf.
> echo $some-value > brightness
>
> picking some-value in a range of 0-max_brightness, repeating the
> echo with different values (e.g. half-range + max) and see if
> the screens brightness changes. Please let me know which directories
> under /sys/class/backlight result in working backlight control
> and which ones do not.
>
> Also what is the output of "ls /sys/class/backlight" when
> "acpi_backlight=video" is present on the kernel commandline ?
>
> Regards,
>
> Hans
>

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

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.

For the need to pass acpi_backlight=video, what you are in essence
doing is setting acpi_backlight=native.

The auto-detect code goes to acpi_backlight=vendor because of the lacking
ACPI video backlight control and manually setting acpi_backlight != vendor
disables the dell_backlight. ATM the native (intel) backlight ingnoes
the acpi_backlight setting so it loads unconditionally. But in the near
future this will change and then you need to pass acpi_backlight=native
otherwise the intel backlight will not register because you requested
video.

So I plan to fix this part by adding a quirk to make native the default
on your machine. Can you do:

sudo dmidecode > dmidecode.txt

And email me the generated dmidecode.txt (this will contain serialnumbers
so you may want to send it off-list) ? Then I can also prepare a patch
to add a quirk to make native the default on your model.

Regards,

Hans










> 
> Thanks again,
> 
> Ben
> 
> On Wed, Jul 13, 2022 at 2:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ben,
>>
>> On 7/13/22 07:27, Ben Greening wrote:
>>> (resending because of HTML formatting)
>>> Hi, I'm on Arch Linux and upgraded from kernel 5.18.9.arch1-1 to
>>> 5.18.10.arch1-1. The brightness keys don't work as well as before.
>>> Gnome had 20 degrees of brightness, now it's 10, and Xfce went from 10
>>> to 5. Additionally, on Gnome the brightness keys are a little slow to
>>> respond and there's a bit of a stutter. Don't know why Xfce doesn't
>>> stutter, but halving the degrees of brightness for both makes me
>>> wonder if each press is being counted twice.
>>
>> Author of the troublesome patch here, sorry that this broke things
>> for you.
>>
>> So this sounds like you are getting duplicate key-events reported,
>> causing the brightness to take 2 steps on each key-press which is
>> likely also causing the perceived stutter.
>>
>> This suggests that acpi_video_handles_brightness_key_presses()
>> was returning true on your system and is now returning false.
>>
>> Lets confirm this theory, please run either evtest or evemu-record
>> as root and then record events from the "Video Bus" device and then
>> press the brightness up/down keys. Press CTRL+C to exit. After this
>> repeat selecting the "Dell WMI hotkeys" device as input device.
>>
>> I expect both tests/recordings to show brightness key events with
>> the troublesome kernel, showing that you are getting duplicate events.
>>
>> If this is the case then as a workaround you can add:
>>
>> video.report_key_events=1
>>
>> to the kernel commandline. This should silence the "Video Bus"
>> events. Also can you provide the output of:
>>
>> ls /sys/class/backlight
>>
>> please?
>>
>>
>>> Reverting commit 3a0cf7ab8d in acpi_video.c and rebuilding
>>> 5.18.10.arch1-1 fixed it.
>>
>>> The laptop is a Dell Inspiron n4010 and I use "acpi_backlight=video"
>>> to make the brightness keys work. Please let me know if there's any
>>> hardware info you need.
>>
>> Note needing to add a commandline argument like this to get things
>> to work is something which really should always be reported upstream,
>> so that we can either adjust our heuristics; or add a quirk for your
>> laptop-model so that things will just work when another user tries
>> Linux on the same model.
>>
>> So while at it lets look into fixing this properly to.
>>
>> When you do not pass anything on the kernel commandline, what
>> is then the output of:
>>
>> ls /sys/class/backlight
>>
>> And for each directory under there, please cd into the dir
>> and then (as root) do:
>>
>> cat max_brightness # this gives you the range of this backlight intf.
>> echo $some-value > brightness
>>
>> picking some-value in a range of 0-max_brightness, repeating the
>> echo with different values (e.g. half-range + max) and see if
>> the screens brightness changes. Please let me know which directories
>> under /sys/class/backlight result in working backlight control
>> and which ones do not.
>>
>> Also what is the output of "ls /sys/class/backlight" when
>> "acpi_backlight=video" is present on the kernel commandline ?
>>
>> Regards,
>>
>> Hans
>>
> 

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

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






[-- Attachment #2: 0001-ACPI-video-Change-how-we-determine-if-brightness-key.patch --]
[-- Type: text/x-patch, Size: 3649 bytes --]

From 12da051beea6de3e2fd495fd8b82acce9dfb3eb5 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
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@mail.gmail.com/
Reported-and-tested-by: Ben Greening <bgreening@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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(&register_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

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Ben Greening]

Hi Hans,

Applying the latest
0001-ACPI-video-Change-how-we-determine-if-brightness-key.patch you
sent me off-list (my fault, forgot to reply all) and
0001-ACPI-video-Use-native-backlight-on-Dell-Inspiron-N40.patch makes
it all work again. And a bonus that I don't need any extra kernel
parameters anymore.

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

I rebooted and ran evemu-record before pressing the brightness keys
and "Dell WMI hotkeys" didn't show any events at all.

Thanks for the fix!
Ben

On Wed, Jul 13, 2022 at 6:49 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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
>
>
>
>
>

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

Hi,

On 7/14/22 01:56, Ben Greening wrote:
> Hi Hans,
> 
> Applying the latest
> 0001-ACPI-video-Change-how-we-determine-if-brightness-key.patch you
> sent me off-list (my fault, forgot to reply all) and
> 0001-ACPI-video-Use-native-backlight-on-Dell-Inspiron-N40.patch makes
> it all work again. And a bonus that I don't need any extra kernel
> parameters anymore.
> 
>> 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...
> 
> I rebooted and ran evemu-record before pressing the brightness keys
> and "Dell WMI hotkeys" didn't show any events at all.

Great thank you for reporting and testing this!

I will send the fix on its way to Linus tomorrow and I've just
submitted the new acpi_backlight=native quirk upstream too.

Regards,

Hans


> On Wed, Jul 13, 2022 at 6:49 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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
>>
>>
>>
>>
>>
> 

Re: [Regression] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

Hi again,

On 7/13/22 15:29, Hans de Goede wrote:

<snip>

> So I plan to fix this part by adding a quirk to make native the default
> on your machine. Can you do:
> 
> sudo dmidecode > dmidecode.txt
> 
> And email me the generated dmidecode.txt (this will contain serialnumbers
> so you may want to send it off-list) ? Then I can also prepare a patch
> to add a quirk to make native the default on your model.

I have found a DMI decode for your model here:
https://github.com/linuxhw/DMI/

So I've written the quirk patch (attached) based on that.

Please give this a try. With a 5.18.1x kernel with both patches applied,
everything should work without needing to specify anything on the kernel
commandline.

Regards,

Hans

[-- Attachment #2: 0001-ACPI-video-Use-native-backlight-on-Dell-Inspiron-N40.patch --]
[-- Type: text/x-patch, Size: 1388 bytes --]

From 12b2ae6cbb36860f996a5ca382bb1dda43a4fb8b Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 13 Jul 2022 15:53:08 +0200
Subject: [PATCH] ACPI: video: Use native backlight on Dell Inspiron N4010

The Dell Inspiron N4010 does not have ACPI backlight control,
so acpi_video_get_backlight_type()'s heuristics return vendor as
the type to use.

But the vendor interface is broken, where as the native (intel_backlight)
works well, add a quirk to use native.

Link: https://lore.kernel.org/regressions/CALF=6jEe5G8+r1Wo0vvz4GjNQQhdkLT5p8uCHn6ZXhg4nsOWow@mail.gmail.com/
Reported-and-tested-by: Ben Greening <bgreening@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video_detect.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..4099140bbd5f 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -347,6 +347,14 @@ static const struct dmi_system_id video_detect_dmi_table[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro12,1"),
 		},
 	},
+	{
+	 .callback = video_detect_force_native,
+	 /* Dell Inspiron N4010 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron N4010"),
+		},
+	},
 	{
 	 .callback = video_detect_force_native,
 	 /* Dell Vostro V131 */
-- 
2.36.0