Page MenuHomeFreeBSD

acpi_video: try our best to work on systems without non-essential methods
ClosedPublic

Authored by avg on May 1 2020, 4:46 PM.

Details

Summary

Only _BCL and _BCM methods seem to be essential to the driver's operation.
If _BQC is missing then we can assume that the current brightness is whatever
we set by the last _BCM invocation.
If _DCS or _DGS is missing the we can make assumptions as well.

The change is based on a patch suggested by
Anthony Jenkins <Scoobi_doo@yahoo.com> in PR
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207086

Test Plan

This has been tested on a system with _BQC, _DCS and _DGS missing.
DSDT: https://wiki.freebsd.org/Laptops/Motile_M141?action=AttachFile&do=get&target=acpidump.txt

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg requested review of this revision.May 1 2020, 4:46 PM

Maybe it would be more clean to request the current level with _BCQ at attach and never deal with the method after, only the cached value ?
I don't see why we would want to call this method over using the cache value after boot.
Otherwise looks good to me.
I think that the _DCS/_DGS fix could be done in another commit though.

In D24653#542928, @manu wrote:

Maybe it would be more clean to request the current level with _BCQ at attach and never deal with the method after, only the cached value ?
I don't see why we would want to call this method over using the cache value after boot.
Otherwise looks good to me.
I think that the _DCS/_DGS fix could be done in another commit though.

Mhm, now I wonder if the level change on a system with _BCQ when using intel drm brighness changes ...
If so that would make sense to always try calling the _BCQ method.
I have to check on my others laptop, this one doesn't have _BCQ.

Yeah, I think that it is better to use _BQC whenever it is actually provided because there can be multiple ways to control the brightness.
The more I look into this the more quirks I discover.
Some systems handle brightness keys completely in firmware, some post ACPI events, some post custom events via WMI.
On some systems _BCM actually changes the brightness (it's handled by the firmware); on some systems _BCM is just another level of indirection -- it just posts more events to be handled elsewhere (e.g., via ATIF with AMD's video).
I think that Linux drifts towards converting brightness events to evdev brightness key events and then letting the userland handle those keys. The actual brightness controls are exposed as sysfs backlight nodes and they cab be hooked to ACPI video (_BCM) or directly to graphics drivers or something vendor specific, etc.

So, I wanted to be very conservative with my change and just let acpi_video work (as best as it can) on my system.

P.S.
I think that maybe it would be a good idea to add an option for acpi_video to register as an evdev input.

In D24653#543367, @avg wrote:

Yeah, I think that it is better to use _BQC whenever it is actually provided because there can be multiple ways to control the brightness.
The more I look into this the more quirks I discover.
Some systems handle brightness keys completely in firmware, some post ACPI events, some post custom events via WMI.
On some systems _BCM actually changes the brightness (it's handled by the firmware); on some systems _BCM is just another level of indirection -- it just posts more events to be handled elsewhere (e.g., via ATIF with AMD's video).
I think that Linux drifts towards converting brightness events to evdev brightness key events and then letting the userland handle those keys. The actual brightness controls are exposed as sysfs backlight nodes and they cab be hooked to ACPI video (_BCM) or directly to graphics drivers or something vendor specific, etc.

So, I wanted to be very conservative with my change and just let acpi_video work (as best as it can) on my system.

Yup agreed on all points.
I'll find time to add my backlight framework into acpi_video and how to "deprecate" some backlight, so if a backlight is known to be "better" (i.e. the one handled by the drm driver) it could disable the acpi_video one.

P.S.
I think that maybe it would be a good idea to add an option for acpi_video to register as an evdev input.

That would be good to do, we might need some sysctl to control this so the user would have the choice of letting acpi_video(4) doing it's thing or fallback on the DE or whatever would listen to those type of event.

P.S.: The patch as is doesn't apply on head.

This revision is now accepted and ready to land.May 4 2020, 8:26 AM

rebase on the latest tree

This revision now requires review to proceed.May 4 2020, 12:51 PM
This revision is now accepted and ready to land.May 4 2020, 1:55 PM