Re: acpivout(4): directly call ws_[gs]et_param
On Fri, Jan 24, 2020 at 01:04:41AM +0100, Patrick Wildt wrote:
> On Thu, Jan 23, 2020 at 11:25:51PM +0100, Mark Kettenis wrote:
> > Since acpi_set_brightness() can be called from anywhere, it should really
> > use the acpi task queue to do its thing instead of calling AML directly.
>
> I didn't know there's an acpi task queue, so that's awesome! I just saw
> that acpithinkpad(4) also uses that. Changing this diff to do it that
> way was also quite easy, so this should read much better. Feel free to
> give it a go.
>
> ok?
>
> Patrick
Looking for some more testing on this. Successful test results would
help to get this diff in, so I can start sending the next diffs to
finally enable X395 backlight keys support.
Expected behaviour: Backlight keys continue to change the backlight
as well as they did before.
Machines: All those where acpivout(4) attaches.
Patrick
> diff --git a/sys/dev/acpi/acpivout.c b/sys/dev/acpi/acpivout.c
> index 58e8e3d431c..89922f4723b 100644
> --- a/sys/dev/acpi/acpivout.c
> +++ b/sys/dev/acpi/acpivout.c
> @@ -60,14 +60,17 @@ struct acpivout_softc {
>
> int *sc_bcl;
> size_t sc_bcl_len;
> +
> + int sc_brightness;
> };
>
> 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_set_brightness(void *, int);
> void acpivout_get_bcl(struct acpivout_softc *);
>
> /* wconsole hook functions */
> @@ -117,6 +120,8 @@ acpivout_attach(struct device *parent, struct device
> *self, void *aux)
> ws_set_param = acpivout_set_param;
>
> acpivout_get_bcl(sc);
> +
> + sc->sc_brightness = acpivout_get_brightness(sc);
> }
>
> int
> @@ -124,6 +129,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 +159,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 +174,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 +232,26 @@ acpivout_get_brightness(struct acpivout_softc *sc)
> return (level);
> }
>
> +int
> +acpivout_select_brightness(struct acpivout_softc *sc, int nlevel)
> +{
> + int nindex,
Re: acpivout(4): directly call ws_[gs]et_param
On Thu, Jan 23, 2020 at 11:25:51PM +0100, Mark Kettenis wrote:
> Since acpi_set_brightness() can be called from anywhere, it should really
> use the acpi task queue to do its thing instead of calling AML directly.
I didn't know there's an acpi task queue, so that's awesome! I just saw
that acpithinkpad(4) also uses that. Changing this diff to do it that
way was also quite easy, so this should read much better. Feel free to
give it a go.
ok?
Patrick
diff --git a/sys/dev/acpi/acpivout.c b/sys/dev/acpi/acpivout.c
index 58e8e3d431c..89922f4723b 100644
--- a/sys/dev/acpi/acpivout.c
+++ b/sys/dev/acpi/acpivout.c
@@ -60,14 +60,17 @@ struct acpivout_softc {
int *sc_bcl;
size_t sc_bcl_len;
+
+ int sc_brightness;
};
void acpivout_brightness_cycle(struct acpivout_softc *);
void acpivout_brightness_step(struct acpivout_softc *, int);
void acpivout_brightness_zero(struct acpivout_softc *);
intacpivout_get_brightness(struct acpivout_softc *);
+intacpivout_select_brightness(struct acpivout_softc *, int);
intacpivout_find_brightness(struct acpivout_softc *, int);
-void acpivout_set_brightness(struct acpivout_softc *, int);
+void acpivout_set_brightness(void *, int);
void acpivout_get_bcl(struct acpivout_softc *);
/* wconsole hook functions */
@@ -117,6 +120,8 @@ acpivout_attach(struct device *parent, struct device *self,
void *aux)
ws_set_param = acpivout_set_param;
acpivout_get_bcl(sc);
+
+ sc->sc_brightness = acpivout_get_brightness(sc);
}
int
@@ -124,6 +129,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 +159,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 +174,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 +232,26 @@ acpivout_get_brightness(struct acpivout_softc *sc)
return (level);
}
+int
+acpivout_select_brightness(struct acpivout_softc *sc, int nlevel)
+{
+ int nindex, level;
+
+ level = sc->sc_brightness;
+ 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)
{
@@ -230,15 +271,16 @@ a
Re: acpivout(4): directly call ws_[gs]et_param
Patrick Wildt schreef op 2020-01-23 08:26:
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.
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.
Since acpi_set_brightness() can be called from anywhere, it should
really
use the acpi task queue to do its thing instead of calling AML directly.
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 *);
intacpivout_get_brightness(struct acpivout_softc *);
+intacpivout_select_brightness(struct acpivout_softc *, int);
intacpivout_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 l
Re: acpivout(4): directly call ws_[gs]et_param
On Thu, Jan 23, 2020 at 05:03:01PM -0500, Ted Unangst wrote: > Patrick Wildt wrote: > > acpivout_[gs]et_param don't know if they are being called by itself > > or by someone else. I don't need to grab it again, I just need to > > have it before calling an ACPI method. But when acpivout(4) calls > > itself, it already has it, so taking it again would break it, as it's > > non recursive. > > > > Since it's a global function pointer that hides the implementation, the > > caller cannot just take the ACPI lock before calling ws_[gs]et_param. > > The caller doesn't know that the implementation needs ACPI. On my X395 > > the function set in ws_[gs]et_param have nothing to do with ACPI at all. > > Can you release the lock before calling ws_set_param? That's a good point, that would make it a lot easier. I'm not sure, maybe kettenis@ can shed a light on this?
Re: acpivout(4): directly call ws_[gs]et_param
Patrick Wildt wrote: > acpivout_[gs]et_param don't know if they are being called by itself > or by someone else. I don't need to grab it again, I just need to > have it before calling an ACPI method. But when acpivout(4) calls > itself, it already has it, so taking it again would break it, as it's > non recursive. > > Since it's a global function pointer that hides the implementation, the > caller cannot just take the ACPI lock before calling ws_[gs]et_param. > The caller doesn't know that the implementation needs ACPI. On my X395 > the function set in ws_[gs]et_param have nothing to do with ACPI at all. Can you release the lock before calling ws_set_param?
Re: acpivout(4): directly call ws_[gs]et_param
On Thu, Jan 23, 2020 at 09:51:11AM +0100, Martin Pieuchot wrote:
> 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?
acpivout_[gs]et_param don't know if they are being called by itself
or by someone else. I don't need to grab it again, I just need to
have it before calling an ACPI method. But when acpivout(4) calls
itself, it already has it, so taking it again would break it, as it's
non recursive.
Since it's a global function pointer that hides the implementation, the
caller cannot just take the ACPI lock before calling ws_[gs]et_param.
The caller doesn't know that the implementation needs ACPI. On my X395
the function set in ws_[gs]et_param have nothing to do with ACPI at all.
> > 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 *);
> > intacpivout_get_brightness(struct acpivout_softc *);
> > +intacpivout_select_brightness(struct acpivout_softc *, int);
> > intacpivout_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
> > +
Re: acpivout(4): directly call ws_[gs]et_param
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 @@ voidacpivout_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
acpivout(4): directly call ws_[gs]et_param
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.
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 *);
intacpivout_get_brightness(struct acpivout_softc *);
+intacpivout_select_brightness(struct acpivout_softc *, int);
intacpivout_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))
+
