acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

joshua stein-3
Newer ThinkPads have ACPI goo to allow acpivout to control screen
backlight, so don't take over ws_[gs]et_param from it.  This allows
for 100 levels of backlight control rather than the 10 or 15 that
are supported through acpithinkpad using its proprietary ACPI or
CMOS interfaces.

You can see the difference with and without this patch by doing:

    xbacklight -set 1 -steps 100
    xbacklight -set 100 -steps 100

Apparently this will also be needed for newer AMD ThinkPads that use
radeondrm.

"Newer" here is being defined as anything not reporting version 1
(THINKPAD_HKEY_VERSION1) of the ThinkPad ACPI interface.

For responding to hardware brightness keys, you'll want to test with
the acpivout patch I posted since otherwise the keys will be
adjusting the backlight by 1% each time, and it may seem like it's
not doing anything.  That patch makes it properly adjust by 5% each
time (but you still get fine-grained changes through wsconsctl or
xbacklight).


Index: sys/dev/acpi/acpithinkpad.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
retrieving revision 1.66
diff -u -p -u -p -r1.66 acpithinkpad.c
--- sys/dev/acpi/acpithinkpad.c 13 Oct 2019 10:56:31 -0000 1.66
+++ sys/dev/acpi/acpithinkpad.c 14 Oct 2019 02:28:57 -0000
@@ -320,8 +320,10 @@ thinkpad_attach(struct device *parent, s
  wskbd_set_backlight = thinkpad_set_kbd_backlight;
  }
 
- if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
-    0, NULL, &sc->sc_brightness) == 0) {
+ /* On version 2 and newer, let *drm or acpivout control brightness */
+ if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
+    (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
+    0, NULL, &sc->sc_brightness) == 0)) {
  ws_get_param = thinkpad_get_param;
  ws_set_param = thinkpad_set_param;
  }

Reply | Threaded
Open this post in threaded view
|

Re: acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

Mark Kettenis
> Date: Sun, 13 Oct 2019 21:37:53 -0500
> From: joshua stein <[hidden email]>
>
> Newer ThinkPads have ACPI goo to allow acpivout to control screen
> backlight, so don't take over ws_[gs]et_param from it.  This allows
> for 100 levels of backlight control rather than the 10 or 15 that
> are supported through acpithinkpad using its proprietary ACPI or
> CMOS interfaces.
>
> You can see the difference with and without this patch by doing:
>
>     xbacklight -set 1 -steps 100
>     xbacklight -set 100 -steps 100
>
> Apparently this will also be needed for newer AMD ThinkPads that use
> radeondrm.
>
> "Newer" here is being defined as anything not reporting version 1
> (THINKPAD_HKEY_VERSION1) of the ThinkPad ACPI interface.

Note that -current reports the version number in dmesg.  For now
you'll need to compile your own kernel though, since there won't be
any new snapshots until 6.6 is released.

> For responding to hardware brightness keys, you'll want to test with
> the acpivout patch I posted since otherwise the keys will be
> adjusting the backlight by 1% each time, and it may seem like it's
> not doing anything.  That patch makes it properly adjust by 5% each
> time (but you still get fine-grained changes through wsconsctl or
> xbacklight).

Wo what we need to get tested is whether backlight control (stil)
works on machines that report:

acpithinkpad0 at acpi0: version 2.0

My x1c3 reports version 1.0 so I suspect the x250/t450/t550 reports
that as well (but it would be goo to get that confirmed).  So we're
primarily interested in later generations.

> Index: sys/dev/acpi/acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.66
> diff -u -p -u -p -r1.66 acpithinkpad.c
> --- sys/dev/acpi/acpithinkpad.c 13 Oct 2019 10:56:31 -0000 1.66
> +++ sys/dev/acpi/acpithinkpad.c 14 Oct 2019 02:28:57 -0000
> @@ -320,8 +320,10 @@ thinkpad_attach(struct device *parent, s
>   wskbd_set_backlight = thinkpad_set_kbd_backlight;
>   }
>  
> - if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> -    0, NULL, &sc->sc_brightness) == 0) {
> + /* On version 2 and newer, let *drm or acpivout control brightness */
> + if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
> +    (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> +    0, NULL, &sc->sc_brightness) == 0)) {
>   ws_get_param = thinkpad_get_param;
>   ws_set_param = thinkpad_set_param;
>   }
>
>

Reply | Threaded
Open this post in threaded view
|

Re: acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

Tracey Emery
In reply to this post by joshua stein-3
Patch works great on a T470s. Stepping is smooth now. No ill effects.

Thanks,

Tracey

