On Mon, May 19, 2025 at 06:43:23PM +1000, Jonathan Matthew wrote:
> On my X13 gen 1, the screen brightness down button sets the screen to
> minimum brightness, which is not very pleasant.  This turns out to be
> because both acpithinkpad(4) and acpivout(4) are processing these events,
> even though this is a 'version 2' machine where acpithinkpad declines to
> take control of screen brightness.
> 
> The diff below resolves this by only unmasking the brightness up/down
> events on version 1 machines.  Without these events unmasked, only
> acpivout is trying to apply screen brightness changes, and everything
> works properly.
> 
> Has anyone seen this problem on other thinkpads?  Could this change
> cause problems anywhere else?

So this has been tested on version 1 machines (X240, T530), version 2
machines with native backlight control (X13 gen 1 and 2), and a version
2 machine with ACPI backlight control (P51).

It doesn't break anything on the version 1 machines, and it improves
backlight control using the brightness up/down hotkeys on the version
2 machines.

As this only changes event masking for these hotkeys, it won't affect
userspace driven backlight control, which will already be going through
acpivout or *drm native support.

This also brings us more in line with how linux handles the thinkpad
brightness events, at least as I understand it, as seen here: 
https://github.com/torvalds/linux/blob/7d4e49a77d9930c69751b9192448fda6ff9100f1/drivers/platform/x86/thinkpad_acpi.c#L3508
It involves userspace stuff on Linux, but the idea is the same - you
don't want two events for the same button press.

ok?

> 
> Index: acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> diff -u -p -r1.74 acpithinkpad.c
> --- acpithinkpad.c	7 Jul 2023 07:37:59 -0000	1.74
> +++ acpithinkpad.c	19 May 2025 08:26:54 -0000
> @@ -428,10 +428,11 @@ thinkpad_enable_events(struct acpithinkp
>  	}
>  
>  	/* Enable events we need to know about */
> -	mask |= (THINKPAD_MASK_MIC_MUTE	|
> -	    THINKPAD_MASK_BRIGHTNESS_UP |
> -	    THINKPAD_MASK_BRIGHTNESS_DOWN |
> -	    THINKPAD_MASK_KBD_BACKLIGHT);
> +	mask |= THINKPAD_MASK_MIC_MUTE |
> +	    THINKPAD_MASK_KBD_BACKLIGHT;
> +	if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1)
> +		mask |= THINKPAD_MASK_BRIGHTNESS_UP |
> +		    THINKPAD_MASK_BRIGHTNESS_DOWN;
>  
>  	DPRINTF(("%s: setting event mask to 0x%llx\n", DEVNAME(sc), mask));
>  
>