On Wed, Dec 16, 2015 at 07:48:23PM +0100, Mark Kettenis wrote:
> Most, if not all, somewhat recent Thinkpads have some subtle issues
> with display brightness control. For example,if you change the
> display brightness using wsconsctl(8) or cbacklight(1), and later use
> the brightness control buttons on the keyboard, you're likely to see a
> big jump in brightness. or if you plug or unplug the power, the
> display brightness will suddenly change. The problem here is that on
> these machines we us the display brightness interface provided by
> inteldrm(4), which doesn't coordinate changes with the firmware.
>
> In principle these machines support the standard acpi brightness
> control interface. However that interface is quite badly broken if
> the OS claims to be Windows 8. Unfortunately that is what we do on
> OpenBSD because otherwise some other machines don't work properly.
>
> Fortunately Thinkpads provide an alternative acpi interface through
> the "hotkey" device that is handled by acpithinkpad(4). The diff
> below implements that interface.
>
> The downside of this diff is that number of levels is limited to 16
> whereas we currently have much finer granularity. But I think that is
> acceptable. The levels are probably better calibrated and we now have
> proper coordination between the OS and the firmware when it comes to
> brightness changes. On top of that, this diff will almost certainly
> result in working brightness control on machines that don't have Intel
> graphics.
>
> ok?
>
I'm ok with this. Didn't test it but I don't have objections to moving
in this direction. Diff looks ok too.
-ml
>
> Index: acpithinkpad.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 acpithinkpad.c
> --- acpithinkpad.c 16 Dec 2015 15:43:14 -0000 1.49
> +++ acpithinkpad.c 16 Dec 2015 18:00:03 -0000
> @@ -122,6 +122,8 @@ struct acpithinkpad_softc {
> uint64_t sc_thinklight;
> const char *sc_thinklight_get;
> const char *sc_thinklight_set;
> +
> + uint64_t sc_brightness;
> };
>
> extern void acpiec_read(struct acpiec_softc *, u_int8_t, int, u_int8_t *);
> @@ -141,13 +143,19 @@ int thinkpad_brightness_down(struct acpi
> int thinkpad_adaptive_change(struct acpithinkpad_softc *);
> int thinkpad_activate(struct device *, int);
>
> -/* wskbd hook functions */
> +/* wscons hook functions */
> void thinkpad_get_thinklight(struct acpithinkpad_softc *);
> void thinkpad_set_thinklight(void *, int);
> int thinkpad_get_backlight(struct wskbd_backlight *);
> int thinkpad_set_backlight(struct wskbd_backlight *);
> extern int (*wskbd_get_backlight)(struct wskbd_backlight *);
> extern int (*wskbd_set_backlight)(struct wskbd_backlight *);
> +void thinkpad_get_brightness(struct acpithinkpad_softc *);
> +void thinkpad_set_brightness(void *, int);
> +int thinkpad_get_param(struct wsdisplay_param *);
> +int thinkpad_set_param(struct wsdisplay_param *);
> +extern int (*ws_get_param)(struct wsdisplay_param *);
> +extern int (*ws_set_param)(struct wsdisplay_param *);
>
> void thinkpad_sensor_attach(struct acpithinkpad_softc *sc);
> void thinkpad_sensor_refresh(void *);
> @@ -267,6 +275,12 @@ thinkpad_attach(struct device *parent, s
> wskbd_set_backlight = thinkpad_set_backlight;
> }
>
> + if (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;
> + }
> +
> /* Run thinkpad_hotkey on button presses */
> aml_register_notify(sc->sc_devnode, aa->aaa_dev,
> thinkpad_hotkey, sc, ACPIDEV_POLL);
> @@ -391,6 +405,9 @@ thinkpad_hotkey(struct aml_node *node, i
> thinkpad_adaptive_change(sc);
> handled = 1;
> break;
> + case THINKPAD_BACKLIGHT_CHANGED:
> + thinkpad_get_brightness(sc);
> + break;
> case THINKPAD_ADAPTIVE_BACK:
> case THINKPAD_ADAPTIVE_GESTURES:
> case THINKPAD_ADAPTIVE_REFRESH:
> @@ -398,7 +415,6 @@ thinkpad_hotkey(struct aml_node *node, i
> case THINKPAD_ADAPTIVE_SNIP:
> case THINKPAD_ADAPTIVE_TAB:
> case THINKPAD_ADAPTIVE_VOICE:
> - case THINKPAD_BACKLIGHT_CHANGED:
> case THINKPAD_KEYLIGHT_CHANGED:
> case THINKPAD_BRIGHTNESS_CHANGED:
> case THINKPAD_BUTTON_BATTERY_INFO:
> @@ -630,4 +646,63 @@ thinkpad_set_backlight(struct wskbd_back
> acpi_addtask(sc->sc_acpi, thinkpad_set_thinklight, sc, 0);
> acpi_wakeup(sc->sc_acpi);
> return 0;
> +}
> +
> +void
> +thinkpad_get_brightness(struct acpithinkpad_softc *sc)
> +{
> + aml_evalinteger(sc->sc_acpi, sc->sc_devnode,
> + "PBLG", 0, NULL, &sc->sc_brightness);
> +}
> +
> +void
> +thinkpad_set_brightness(void *arg0, int arg1)
> +{
> + struct acpithinkpad_softc *sc = arg0;
> + struct aml_value arg;
> +
> + memset(&arg, 0, sizeof(arg));
> + arg.type = AML_OBJTYPE_INTEGER;
> + arg.v_integer = sc->sc_brightness & 0xff;
> + aml_evalname(sc->sc_acpi, sc->sc_devnode,
> + "PBLS", 1, &arg, NULL);
> +}
> +
> +int
> +thinkpad_get_param(struct wsdisplay_param *dp)
> +{
> + struct acpithinkpad_softc *sc = acpithinkpad_cd.cd_devs[0];
> +
> + if (sc == NULL)
> + return -1;
> +
> + dp->min = 0;
> + dp->max = (sc->sc_brightness >> 8) & 0xff;
> + dp->curval = sc->sc_brightness & 0xff;
> + return 0;
> +}
> +
> +int
> +thinkpad_set_param(struct wsdisplay_param *dp)
> +{
> + struct acpithinkpad_softc *sc = acpithinkpad_cd.cd_devs[0];
> + int maxval = (sc->sc_brightness >> 8) & 0xff;
> +
> + if (sc == NULL)
> + return -1;
> +
> + switch (dp->param) {
> + case WSDISPLAYIO_PARAM_BRIGHTNESS:
> + if (dp->curval < 0)
> + dp->curval = 0;
> + if (dp->curval > maxval)
> + dp->curval = maxval;
> + sc->sc_brightness &= ~0xff;
> + sc->sc_brightness |= dp->curval;
> + acpi_addtask(sc->sc_acpi, thinkpad_set_brightness, sc, 0);
> + acpi_wakeup(sc->sc_acpi);
> + return 0;
> + default:
> + return -1;
> + }
> }
>
