Laptop - special function keys - ACPI

Dear community,
I have a Lenovo Laptop T470 which runs FreeBSD-13,2p3-RELEASE. The device has a row of function keys F1 to F12. With an additional simultaneous Fn key press they provide special functions as XF86AudioRaiseVolume, XF86MonBrightnessDown, XF86Favorites and more. The default FreeBSD setup supports the XF86Audio* keys but nothing more. With xev(1) no reaction on the other special function keys can be detected. On Debian Bookworm all keys are supported, I think due to a thinkpad_acpi.ko module which is not available in the FreeBSD-13.2p3-RELEASE setup.

I have found information about a acpi_ibm module which does not change the behaviour. Loading acpi_video enables the XF86MonBrightnessUp and XF86MonBrightnessDown keys. In contrast to the XF86Audio* keys they trigger the backlight control somehow by incrementing or decrementing the brightness in steps of one.

Currently I use the window manager x11-wm/awesome with lua scripts to react on the special function keys. In case of the brightness it is unfortunate if it is controlled by awesome lua scripts and in parallel by other software. It does not work well. I have not found out how the brightness keys trigger the activity of the backlight control. This ends up in a few questions.

Is the activity of the special function keys configurable or is it hard coded? For example in acpi_video.ko?
If it can be configured somehow, how to do so?
Is there any other option to enable the special function keys?

I can map functions as the brightness control to other keys as F11 or F12. On a different laptop I have misused XF86AudioPrev and XF86AudioNext. But it would be nice to have the same key assignment on all installations.

Kind regards,
Christoph
 
  • Like
Reactions: mro
Always check the man pages. :-) As far as I know, acpi_video.ko only deals with screen brightness. acpi_ibm.ko supports some thinkpad function keys, I think. There is no "generic acpi function key kernel module".
 
Hi

Try to enable the keys with hw.usb.usbhid.enable=1 in /boot/loader.conf
Many things for desktops has to be configred manually. But then just works for many years :)
 
Always check the man pages. :) As far as I know, acpi_video.ko only deals with screen brightness. acpi_ibm.ko supports some thinkpad function keys, I think. There is no "generic acpi function key kernel module".
Yes. According to acpi_ibm(4) many keys should work. The default event mask enables all special keys but only the three audio related keys create an event. There are settings which can be controlled by sysctl(8). With writing I can only mute or unmute but nothing else.

Try to enable the keys with hw.usb.usbhid.enable=1 in /boot/loader.conf
That did not change the situation, too. At a late stage of the boot process usbhid(3) is auto-loaded anyhow.

It is no drama if the almost all special function keys do not work. I can go through the modules reported by kldstat. May be I will find some hook. The issue can also be related to the laptop. It did not work with FreeBSD-12.x-RELEASE, it has thrown a flood of ACPI error messages. I am already happy the FreeBSD-13.2-RELEASE runs without any real issues.
 
Yes. According to acpi_ibm(4) many keys should work. The default event mask enables all special keys but only the three audio related keys create an event. There are settings which can be controlled by sysctl(8). With writing I can only mute or unmute but nothing else.


That did not change the situation, too. At a late stage of the boot process usbhid(3) is auto-loaded anyhow.

It is no drama if the almost all special function keys do not work. I can go through the modules reported by kldstat. May be I will find some hook. The issue can also be related to the laptop. It did not work with FreeBSD-12.x-RELEASE, it has thrown a flood of ACPI error messages. I am already happy the FreeBSD-13.2-RELEASE runs without any real issues.
On my system multimedia buttons, does not work without. So no automatic loading of key functions on my system. And yes FreeBSD is slow to adapt to new hardware. But when it over time works it is a very good system.
 
I have only old hardware, partly bought as refurbished hardware, partly from family members or friends because the computer is getting slower and slower over time. There is no need to mention the OS. With the issues of the T470 thinkpad I have got an impression about Debian. Startup is ultra fast and everything worked already out of the box. Systemd is not as bad as I thought, it works well and is just different to other systems. But it takes a lot of tweaking to get things tidy. WIFI might perform better, but for normal use cases the bottleneck is not the WLAN but the internet. FreeBSD is much more structured and tidy. The documentation is very good and consistent. And so on and so on :-). In total one can expect less "surprises" running FreeBSD.
 
