Get ACPI notification for brightness keys from GFX0.DD1F/DD02 #22

zhen-zen · 2020-08-19T07:40:20Z

EC query for brightness key usually notifies a device under IGPU(GFX0).
For example:

Method (_Q14, 0, NotSerialized)
{
    Notify (\_SB.PCI0.GFX0.DD1F, 0x86)
}

Method (_Q15, 0, NotSerialized)
{
    Notify (\_SB.PCI0.GFX0.DD1F, 0x87)
}

The device usually has an _ADR of 0x400 and is named as DD1F on Haswell+ machines (DD02 for Sandy Bridge). By subscribing to ACPI notification of this device, brightness key will work out of box.

According to ACPI Spec Appendix B.6, 0x86 is for Increase Brightness and 0x87 is for Decrease Brightness. It also defines 0x85 for Cycle Brightness, 0x88 for Zero Brightness and 0x89 for Display Device Off. However latter ones don't have corresponding ADB key and are rarely used or handled by firmware.

For machines with patched DSDT, possible duplicated input will be ignored for compatibility.

There's also _BCL, _BCM, _BQC method to adjust panel brightness. Are them deprecated now in favor of SSDT-PNLF? Rehabman introduced KBCL, KBCM, KBQC under PS2K to call these methods. But this part haven't been updated for a long time.

zhen-zen · 2020-08-19T07:52:37Z

Almost missed a warning. Xcode editor is broken due to a special character and failed to display git revision and warning in that file.

vit9696 · 2020-08-19T10:04:42Z

Hmm, usually people just write a call to PS2K when hitting this notification in their ACPI. At least I did very long ago. Is it really a good idea to change that in the first place? If it is, I strongly do not like hardcoding the ACPI path for the devices. The device should be possible to match by _HID or other methods by mere iteration over the ACPI plane.

zhen-zen · 2020-08-19T10:20:46Z

Hmm, usually people just write a call to PS2K when hitting this notification in their ACPI. At least I did very long ago. Is it really a good idea to change that in the first place? If it is, I strongly do not like hardcoding the ACPI path for the devices. The device should be possible to match by _HID or other methods by mere iteration over the ACPI plane.

My starting point is to eliminate the additional work on SSDT. ACPIDebug in the old method is not well supported on newer systems and I have got a lot of KPs with that.
I have also tried to avoid the hardcoded part but failed. But there's no property like _HID that could be easily identified. The list should have covered most cases based on samples I have gathered. (Asus's has some differences but that was handled by AsusSMC)

vit9696 · 2020-08-19T10:39:45Z

Hmmm, could you provide some ACPI dumps? I wonder if we can at least use some parent device…

zhen-zen · 2020-08-19T10:50:02Z

BrightnessSample.zip
So far, I suppose it could only be distinguished by their _ADR (0x400 for iGPU and 0x110 for dGPU) or maybe the existence of _BCL _BCM, _BQC method (low confidence).

HQuest · 2020-08-19T14:30:23Z

On my Dell Latitude 7480 (i5-6300U/HD520 Skylake based), both DD02 and DD1F devices are defined as external and not found at any of the system ACPI tables. Neither _Q14 or _Q15 methods are available. The only call for brightness adjusts 0x86/0x87 are under BRT6 method. Not sure this would be applicable to all devices out there but to perhaps certain use cases.

vit9696 · 2020-08-19T16:06:13Z

There is one common thing between all these DSDTs. These devices sit on a graphics card, so you could look up a graphics card by a PCI class code (check Lilu code), and then find its child devices. If necessary, we could make VoodooPS2 depend on Lilu actually.

For an IGPU I believe it is rock solid that this method is going to have 0x400 address. Vendors simply copy-paste the Intel RC code, and it is coming from there. Even my desktop has it.
For a dGPU I believe it is more complicated, but dGPUs rarely have that in the first place. Could hardcode either an address or a list of device names. I believe we will have to readdress it with the time.

Sniki · 2020-08-19T16:30:21Z

There is one common thing between all these DSDTs. These devices sit on a graphics card, so you could look up a graphics card by a PCI class code (check Lilu code), and then find its child devices. If necessary, we could make VoodooPS2 depend on Lilu actually.

