On 23/01/20(Thu) 08:26, Patrick Wildt wrote:
> Hi,
>
> this is the second attempt of a diff that prepares acpivout(4) to work
> with the X395. The previous one broke due to recursive ACPI locking.
>
> So apparently we cannot change the brightness using the acpivout(4) ACPI
> methods. Instead we need to call amdgpu(4) to change the brightness.
> But the brightness key events are still reported by acpivout(4). That
> means that we need to seperate the keystroke events from the actual
> brightness change.
>
> This diff changes the code to always use ws_[gs]et_param instead of just
> calling the ACPI methods. This means that the function pointers can
> point to acpivout(4) or amdgpu(4), it shouldn't matter.
>
> Unfortunately there's an issue with acpivout(4) calling itself. The
> keystroke event handler runs with the ACPI lock, so if we call our own
> function, we try to grab the ACPI lock again and panic. So, in this
> diff I check whether we already have it and skip taking it again.
Why do you need to grab the ACPI lock again? Can't you assert the lock
is always taken and use a wrapper in the code path where it isn't?
> I'm sure this looks ugly, and I'm wondering if it's too ugly. If it is
> indeed too ugly, I hope you'll also provide another idea how we can work
> around it.
> More testing would be appreciated.
>
> Thanks,
> Patrick
>
> diff --git a/sys/dev/acpi/acpivout.c b/sys/dev/acpi/acpivout.c
> index 58e8e3d431c..f3de0c582fb 100644
> --- a/sys/dev/acpi/acpivout.c
> +++ b/sys/dev/acpi/acpivout.c
> @@ -66,6 +66,7 @@ void acpivout_brightness_cycle(struct acpivout_softc
> *);
> void acpivout_brightness_step(struct acpivout_softc *, int);
> void acpivout_brightness_zero(struct acpivout_softc *);
> int acpivout_get_brightness(struct acpivout_softc *);
> +int acpivout_select_brightness(struct acpivout_softc *, int);
> int acpivout_find_brightness(struct acpivout_softc *, int);
> void acpivout_set_brightness(struct acpivout_softc *, int);
> void acpivout_get_bcl(struct acpivout_softc *);
> @@ -124,6 +125,9 @@ acpivout_notify(struct aml_node *node, int notify, void
> *arg)
> {
> struct acpivout_softc *sc = arg;
>
> + if (ws_get_param == NULL || ws_set_param == NULL)
> + return (0);
> +
> switch (notify) {
> case NOTIFY_BRIGHTNESS_CYCLE:
> acpivout_brightness_cycle(sc);
> @@ -151,12 +155,13 @@ acpivout_notify(struct aml_node *node, int notify, void
> *arg)
> void
> acpivout_brightness_cycle(struct acpivout_softc *sc)
> {
> - int cur_level;
> + struct wsdisplay_param dp;
>
> - if (sc->sc_bcl_len == 0)
> + dp.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
> + if (ws_get_param(&dp))
> return;
> - cur_level = acpivout_get_brightness(sc);
> - if (cur_level == sc->sc_bcl[sc->sc_bcl_len - 1])
> +
> + if (dp.curval == dp.max)
> acpivout_brightness_zero(sc);
> else
> acpivout_brightness_step(sc, 1);
> @@ -165,33 +170,45 @@ acpivout_brightness_cycle(struct acpivout_softc *sc)
> void
> acpivout_brightness_step(struct acpivout_softc *sc, int dir)
> {
> - int level, nindex;
> + struct wsdisplay_param dp;
> + int delta, new;
>
> - if (sc->sc_bcl_len == 0)
> - return;
> - level = acpivout_get_brightness(sc);
> - if (level == -1)
> + dp.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
> + if (ws_get_param(&dp))
> return;
>
> - nindex = acpivout_find_brightness(sc, level + (dir * BRIGHTNESS_STEP));
> - if (sc->sc_bcl[nindex] == level) {
> - if (dir == 1 && (nindex + 1 < sc->sc_bcl_len))
> - nindex++;
> - else if (dir == -1 && (nindex - 1 >= 0))
> - nindex--;
> + new = dp.curval;
> + delta = ((dp.max - dp.min) * BRIGHTNESS_STEP) / 100;
> + if (dir > 0) {
> + if (delta > dp.max - dp.curval)
> + new = dp.max;
> + else
> + new += delta;
> + } else if (dir < 0) {
> + if (delta > dp.curval - dp.min)
> + new = dp.min;
> + else
> + new -= delta;
> }
> - if (sc->sc_bcl[nindex] == level)
> +
> + if (dp.curval == new)
> return;
>
> - acpivout_set_brightness(sc, sc->sc_bcl[nindex]);
> + dp.curval = new;
> + ws_set_param(&dp);
> }
>
> void
> acpivout_brightness_zero(struct acpivout_softc *sc)
> {
> - if (sc->sc_bcl_len == 0)
> + struct wsdisplay_param dp;
> +
> + dp.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
> + if (ws_get_param(&dp))
> return;
> - acpivout_set_brightness(sc, sc->sc_bcl[0]);
> +
> + dp.curval = dp.min;
> + ws_set_param(&dp);
> }
>
> int
> @@ -211,6 +228,26 @@ acpivout_get_brightness(struct acpivout_softc *sc)
> return (level);
> }
>
> +int
> +acpivout_select_brightness(struct acpivout_softc *sc, int nlevel)
> +{
> + int nindex, level;
> +
> + level = acpivout_get_brightness(sc);
> + if (level == -1)
> + return level;
> +
> + nindex = acpivout_find_brightness(sc, nlevel);
> + if (sc->sc_bcl[nindex] == level) {
> + if (nlevel > level && (nindex + 1 < sc->sc_bcl_len))
> + nindex++;
> + else if (nlevel < level && (nindex - 1 >= 0))
> + nindex--;
> + }
> +
> + return nindex;
> +}
> +
> int
> acpivout_find_brightness(struct acpivout_softc *sc, int level)
> {
> @@ -290,7 +327,7 @@ int
> acpivout_get_param(struct wsdisplay_param *dp)
> {
> struct acpivout_softc *sc = NULL;
> - int i;
> + int i, needlock;
>
> switch (dp->param) {
> case WSDISPLAYIO_PARAM_BRIGHTNESS:
> @@ -304,10 +341,13 @@ acpivout_get_param(struct wsdisplay_param *dp)
> }
> if (sc != NULL && sc->sc_bcl_len != 0) {
> dp->min = 0;
> - dp->max = sc->sc_bcl[sc->sc_bcl_len - 1];
> - rw_enter_write(&sc->sc_acpi->sc_lck);
> + dp->max = sc->sc_bcl[sc->sc_bcl_len - 1];
> + needlock = rw_status(&sc->sc_acpi->sc_lck) != RW_WRITE;
> + if (needlock)
> + rw_enter_write(&sc->sc_acpi->sc_lck);
> dp->curval = acpivout_get_brightness(sc);
> - rw_exit_write(&sc->sc_acpi->sc_lck);
> + if (needlock)
> + rw_exit_write(&sc->sc_acpi->sc_lck);
> if (dp->curval != -1)
> return 0;
> }
> @@ -321,7 +361,7 @@ int
> acpivout_set_param(struct wsdisplay_param *dp)
> {
> struct acpivout_softc *sc = NULL;
> - int i, nindex;
> + int i, nindex, needlock;
>
> switch (dp->param) {
> case WSDISPLAYIO_PARAM_BRIGHTNESS:
> @@ -334,10 +374,14 @@ acpivout_set_param(struct wsdisplay_param *dp)
> break;
> }
> if (sc != NULL && sc->sc_bcl_len != 0) {
> - rw_enter_write(&sc->sc_acpi->sc_lck);
> - nindex = acpivout_find_brightness(sc, dp->curval);
> - acpivout_set_brightness(sc, sc->sc_bcl[nindex]);
> - rw_exit_write(&sc->sc_acpi->sc_lck);
> + needlock = rw_status(&sc->sc_acpi->sc_lck) != RW_WRITE;
> + if (needlock)
> + rw_enter_write(&sc->sc_acpi->sc_lck);
> + nindex = acpivout_select_brightness(sc, dp->curval);
> + if (nindex != -1)
> + acpivout_set_brightness(sc, sc->sc_bcl[nindex]);
> + if (needlock)
> + rw_exit_write(&sc->sc_acpi->sc_lck);
> return 0;
> }
> return -1;
>