On Sun, Oct 13, 2019 at 09:37:53PM -0500, joshua stein wrote:

> Newer ThinkPads have ACPI goo to allow acpivout to control screen
> backlight, so don't take over ws_[gs]et_param from it.  This allows
> for 100 levels of backlight control rather than the 10 or 15 that
> are supported through acpithinkpad using its proprietary ACPI or
> CMOS interfaces.
>
> You can see the difference with and without this patch by doing:
>
>     xbacklight -set 1 -steps 100
>     xbacklight -set 100 -steps 100
>
> Apparently this will also be needed for newer AMD ThinkPads that use
> radeondrm.
>
> "Newer" here is being defined as anything not reporting version 1
> (THINKPAD_HKEY_VERSION1) of the ThinkPad ACPI interface.
>
> For responding to hardware brightness keys, you'll want to test with
> the acpivout patch I posted since otherwise the keys will be
> adjusting the backlight by 1% each time, and it may seem like it's
> not doing anything.  That patch makes it properly adjust by 5% each
> time (but you still get fine-grained changes through wsconsctl or
> xbacklight).
>
>
> Index: sys/dev/acpi/acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.66
> diff -u -p -u -p -r1.66 acpithinkpad.c
> --- sys/dev/acpi/acpithinkpad.c 13 Oct 2019 10:56:31 -0000 1.66
> +++ sys/dev/acpi/acpithinkpad.c 14 Oct 2019 02:28:57 -0000
> @@ -320,8 +320,10 @@ thinkpad_attach(struct device *parent, s
>   wskbd_set_backlight = thinkpad_set_kbd_backlight;
>   }
>  
> - if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> -    0, NULL, &sc->sc_brightness) == 0) {
> + /* On version 2 and newer, let *drm or acpivout control brightness */
> + if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
> +    (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> +    0, NULL, &sc->sc_brightness) == 0)) {
>   ws_get_param = thinkpad_get_param;
>   ws_set_param = thinkpad_set_param;
>   }

--

Tracey Emery

Reply | Threaded
Open this post in threaded view
|

Re: acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

Theo Buehler-3
In reply to this post by Mark Kettenis
On Mon, Oct 14, 2019 at 11:00:57PM +0200, Mark Kettenis wrote:

> > Date: Sun, 13 Oct 2019 21:37:53 -0500
> > From: joshua stein <[hidden email]>
> >
> > Newer ThinkPads have ACPI goo to allow acpivout to control screen
> > backlight, so don't take over ws_[gs]et_param from it.  This allows
> > for 100 levels of backlight control rather than the 10 or 15 that
> > are supported through acpithinkpad using its proprietary ACPI or
> > CMOS interfaces.
> >
> > You can see the difference with and without this patch by doing:
> >
> >     xbacklight -set 1 -steps 100
> >     xbacklight -set 100 -steps 100
> >
> > Apparently this will also be needed for newer AMD ThinkPads that use
> > radeondrm.
> >
> > "Newer" here is being defined as anything not reporting version 1
> > (THINKPAD_HKEY_VERSION1) of the ThinkPad ACPI interface.
>
> Note that -current reports the version number in dmesg.  For now
> you'll need to compile your own kernel though, since there won't be
> any new snapshots until 6.6 is released.
>
> > For responding to hardware brightness keys, you'll want to test with
> > the acpivout patch I posted since otherwise the keys will be
> > adjusting the backlight by 1% each time, and it may seem like it's
> > not doing anything.  That patch makes it properly adjust by 5% each
> > time (but you still get fine-grained changes through wsconsctl or
> > xbacklight).
>
> Wo what we need to get tested is whether backlight control (stil)
> works on machines that report:
>
> acpithinkpad0 at acpi0: version 2.0
>
> My x1c3 reports version 1.0 so I suspect the x250/t450/t550 reports
> that as well (but it would be goo to get that confirmed).  So we're
> primarily interested in later generations.

My x280 reports version 2.0 and this feels like a great improvement.

Reply | Threaded
Open this post in threaded view
|

Re: acpithinkpad: don't take over ws_[gs]et_param on version 2 devices

Christian Weisgerber
In reply to this post by Mark Kettenis
On 2019-10-14, Mark Kettenis <[hidden email]> wrote:

> Wo what we need to get tested is whether backlight control (stil)
> works on machines that report:
>
> acpithinkpad0 at acpi0: version 2.0

Like my X1C5.  I've tested the patches there, and they work as
intended.  The acpithinkpad diff causes working backlight control
in 1% steps, and the acpivout diff increases that to more sensible
5% steps.

--
Christian "naddy" Weisgerber                          [hidden email]