For an IGPU I believe it is rock solid that this method is going to have 0x400 address. Vendors simply copy-paste the Intel RC code, and it is coming from there. Even my desktop has it.

For a dGPU I believe it is more complicated, but dGPUs rarely have that in the first place. Could hardcode either an address or a list of device names. I believe we will have to readdress it with the time.

Exactly, the majority of the people who will need this, do use IGPU and have custom SSDT/Patch to map to Brightness keys.
So this will be a decent configuration cleanup for the majority of the people.

Because as you said DGPU(s) are more complicated and most of them don't work as they use something like Optimus (nvidia) and the AMD hybrid.
I think the only exception for this may be some laptops that have the option to choose DGPU only / as main GPU on BIOS and completely disable intel, (some Lenovo ThinkPad workstation laptops like P1 Extreme if i am correct).
So since we all already use custom patches for mapping, let the DGPU users continue like that until it is resolved for them as well and it's a small number of people anyway.

Thanks !

vit9696 · 2020-08-19T17:00:15Z

I do not believe we should completely ignore dGPU machines. E.g. in SNB times there were many machines without IGPU at all.

Sniki · 2020-08-19T17:19:31Z

I do not believe we should completely ignore dGPU machines. E.g. in SNB times there were many machines without IGPU at all.

No, i actually didn't mean to ignore them, just meant to say that this targets the majority.
Yes 1st and 2nd generation ones were common to have only DGPU.
Some 1st gen Intel Dell Inspiron 5**** series have AMD graphics and no IGPU at all.

zhen-zen · 2020-08-19T23:17:00Z

Just realized gIODTPlane might be a good start point and got the following code working on Big Sur (Kernel Collection), but failed for Catalina. Maybe something's wrong with kext loading? I suppose init() should always happen before start().

    IORegistryEntry* entry;
    OSString* loc;
    if ((entry = IORegistryEntry::fromPath("/PCI0@0/IGPU@2/DD1F@400", gIODTPlane)) ||
        (entry = IORegistryEntry::fromPath("/PCI0@0/IGPU@2/DD02@400", gIODTPlane))) {
        loc = OSDynamicCast(OSString, entry->getProperty("acpi-path"));
        if (loc) {
            setProperty(kBrightnessDevice, loc);
            if ((_gfx = (IOACPIPlatformDevice*)IORegistryEntry::fromPath(loc->getCStringNoCopy()))) {
                if ((_gfxNotifiers = _gfx->registerInterest(gIOGeneralInterest, _gfxNotification, this)))
                    setProperty(kBrightnessDevice, _gfx->getName());
                else
                    IOLog("ps2br: unable to register interest for GFX notifications\n");
            }
        }
        entry->release();
    }

vit9696 · 2020-08-20T01:07:32Z

You are doing matching in parallel with IODeviceTree population. It is not instant. If you check e.g. Lilu code it is waiting for devices to publish for that very reason.

vit9696 · 2020-08-20T11:23:20Z

VoodooPS2Keyboard/VoodooPS2Keyboard.cpp