I have found information about a acpi_ibm module which does not change the behaviour. Loading acpi_video enables the XF86MonBrightnessUp and XF86MonBrightnessDown keys. In contrast to the XF86Audio* keys they trigger the backlight control somehow by incrementing or decrementing the brightness in steps of one.
Now I have installed the kernel sources and compiled and installed the generic FreeBSD-13.2-RELEASE, just to be on the safe side. Then I have modified acpi_video.c which now supports an additional by sysctl tunable step size. The default step size remains 1 as it is in the original code. The modification is as below.
Code:
diff --git a/sys/dev/acpica/acpi_video.c b/sys/dev/acpica/acpi_video.c
index cbe31d3a66c2..442304b589a0 100644
--- a/sys/dev/acpica/acpi_video.c
+++ b/sys/dev/acpica/acpi_video.c
@@ -64,6 +64,7 @@ struct acpi_video_output {
    int     vo_brightness;
    int     vo_fullpower;
    int     vo_economy;
+   int     vo_step;
    int     vo_numlevels;
    int     *vo_levels;
    struct sysctl_ctx_list vo_sysctl_ctx;
@@ -102,6 +103,7 @@ static void acpi_video_vo_destroy(struct acpi_video_output *);
 static int acpi_video_vo_check_level(struct acpi_video_output *, int);
 static void    acpi_video_vo_notify_handler(ACPI_HANDLE, UINT32, void *);
 static int acpi_video_vo_active_sysctl(SYSCTL_HANDLER_ARGS);
+static int acpi_video_vo_step_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_bright_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_presets_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_levels_sysctl(SYSCTL_HANDLER_ARGS);
@@ -626,6 +628,7 @@ acpi_video_vo_init(UINT32 adr)
        vo->vo_brightness = -1;
        vo->vo_fullpower = -1;  /* TODO: override with tunables */
        vo->vo_economy = -1;
+       vo->vo_step = 1;
        vo->vo_numlevels = 0;
        vo->vo_levels = NULL;
        snprintf(env, sizeof(env), "hw.acpi.video.%s.fullpower", name);
@@ -634,6 +637,9 @@ acpi_video_vo_init(UINT32 adr)
        snprintf(env, sizeof(env), "hw.acpi.video.%s.economy", name);
        if (getenv_int(env, &x))
            vo->vo_economy = x;
+       snprintf(env, sizeof(env), "hw.acpi.video.%s.step", name);
+       if (getenv_int(env, &x))
+           vo->vo_step = x;

        sysctl_ctx_init(&vo->vo_sysctl_ctx);
        if (vp != NULL)
@@ -673,6 +679,12 @@ acpi_video_vo_init(UINT32 adr)
                POWER_PROFILE_ECONOMY,
                acpi_video_vo_presets_sysctl, "I",
                "preset level for economy mode");