+    // Waiting for IGPU
+    size_t counter = 20;
+    while (counter--) {
+        if ((entry = IORegistryEntry::fromPath("/PCI0/IGPU", gIODTPlane)))


Ugh, why not to iterate over the devices and look for an intel-based vendor. This is just ugly, as not all PCI root hubs are named PCI0 even. Have a look at https://github.com/acidanthera/Lilu/blob/master/Lilu/Sources/kern_devinfo.cpp

Sorry, I have little knowledge with this situation. I found some gen1 laptop examples but the IGPU is still under PCI0.
My assumption here is that the user either had the old-school GFX0 to IGPU patch, or adopted WhateverGreen that provided gen1 support recently.

Like I said, it is not always the case. Especially on devices with multiple PCI lanes and exotic chipsets. Please change that.

vit9696 · 2020-09-11T15:09:30Z

Please try the latest addition I pushed in your branch.

zhen-zen · 2020-09-13T04:09:42Z

Just fixed my laptop. There's a panic at getBrightnessPanel and I'm trying to figure it out.

zhen-zen · 2020-09-13T05:58:51Z

The panic occurs when comparing the casted pointer from OSDynamicCast. I'm not sure what happened and got trapped in the same problem earlier. Maybe something's missing for RTTI?

vit9696 · 2020-09-13T11:47:36Z

Not good, I do not think we should avoid OSDynamicCast without understanding what exactly is wrong.

If it panics, that might mean it is not an IOACPIPlatformDevice? Is there an I/O registry for your platform?
If it definitely is, maybe it is not published in IOService plane yet? Did you try WIOKit::awaitPublishing(x)?
May I see the panic with symbols, might help to understand it better?

zhen-zen · 2020-09-13T16:04:21Z

My bad, I thought IOACPIFamily was in Info.plist for existing ACPI operations. I should have checked that file after sneaking the suspicion...

vit9696 · 2020-09-13T16:21:18Z

So, this looks good to me. Would you update the changelog mentioning Lilu dependency and the new features? Afterwards I will merge.

zhen-zen · 2020-09-13T16:26:15Z

Just after found the brightness panel, there was a legacy routine to adjust the brightness via ACPI method. It's only documented in https://www.tonymacx86.com/threads/new-voodoops2controller-keyboard-trackpad.75649/post-560175.

Required DSDT patch involves proxy ACPI calls to _BCL, _BCM and _BQC which is under the same 0x400/0x110 device.

Should it be depreciated for modern SSDT-PNLF? Or improve that by calling these methods directly?

vit9696 · 2020-09-13T16:48:17Z

Hmm. I might miss something. PNLF SSDT is just the code used to do two things:

Initialise backlight support in the hardware to be compatible with macOS driver
Expose the device for macOS drivers to enable macOS brightness management

Therefore there are two routes to change the brightness:

Whenever macOS wants to adjust the brightness itself, its kext simply writes the right value to a dedicated register. I think this is done via a IOUserClient call to AppleBacklight.
Whenever a button to adjust brightness is pressed by the user, macOS keyboard driver makes a request to that kext (AppleBacklight, I think) to adjust the brightness level. This should probably be done by userspace too, upon receiving a dedicated key press.

I think on the laptops you normally feed an SSDT to rebind some OEM keys to macOS brightness keys to make sure that the brightness keys on your keyboard actually cause the backlight change.

Now, how does that correlate with these changes? I am starting to wonder why do we need to directly adjust the backlight via ACPI methods?!

zhen-zen · 2020-09-13T16:56:18Z

You are right, that's the old way to manipulate _KCL, _KCM without SSDT-PNLF, and I think that won't create a notification in system. So I wonder if we can remove this functionality. Since it's not documented clearly, maybe it's not used widely?

vit9696 · 2020-09-13T17:02:14Z

Yes, I do understand why we need that when the keys are not dispatched via PS/2, and we only get an ACPI notification. I myself patched an ACPI once to achieve exactly that effect instead of changing the driver, but in all other cases it should be pretty much useless.

I also wonder whether there are systems that send both a notification and a keycode out of the box. If they do exist, perhaps we should provide a more or less intuitive way to disable the behaviour we have just added? What about a property in the GPU (the one with the panel) and a boot argument? The property could be named ignore-backlight-notify and the argument could be -nobklnotify or something like that.

zhen-zen · 2020-09-13T17:07:46Z

I think the latter issue is already addressed and tested when the old _QXX hack coexisted with ACPI notification (I have callback to renamed XQXX). Once ACPI notification is triggered, key code for brightness control will be silenced.

vit9696 · 2020-09-13T17:14:09Z

Hmmm, right. Let's assume it is enough to avoid the conflict for now. Please update the changelog and I will merge later today.

zhen-zen · 2020-09-15T03:33:32Z

It seems that there's an exception on ICL laptop(s). _ADR for DD1F is 0x1f although the ACPI code is not changed. I'm trying to find support references from graphics manual.

        Device (DD1F)
        {
            Method (_ADR, 0, Serialized)  // _ADR: Address
            {
                If ((EDPV == Zero))
                {
                    Return (0x1F)
                }
                Else
                {
                    Return ((0xFFFF & DIDX))
                }
            }

Currently following changes work for that laptop, and I'm still finding more samples.

diff --git a/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp b/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
index f96b3ee..370f694 100644
--- a/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
+++ b/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
@@ -391,10 +391,9 @@ IOACPIPlatformDevice* ApplePS2Keyboard::getBrightnessPanel() {
     };

     if (info) {
-        if (info->videoBuiltin != nullptr)
-            panel = getAcpiDevice(getDevicebyAddress(info->videoBuiltin, 0x400));
-
-        if (panel == nullptr)
+        if ((info->videoBuiltin != nullptr) &&
+            !(panel = getAcpiDevice(getDevicebyAddress(info->videoBuiltin, 0x400))) &&
+            !(panel = getAcpiDevice(getDevicebyAddress(info->videoBuiltin, 0x1f))))
             for (size_t i = 0; panel == nullptr && i < info->videoExternal.size(); ++i)
                 panel = getAcpiDevice(getDevicebyAddress(info->videoExternal[i].video, 0x110));

zhen-zen · 2020-09-15T03:48:25Z

However, it's quite weird that if 0x1f is returned for _ADR, another method for DD1F will be:

            Method (_DCS, 0, NotSerialized)  // _DCS: Display Current Status
            {
                If ((EDPV == Zero))
                {
                    Return (Zero)
                }
                Else
                {
                    Return (CDDS (DIDX))
                }
            }

Per ACPI spec, "If the output connector does not exist (when undocked), _DCS returns 0x00."

vit9696 · 2020-09-15T06:36:28Z

Unpleasant, but I guess we still can add more addresses. At least for now. Please submit a PR when the situation is more clear.

zhen-zen · 2020-09-15T07:15:03Z

I suppose there is something wrong with ICL driver when initializing panel or with OEM's configuration. If it's the latter case, I think DD1F (_ADR 0x1f) then DD02 (_ADR 0x02) could be retrieved as a last resort after probing both internal and external GFX devices. Currently I'm still collecting more cases.

zhen-zen · 2020-09-15T09:03:55Z

After comparing ioreg from multiple vendors, most of them both failed to set display output device address correctly except Dell. Under Linux, the address for DD1F is still 0x1f.

I'm asking for some more reports from Comet Lake ones, but maybe there won't be issues with old generation graphics.

So the proposed changes might be

diff --git a/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp b/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
index f96b3ee..6e3e4f8 100644
--- a/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
+++ b/VoodooPS2Keyboard/VoodooPS2Keyboard.cpp
@@ -391,9 +391,18 @@ IOACPIPlatformDevice* ApplePS2Keyboard::getBrightnessPanel() {
     };

     if (info) {
-        if (info->videoBuiltin != nullptr)
+        if (info->videoBuiltin != nullptr) {
             panel = getAcpiDevice(getDevicebyAddress(info->videoBuiltin, 0x400));

+            //
+            // On some IceLake Laptops, address of display output device may not export panel
+            // information, use 1f for DD1F instead
+            //
+            if (panel == nullptr)
+                if (BaseDeviceInfo::get().cpuGeneration == CPUInfo::CpuGeneration::IceLake)
+                    panel = getAcpiDevice(getDevicebyAddress(info->videoBuiltin, 0x1f));
+        }
+
         if (panel == nullptr)
             for (size_t i = 0; panel == nullptr && i < info->videoExternal.size(); ++i)
                 panel = getAcpiDevice(getDevicebyAddress(info->videoExternal[i].video, 0x110));

This reverts commit 86d8fb2. Revert "Fix typo in Changelog (#25)" This reverts commit 383e672. Revert "Workaround for ICL/CML brightness keys (#23)" This reverts commit 1e2925d. Revert "Get ACPI notification for brightness keys from GFX0.DD1F/DD02 (#22)" This reverts commit d5f0541.

zhen-zen added 2 commits August 18, 2020 20:04

Xcode 12 check

07aaa99

Get ACPI notification for brightness keys from GFX0.DD1F/DD02

99bd567

zhen-zen force-pushed the gfx branch from cd04d15 to 99bd567 Compare August 19, 2020 07:46

Add more possible path, thx @Sniki

f405e0f

Test IODT

db2c8a8

Get display device dynamically

118e61c

vit9696 reviewed Aug 20, 2020

View reviewed changes

zhen-zen marked this pull request as draft August 20, 2020 19:20

zhen-zen added 5 commits August 20, 2020 16:56

Extract getDevicebyAddress() for finding built-in display

b78a60b

Prototype for dynamic probing

8d43a5d

Revert WIP changes

f1c61bf

Less confusing name

ccb1239

Revert to original coding convention

6d84ca5

zhen-zen marked this pull request as ready for review August 21, 2020 08:30

zhen-zen requested a review from vit9696 August 24, 2020 07:00

vit9696 mentioned this pull request Aug 31, 2020

Update recommendation on ACPI patches acidanthera/OpenCorePkg#113

Merged

Improve brightness panel detection via Lilu

410d70e

vit9696 added 2 commits September 11, 2020 18:10

Fix indentation

992540d

Fix analyzer warning

6870082

Replace OSDynamicCast with C-style cast to avoid panic

5dadeec

zhen-zen force-pushed the gfx branch from 8b680d1 to 5dadeec Compare September 13, 2020 05:54

Revert to OSDynamicCast, fix dependency.

d0f59e9

Sync changelog

e9e7fae

vit9696 merged commit d5f0541 into acidanthera:master Sep 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get ACPI notification for brightness keys from GFX0.DD1F/DD02 #22

Get ACPI notification for brightness keys from GFX0.DD1F/DD02 #22

zhen-zen commented Aug 19, 2020 •

edited

Loading

zhen-zen commented Aug 19, 2020

vit9696 commented Aug 19, 2020

zhen-zen commented Aug 19, 2020 •

edited

Loading

vit9696 commented Aug 19, 2020

zhen-zen commented Aug 19, 2020 •

edited

Loading

HQuest commented Aug 19, 2020

vit9696 commented Aug 19, 2020 •

edited

Loading

Sniki commented Aug 19, 2020

vit9696 commented Aug 19, 2020

Sniki commented Aug 19, 2020

zhen-zen commented Aug 19, 2020

vit9696 commented Aug 20, 2020

vit9696 Aug 20, 2020

zhen-zen Aug 20, 2020 •

edited

Loading

vit9696 Aug 21, 2020

vit9696 commented Sep 11, 2020

zhen-zen commented Sep 13, 2020

zhen-zen commented Sep 13, 2020 •

edited

Loading

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020 •

edited

Loading

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 15, 2020

zhen-zen commented Sep 15, 2020 •

edited

Loading

vit9696 commented Sep 15, 2020

zhen-zen commented Sep 15, 2020

zhen-zen commented Sep 15, 2020 •

edited

Loading

Get ACPI notification for brightness keys from GFX0.DD1F/DD02 #22

Get ACPI notification for brightness keys from GFX0.DD1F/DD02 #22

Conversation

zhen-zen commented Aug 19, 2020 • edited Loading

zhen-zen commented Aug 19, 2020

vit9696 commented Aug 19, 2020

zhen-zen commented Aug 19, 2020 • edited Loading

vit9696 commented Aug 19, 2020

zhen-zen commented Aug 19, 2020 • edited Loading

HQuest commented Aug 19, 2020

vit9696 commented Aug 19, 2020 • edited Loading

Sniki commented Aug 19, 2020

vit9696 commented Aug 19, 2020

Sniki commented Aug 19, 2020

zhen-zen commented Aug 19, 2020

vit9696 commented Aug 20, 2020

vit9696 Aug 20, 2020

Choose a reason for hiding this comment

zhen-zen Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

vit9696 Aug 21, 2020

Choose a reason for hiding this comment

vit9696 commented Sep 11, 2020

zhen-zen commented Sep 13, 2020

zhen-zen commented Sep 13, 2020 • edited Loading

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 13, 2020 • edited Loading

vit9696 commented Sep 13, 2020

zhen-zen commented Sep 15, 2020

zhen-zen commented Sep 15, 2020 • edited Loading

vit9696 commented Sep 15, 2020

zhen-zen commented Sep 15, 2020

zhen-zen commented Sep 15, 2020 • edited Loading

zhen-zen commented Aug 19, 2020 •

edited

Loading

zhen-zen commented Aug 19, 2020 •

edited

Loading

zhen-zen commented Aug 19, 2020 •

edited

Loading

vit9696 commented Aug 19, 2020 •

edited

Loading

zhen-zen Aug 20, 2020 •

edited

Loading

zhen-zen commented Sep 13, 2020 •

edited

Loading

zhen-zen commented Sep 13, 2020 •

edited

Loading

zhen-zen commented Sep 15, 2020 •

edited

Loading

zhen-zen commented Sep 15, 2020 •

edited

Loading