+           SYSCTL_ADD_PROC(&vo->vo_sysctl_ctx,
+               SYSCTL_CHILDREN(vo->vo_sysctl_tree),
+               OID_AUTO, "step",
+               CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_MPSAFE, vo,
+               0, acpi_video_vo_step_sysctl, "I",
+               "increment decrement step size");
            SYSCTL_ADD_PROC(&vo->vo_sysctl_ctx,
                SYSCTL_CHILDREN(vo->vo_sysctl_tree),
                OID_AUTO, "levels",
@@ -826,7 +838,7 @@ acpi_video_vo_notify_handler(ACPI_HANDLE handle, UINT32 notify, void *context)
        break;
    case VID_NOTIFY_INC_BRN:
    case VID_NOTIFY_DEC_BRN:
-       for (i = 0; i < vo->vo_numlevels; i++) {
+       for (i = 0; i < vo->vo_numlevels; i=i+vo->vo_step) {
            j = vo->vo_levels[i];
            if (notify == VID_NOTIFY_INC_BRN) {
                if (j > level &&
@@ -887,6 +899,37 @@ acpi_video_vo_active_sysctl(SYSCTL_HANDLER_ARGS)
    return (err);
 }

+
+static int
+acpi_video_vo_step_sysctl(SYSCTL_HANDLER_ARGS)
+{
+   struct acpi_video_output *vo;
+   int stepsize, err;
+
+   vo = (struct acpi_video_output *)arg1;
+   ACPI_SERIAL_BEGIN(video_output);
+   if (vo->handle == NULL) {
+       err = ENXIO;
+       goto out;
+   }
+
+   stepsize = vo->vo_step;
+
+   err = sysctl_handle_int(oidp, &stepsize, 0, req);
+   if (err != 0 || req->newptr == NULL)
+       goto out;
+   if (stepsize < -1 || stepsize > 100) {
+       err = EINVAL;
+       goto out;
+   }
+
+   vo->vo_step = stepsize;
+
+out:
+   ACPI_SERIAL_END(video_output);
+   return (err);
+}
+
 /* ARGSUSED */
 static int
 acpi_video_vo_bright_sysctl(SYSCTL_HANDLER_ARGS)
The parameter seen by sysctl are
Code:
  % sysctl hw.acpi.video
hw.acpi.video.lcd0.levels: 100 100 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100
hw.acpi.video.lcd0.step: 10
hw.acpi.video.lcd0.economy: 60
hw.acpi.video.lcd0.fullpower: 70
hw.acpi.video.lcd0.brightness: 70
hw.acpi.video.lcd0.active: 0
The parameter step has been added and can be preset in /etc/sysctl.conf or modified by the superuser. If someone would like to try it I can also provide the compiled module acpi_video.ko.
 
Then I have modified acpi_video.c which now supports an additional by sysctl tunable step size. The default step size remains 1 as it is in the original code. The modification is as below.

Cool. Couple of points (excuse format loss):

+ if (stepsize < -1 || stepsize > 100) {
+ err = EINVAL;
+ goto out;

Any stepsize < 1 must be invalid.
I suggest <= 20 as a useful limit; even 20 only allows 6 steps.

case VID_NOTIFY_INC_BRN:
case VID_NOTIFY_DEC_BRN:
- for (i = 0; i < vo->vo_numlevels; i++) {
+ for (i = 0; i < vo->vo_numlevels; i=i+vo->vo_step) {
j = vo->vo_levels[i];

Before referencing vo->vo_levels[i] the new value of i needs testing with
acpi_video_vo_check_level() as it may now exceed 100.

cheers
 
Cool. Couple of points (excuse format loss):


Any stepsize < 1 must be invalid.
I suggest <= 20 as a useful limit; even 20 only allows 6 steps.
Ups, I am too stupid to quote the code block. This is true. But the first intention has been to be as close to the original. In my opinion a minimum step size of 1 is also not practical.
Before referencing vo->vo_levels the new value of i needs testing with
acpi_video_vo_check_level() as it may now exceed 100.
This is done in the code block which actually updates the level. But there are more things which are not critical for the operation but which are strange. For example the decrement is triggered (I think normally 2x) even before the configuration parameter of sysctl are read out. This means if the initial brightness is for example 70 it ends up at 68 without any interaction by the user. This does not hurt but it is not tidy.
Additionally I am not sure why a construct as the list of levels has been chosen. In my opinion it makes things more complicated without any obvious reason. May be it has been foreseen to compensate a non-linear behaviour of the brightness impression by the user and the brightness setting.

Do you know how to print debug information to the console or a log file? Then I might dig deeper. Trial and error is quite time consuming.

About my personal qualification, there is almost nothing. I did tiny stuff in C by myself. But I am not too bad in copy and modify existing stuff. Or if there is an existing skeleton. Therefore it can easily be that I am wrong with any statements :-).
 
But the first intention has been to be as close to the original. In my opinion a minimum step size of 1 is also not practical.

Sure, and people have used devd scripts to choose larger steps, but re your code my point was:
Any stepsize < 1 must be invalid.

You currently allow -1 (?) and 0 (causing an infinite loop) so check minimum >= 1.

Before referencing vo->vo_levels[i] the new value of i needs testing with
acpi_video_vo_check_level() as it may now exceed 100.
This is done in the code block which actually updates the level.

Too late. If i increases to (say) 109 from 99, vo->vo_levels[109] is out of bounds and could be any value, so i needs validating before accessing the array.

But there are more things which are not critical for the operation but which are strange. For example the decrement is triggered (I think normally 2x) even before the configuration parameter of sysctl are read out. This means if the initial brightness is for example 70 it ends up at 68 without any interaction by the user. This does not hurt but it is not tidy.

Yes I've seen that behaviour.

Additionally I am not sure why a construct as the list of levels has been chosen. In my opinion it makes things more complicated without any obvious reason. May be it has been foreseen to compensate a non-linear behaviour of the brightness impression by the user and the brightness setting.

Or simply to list all possible values? I agree it's suboptimal but doubt changing that now would survive a code review.

Do you know how to print debug information to the console or a log file? Then I might dig deeper. Trial and error is quite time consuming.

Others will help I'm sure, but I'm a pascal programmer. C is readonly here, though I've been rendering C into pascal since Chuck Forsburg's YAM ;)

About my personal qualification, there is almost nothing. I did tiny stuff in C by myself. But I am not too bad in copy and modify existing stuff. Or if there is an existing skeleton. Therefore it can easily be that I am wrong with any statements :).

Just watch those array bounds ;)
 
Code:
case VID_NOTIFY_INC_BRN:
case VID_NOTIFY_DEC_BRN:
-    for (i = 0; i < vo->vo_numlevels; i++) {
+   for (i = 0; i < vo->vo_numlevels; i=i+vo->vo_step) { 
       j = vo->vo_levels[i];
Before referencing vo->vo_levels the new value of i needs testing with
acpi_video_vo_check_level() as it may now exceed 100.

Now I got the point. There is already a problem at that stage. And sure, the step size should be at least 1. By the way, here is a part of the initialisation.
Code:
...
        vo->vo_level = -1;
        vo->vo_brightness = -1;
        vo->vo_fullpower = -1;  /* TODO: override with tunables */
        vo->vo_economy = -1;
        vo->vo_step = 1;
        vo->vo_numlevels = 0;
        vo->vo_levels = NULL;
...
Many parameter have invalid initial values. Of course I started with vo->vo_step = -1, too. Things still worked in X but when pressing a brightness button in the console the software has crashed.

May be this could be a hook to prevent the unintentional triggering at startup. I initialise vo-vo_step = -1 and catch invalid step size before entering the for loop. About the debug messages I have seem printf in the code. May be it is a simple as that. This is easy to try out.
Others will help I'm sure, but I'm a pascal programmer. C is readonly here, though I've been rendering C into pascal since Chuck Forsburg's YAM ;)
You have helped me already a lot! I am also almost C readonly, the most useful project has been interfacing Tcl code to make use of fast Fourier transforms which has been available in C code. The second useful project has been an optimizer for circuit simulation using spice as the computing engine. The C part has been in reading the output file of spice, evaluation the result compared to the design targets and then providing a modified circuit description file for the next iteration. The main purpose has been to learn C a little bit. I really have no idea about tools as automake, autoconf and so on. That is a huge blind spot.
Thank you again,
Christoph
 
About the debug messages I have seem printf in the code. May be it is a simple as that. This is easy to try out.
It is as simple at that. In case of the particular module I am investigating printf prints to the console. But even better the messages appear as kern.crit in the /var/log/whatever-is-configured. This is very convenient.
 
It is as simple at that. In case of the particular module I am investigating printf prints to the console. But even better the messages appear as kern.crit in the /var/log/whatever-is-configured. This is very convenient.

That's great Christoph. verbose_loading="YES" in loader.conf may help too.

BTW those -1 values set on init indicate uninitialised rather than invalid. At 13.2-R there are 24 instances of '-1' in acpi_video.c; following them you should find what else is needed.

From your first post, however, you might like to check out
https://www.davidschlachter.com/misc/freebsd-acpi_video-thinkpad-display-brightness

for an easier path to getting where you want sooner - not to deter your learning experience with C device code!

On my T430s the brightness steps there don't work well - likely different LCDs need different profiles - so I'm still experimenting, easier for me as a sh script run from devd.

cheers, Ian
 
For example the decrement is triggered (I think normally 2x) even before the configuration parameter of sysctl are read out. This means if the initial brightness is for example 70 it ends up at 68 without any interaction by the user.
Now I think that the root cause is in the evaluation of vo->vo_levels. The first to integer might present the maximum level and the number of steps.
I have checked
Code:
+   for (i = 0; i < vo->vo_numlevels; i=i+vo->vo_step) {
       j = vo->vo_levels[i];
This does not violate the range of level because the loop counter i does not exceed vo->vo_numlevels.
From your first post, however, you might like to check out
https://www.davidschlachter.com/misc/freebsd-acpi_video-thinkpad-display-brightness

for an easier path to getting where you want sooner - not to deter your learning experience with C device code!
This is very useful. I have only little experience using devd. I have also thought about changing the list vo->vo.levels[] but failed in the first steps. Additionally I have to find out the exact meaning of all parameter. Regarding the current procedure I think about running the for loop in steps of 1 and to modify the next_level by adding the offset there. When I seek the best match of the actual level + the steps size I can also handle the corner cases of maximum and minimum level. Additionally it finds the best match if the list of levels is not as simple as in steps of 1.

I found also one additional phenomena. There is a tool backlight to query and to set the brightness. When setting the brightness via acpi_video backlight returns that value but with some little level depending offset. When setting the brightness using backlight the read out via acpi_video does not change. This means that there are shadow registers involved, too.
 
About the level specification I have found https://uefi.org/specs/ACPI/6.4_A/Apx_B_Video_Extensions.html#acpi-namespace
Code:
Each brightness level is represented by a number between 0 and 100, 
and can be thought of as a percentage. For example, 50 can be 50% 
power consumption or 50% brightness, as defined by the OEM.
Code:
The first number in the package is the level of the panel when full 
power is connected to the machine. The second number in the 
package is the level of the panel when the machine is on batteries. 
All other numbers are treated as a list of levels OSPM will cycle
 through when the user toggles (via a keystroke) the brightness 
level of the display.
That information makes it simple. The exact list comes from the display maker but the range is 0...100.
 
  • Thanks
Reactions: mro
Now there is an update of the part which handles the decrement/increment. It takes into account that the allowed levels are not just 0...100 in steps of 1. The next level is at least one step apart from the current level. The candidate of the list of levels with the lowest offset to the target is taken.
Code:
  acpi_video_vo_notify_handler(ACPI_HANDLE handle, UINT32 notify, void *context)
 {
    struct acpi_video_output *vo;
-   int i, j, level, new_level;
+   int i, j, level, new_level, ofs_level;

    vo = context;
    ACPI_SERIAL_BEGIN(video_output);
@@ -826,16 +838,33 @@ acpi_video_vo_notify_handler(ACPI_HANDLE handle, UINT32 notify, void *context)
        break;
    case VID_NOTIFY_INC_BRN:
    case VID_NOTIFY_DEC_BRN:
-       for (i = 0; i < vo->vo_numlevels; i++) {
+       // The range of the loop is 0...103. The first and the second index of vo_levels[i] are 100
+       // followed by integers 0, 1, 2, ...98, 99, 100.
+       // The index i is in the range of 2...102.
+       //           j is in the range of vo->vo_numlevels without the first two entries.
+       // See https://uefi.org/specs/ACPI/6.4_A/Apx_B_Video_Extensions.html#acpi-namespace
+       // In case the list vo->vo_levels has a high step size increment and decrement
+       // should be at least the step size. E.g. with a level of 50, a step size of 5 and a list
+       // as ..., 48, 60, ... one tune increment should not fall back to 48.
+       // This means the evaluation starts at 48+5=53. Lower levels are ignored.
+       // The min and max functions allows to include the bounds of 0 and 100.
+       ofs_level = 100;
+       for (i = 2; i < vo->vo_numlevels; i++) {
            j = vo->vo_levels[i];
            if (notify == VID_NOTIFY_INC_BRN) {
-               if (j > level &&
-                   (j < new_level || level == new_level))
-                   new_level = j;
+               if (j >= min(level + vo->vo_step, 100) &&
+                   abs(j-(level+vo->vo_step)) <= ofs_level)
+                   {
+                       new_level = j;
+                       ofs_level = abs(j-(level+vo->vo_step));
+               }
            } else {
-               if (j < level &&
-                   (j > new_level || level == new_level))
-                   new_level = j;
+               if (j <= max(level - vo->vo_step, 0) &&
+                   abs(j-(level-vo->vo_step)) <= ofs_level)
+                   {
+                       new_level = j;
+                       ofs_level = abs(j-(level-vo->vo_step));
+               }
            }
        }
        break;
@@ -887,6 +916,37 @@ acpi_video_vo_active_sysctl(SYSCTL_HANDLER_ARGS)
    return (err);
 }
A different approach would be to generate a smaller list of levels. This list must be a subset of the original list. Otherwise an update of the brightness might fail. The effort to generate such list is similar to the search as above. Therefore it does not make sense.
 
For example the decrement is triggered (I think normally 2x) even before the configuration parameter of sysctl are read out. This means if the initial brightness is for example 70 it ends up at 68 without any interaction by the user. This does not hurt but it is not tidy.
Now I think that the root cause is in the evaluation of vo->vo_levels. The first to integer might present the maximum level and the number of steps.

No, you quoted it later from the description of the ACPI package: the first two are preset fullpower and economy levels. These can only be modified in loader.conf (see TODO), not later by sysctl.

I have checked
Code:
+   for (i = 0; i < vo->vo_numlevels; i=i+vo->vo_step) {
       j = vo->vo_levels[i];
This does not violate the range of level because the loop counter i does not exceed vo->vo_numlevels.

Oops, you are correct. [lame] I told you C was readonly here, but then I misread :( [/lame]

This is very useful. I have only little experience using devd. I have also thought about changing the list vo->vo.levels[] but failed in the first steps. Additionally I have to find out the exact meaning of all parameter.

I'll leave it here, seeing you've just posted an update, except:

Additionally it finds the best match if the list of levels is not as simple as in steps of 1.

My Thinkpad X200 has working brightness keys (FnHome/End) but loading acpi_video, I find hw.acpi.video.lcd0.levels:
100 100 20 25 30 35 40 45 50 55 60 65 70 75 80 85 90 100
i.e. 16 levels, both presets 100%.
 

Thanks, useful link.

Code:
Each brightness level is represented by a number between 0 and 100,
and can be thought of as a percentage. For example, 50 can be 50%
power consumption or 50% brightness, as defined by the OEM.

Ok.

Code:
The first number in the package is the level of the panel when full
power is connected to the machine. The second number in the
package is the level of the panel when the machine is on batteries.
All other numbers are treated as a list of levels OSPM will cycle
 through when the user toggles (via a keystroke) the brightness
level of the display.
That information makes it simple. The exact list comes from the display maker but the range is 0...100.

Exactly, so you can't modify that array, unless working from a copy I guess; I see you've moved on ...
 
Now there is an update of the part which handles the decrement/increment. It takes into account that the allowed levels are not just 0...100 in steps of 1. The next level is at least one step apart from the current level. The candidate of the list of levels with the lowest offset to the target is taken.

Noting my X200's set of levels as one example of something other than the 0..100 of yours or my T430s, we can't rely on any assumptions. E.g. note 95 isn't in X200's list, nor any value <20.

I'd best not comment on your C.

Anyway, good luck and have fun!
 
Exactly, so you can't modify that array, unless working from a copy I guess; I see you've moved on ...
From acpi_video I can even change the array. This is of course nothing one should do. Who knows which services rely on that table. One thing I have to do is to try some test tables with values different to the trivial case which I get from my laptop.
 
My Thinkpad X200 has working brightness keys (FnHome/End) but loading acpi_video, I find hw.acpi.video.lcd0.levels:
100 100 20 25 30 35 40 45 50 55 60 65 70 75 80 85 90 100
i.e. 16 levels, both presets 100%.
This is a sane configuration, no need to modify anything.
I'd best not comment on your C.
I really appreciate any hint to improve. If you have a comment which might not be true at the end, it is still better to double check. In best case the code is improved and all parties have learned. Now comes the complete patch. Later I will contact the author and ask for his opinion.
Code:
diff --git a/sys/dev/acpica/acpi_video.c b/sys/dev/acpica/acpi_video.c
index cbe31d3a66c2..03990917d24d 100644
--- a/sys/dev/acpica/acpi_video.c
+++ b/sys/dev/acpica/acpi_video.c
@@ -64,6 +64,7 @@ struct acpi_video_output {
    int     vo_brightness;
    int     vo_fullpower;
    int     vo_economy;
+   int     vo_step;
    int     vo_numlevels;
    int     *vo_levels;
    struct sysctl_ctx_list vo_sysctl_ctx;
@@ -102,6 +103,7 @@ static void acpi_video_vo_destroy(struct acpi_video_output *);
 static int acpi_video_vo_check_level(struct acpi_video_output *, int);
 static void    acpi_video_vo_notify_handler(ACPI_HANDLE, UINT32, void *);
 static int acpi_video_vo_active_sysctl(SYSCTL_HANDLER_ARGS);
+static int acpi_video_vo_step_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_bright_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_presets_sysctl(SYSCTL_HANDLER_ARGS);
 static int acpi_video_vo_levels_sysctl(SYSCTL_HANDLER_ARGS);
@@ -626,6 +628,7 @@ acpi_video_vo_init(UINT32 adr)
        vo->vo_brightness = -1;
        vo->vo_fullpower = -1;  /* TODO: override with tunables */
        vo->vo_economy = -1;
+       vo->vo_step = 1;
        vo->vo_numlevels = 0;
        vo->vo_levels = NULL;
        snprintf(env, sizeof(env), "hw.acpi.video.%s.fullpower", name);
@@ -634,6 +637,9 @@ acpi_video_vo_init(UINT32 adr)
        snprintf(env, sizeof(env), "hw.acpi.video.%s.economy", name);
        if (getenv_int(env, &x))
            vo->vo_economy = x;
+       snprintf(env, sizeof(env), "hw.acpi.video.%s.step", name);
+       if (getenv_int(env, &x))
+           vo->vo_step = x;

        sysctl_ctx_init(&vo->vo_sysctl_ctx);
        if (vp != NULL)
@@ -673,6 +679,12 @@ acpi_video_vo_init(UINT32 adr)
                POWER_PROFILE_ECONOMY,
                acpi_video_vo_presets_sysctl, "I",
                "preset level for economy mode");
+           SYSCTL_ADD_PROC(&vo->vo_sysctl_ctx,
+               SYSCTL_CHILDREN(vo->vo_sysctl_tree),
+               OID_AUTO, "step",
+               CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_MPSAFE, vo,
+               0, acpi_video_vo_step_sysctl, "I",
+               "increment decrement step size");
            SYSCTL_ADD_PROC(&vo->vo_sysctl_ctx,
                SYSCTL_CHILDREN(vo->vo_sysctl_tree),
                OID_AUTO, "levels",
@@ -786,7 +798,7 @@ static void
 acpi_video_vo_notify_handler(ACPI_HANDLE handle, UINT32 notify, void *context)
 {
    struct acpi_video_output *vo;
-   int i, j, level, new_level;
+   int i, j, level, new_level, ofs_level;

    vo = context;
    ACPI_SERIAL_BEGIN(video_output);
@@ -826,16 +838,38 @@ acpi_video_vo_notify_handler(ACPI_HANDLE handle, UINT32 notify, void *context)
        break;
    case VID_NOTIFY_INC_BRN:
    case VID_NOTIFY_DEC_BRN:
-       for (i = 0; i < vo->vo_numlevels; i++) {
+       // The range of the loop is 0...103. The first and the second index of vo_levels[i] are 100
+       // followed by integers 0, 1, 2, ...98, 99, 100.
+       // The index i is in the range of 2...102.
+       //           j is in the range of vo->vo_numlevels without the first two entries.
+       // See https://uefi.org/specs/ACPI/6.4_A/Apx_B_Video_Extensions.html#acpi-namespace
+       // Decrement and increment are at least one step size. Then the best match of the
+       // next level target in the level grid is taken. This results in a step which is
+       // at least almost the step size. It can be lower, too.
+       // Example: levels ...50, 60, 70... with 50 as the actual level and increment:
+       // a step size of 10, 11, 12, 13, 14, 15 result in a next level of 60.
+       // a step size of 16, 17, 18, 19, 20 result in a next level of 70.
+       // The comparison abs(j-(level+vo->vo_step)) with ofs_level differns in direction
+       // because the level is incremented to brightness up and down. In a case as the example
+       // levels above and a step of 15 this takes care that the level next to the actual level
+       // is taken as the best matching candidate.
+       ofs_level = 100;
+       for (i = 2; i < vo->vo_numlevels; i++) {
            j = vo->vo_levels[i];
            if (notify == VID_NOTIFY_INC_BRN) {
                if (j > level &&
-                   (j < new_level || level == new_level))
-                   new_level = j;
+                   abs(j-(level+vo->vo_step)) < ofs_level)
+                   {
+                       new_level = j;
+                       ofs_level = abs(j-(level+vo->vo_step));
+               }
            } else {
                if (j < level &&
-                   (j > new_level || level == new_level))
-                   new_level = j;
+                   abs(j-(level-vo->vo_step)) <= ofs_level)
+                   {
+                       new_level = j;
+                       ofs_level = abs(j-(level-vo->vo_step));
+               }
            }
        }
        break;
@@ -887,6 +921,36 @@ acpi_video_vo_active_sysctl(SYSCTL_HANDLER_ARGS)
    return (err);
 }

+static int
+acpi_video_vo_step_sysctl(SYSCTL_HANDLER_ARGS)
+{
+   struct acpi_video_output *vo;
+   int stepsize, err;
+
+   vo = (struct acpi_video_output *)arg1;
+   ACPI_SERIAL_BEGIN(video_output);
+   if (vo->handle == NULL) {
+       err = ENXIO;
+       goto out;
+   }
+
+   stepsize = vo->vo_step;
+
+   err = sysctl_handle_int(oidp, &stepsize, 0, req);
+   if (err != 0 || req->newptr == NULL)
+       goto out;
+   if (stepsize < -1 || stepsize > 100) {
+       err = EINVAL;
+       goto out;
+   }
+
+   vo->vo_step = stepsize;
+
+out:
+   ACPI_SERIAL_END(video_output);
+   return (err);
+}
+
 /* ARGSUSED */
 static int
 acpi_video_vo_bright_sysctl(SYSCTL_HANDLER_ARGS)
 
[ X200's brightness levels ]

This is a sane configuration, no need to modify anything.

Sure, and the keys work without loading acpi_video anyway.

But you might generalise your code comments to be equally applicable to one like that?

I really appreciate any hint to improve. If you have a comment which might not be true at the end, it is still better to double check.

Ok, well I'm not tackling your stepping algorithm, but this one is still an issue I think:

Code:
+static int
+acpi_video_vo_step_sysctl(SYSCTL_HANDLER_ARGS)
+{
+   struct acpi_video_output *vo;
+   int stepsize, err;
+
+   vo = (struct acpi_video_output *)arg1;
+   ACPI_SERIAL_BEGIN(video_output);
+   if (vo->handle == NULL) {
+       err = ENXIO;
+       goto out;
+   }
+
+   stepsize = vo->vo_step;
+
+   err = sysctl_handle_int(oidp, &stepsize, 0, req);
+   if (err != 0 || req->newptr == NULL)
+       goto out;
+   if (stepsize < -1 || stepsize > 100) {
+       err = EINVAL;
+       goto out;
+   }
+
+   vo->vo_step = stepsize;
+
+out:
+   ACPI_SERIAL_END(video_output);
+   return (err);
+}
+
 /* ARGSUSED */
 static int
 acpi_video_vo_bright_sysctl(SYSCTL_HANDLER_ARGS)

That allows stepsize to be -1 or 0. Both are EINVAL, surely? So shouldn't that be (stepsize < 1) ?
Similarly over 50 isn't sensible. I still reckon 20 for largest step (giving 6 levels incl. 0 and 100).

Cheers
 
But you might generalise your code comments to be equally applicable to one like that?
Unfortunately I do not understand the meaning. If it is about the key functions, on my system they do not trigger an event which can be monitored by xev. This is with or without acpi_video.

That allows stepsize to be -1 or 0. Both are EINVAL, surely? So shouldn't that be (stepsize < 1) ?
Similarly over 50 isn't sensible. I still reckon 20 for largest step (giving 6 levels incl. 0 and 100).
Yes, this is correct. The case of -1 is catched elsewhere to end up as +1, but it is better not to accept -1 or 0 at all.
A step of 100 makes no sense to me, too. But it can be used to switch on/off. In practice even 20 is quite coarse.
 
Back
Top