Keyboard backlight change UI

[sidevesh]

I have an AMD 5800O Yoga 7 14ACN6, which is similar to this.
One minor issue in interest of listing everything is that while the keyboard backlight changing works via the Fn+Space shortcut,
it does not reflect in UI and no pop up / osd is shown in gnome.
In my older laptops the keyboard backlight changes would show an osd,
This is something listed in the last point at https://github.com/PJungkamp/yoga9-linux#extra-features:

The ACPI can also control the keyboard backlight setting (Off/Low/High/Auto) but I have not written a patch for that yet.

DOes this work normally in 14ARB7 for y'all ?

[stuarthayhurst]

The ideapad platform driver it uses doesn't recognise the events the backlight sends (use dmesg and read the output when changing the backlight level), I have plans to write a kernel patch for this, but I haven't gotten around to it yet.

[sidevesh]

yah, I can see [85630.622027] ideapad_acpi VPC2004:00: Unknown event: 12 in dmesg, does that match what you are seeing in your machine ?

[sidevesh]

@stuarthayhurst I dont have much knowledge (or really any at all) of linux kernel development, but I will be interested in working on one if you can fill me in on the required changes, if you think this is simple enough for someone with no background into linux kernel but background in C / C++ to be able to do then lmk and I can help out here :)

[stuarthayhurst]

The kernel is written in C, you'll find it in the ideapad-driver module. This line should be the switch-case responsible for handling the events detected. From memory, the backlight isn't detected in the ACPI table, but if you can get the driver to recognise the backlight, and connect the signals to the right handlers, it should integrate with the rest of the system properly.

I don't know how complex this will end up being, I've only spent a bit of time working on a driver for my headset, patches for my keyboard and testing other peoples' stuff.

[stuarthayhurst]

yah, I can see [85630.622027] ideapad_acpi VPC2004:00: Unknown event: 12 in dmesg, does that match what you are seeing in your machine ?

Yep, that's the event id that needs handling, but the backlight also needs to register with the kernel as an LED before the signal can be any use

[stuarthayhurst]

I did some more digging, and a keyboard backlight isn't even being registered with the kernel yet, so simply connecting the signal up wouldn't work.

ideapad_check_features is responsible for detecting this, and it checks a bit from something exposed to the kernel(?). I'm doing a test build which ignores the check and enables the keyboard anyway, and also has the event wired up. If this works, I'll look into which bit / mechanism is being used to advertise the keyboard's backlight, and throw together a quirk for the kernel.

If this doesn't work, then I'm out of my depth and it's a problem for someone at Lenovo.

[stuarthayhurst]

Hm no luck. No errors, it registers a keyboard, but it's stuck at brightness 0.

[stuarthayhurst]

Oh right, ideapad_kbd_bl_brightness_get is still using the same system I overrode. It's checking a specific bit, so if the bit to expose the keyboard support and event has changed, that bit probably changed too. I'll put some debug in later and investigate a bit deeper.

[stuarthayhurst]

Hm, the driver seems set up to basically report either on / off, and set on / off. It doesn't seem to be aware of multiple brightness levels. Looking through the debug reported by the driver, none of the data parsed by the driver changes when the backlight changes, so the driver is completely unaware of the changes.

This is going to take a much better knowledge of ACPI than I have, or specific knowledge of the laptop. Maybe someone at Lenovo knows, but considering it's not a Thinkpad, they probably won't care much about supporting it. Still worth a shot if anyone knows who to contact.

[sidevesh]

thanks @stuarthayhurst for working on this, would it be possible to just implement it as an backlight on off indicator and maybe allow it to be turned on or off from the ui ?

[sidevesh]

I wish I could help directly, I have been going over the ideapad driver source in case I can figure something out.
I have had some discussion about this last year on another project:
PJungkamp/yoga9-linux#4 (comment)
Does this help us in some way ?
Also, I had seen this https://github.com/PJungkamp/yoga9-linux/tree/main/acpi-dump#readme

These maybe are just for intel versions so not applicable here but thought I will share just in case.

[stuarthayhurst]

I'll test my machine later, but if he's found a way to configure the backlight using ACPI, that's exactly what I wasn't sure how to do.

I'll look into this later and see if I can get something simple working.

[stuarthayhurst]

I get Error: AE_NOT_FOUND when using exactly the same commands as the intel version. I poked around my own ACPI tables a bit, and using \\_SB.PCI0.LPC0.EC0.VPC0.KBLC I don't get an error anymore.

However, the values in his table have no effect on my system. Everything returns 0x0, except for 0x1, which returns 0x5.

[PJungkamp]

Could you attach the decompiled DSDT containing the KBLC function? I'm kind of curious now.
Are the keyboards on the AMD models also Off/Low/High/Auto controllable or is there only Off/Low/High?

[stuarthayhurst]

Thanks for taking a look, it's Off / Low / High for the AMD version.
dsdt.dsl.txt
Added .txt to stop GitHub complaining about file types.

[PJungkamp]

Could you try the ACPI calls for set/query from my notes with the second lowest hexadecimal digit set to 2?

E.g. 0x10023? I just skimmed the KBLC ACPI function. Based on your report of a call with 0x1 as an argument returning 0x5, this should at least do something.

[stuarthayhurst]

Yep, that works perfectly, thanks.

I'll look into modifying the driver when I get time, but that won't be for a while.

[PJungkamp]

I presume the value read may indicate the number of supported states or the version of the protocol. The ACPI ASL code is the same as on my Intel model. You can deduce the second lowest hex digit from the 0x01 query. So a driver may work for both models.

[stuarthayhurst]

The core of it should be the same for the 2, where we need to set a new max_brightness on models with more than just on / off. Then wire up the calls to set (off / low / high), and connect the event to a call to query the brightness.

I'm not sure how the auto level would fit into the driver, but I also don't have the hardware to test it.

[stuarthayhurst]

Doing a test build now for a quick attempt at supporting the AMD version. I'll update with how it goes, but I won't be able to work on it for the next ~20 days as I'm busy. I'll upload my attempt at a patch in case anyone wants to work on it :)

The Intel version will need different values, and handling for the auto state. A proper upstream-able version would also need proper error handling in the ACPI calls as well.

Warning to anyone testing this: it's prodding at firmware, so who knows what happens if something goes wrong.
0001-Attempt-patching-for-Yoga-keyboard.patch.txt

[stuarthayhurst]

Whoops, turns out I'm and idiot and forgot some braces.

0001-Attempt-patching-for-Yoga-keyboard.patch.txt

EDIT: This patch works for setting the LED backlight, but it seems to be triggering the default case for the backlight reporting, so it reports 0. I probably just got the format of the returned values wrong, but I'm all out of time for now, but a driver is 100% possible for this keyboard.

[stuarthayhurst]

Never mind, I got it working :D
0001-Attempt-patching-for-Yoga-keyboard.patch.txt

That should apply cleanly to 6.4.4, or the latest git.
Getting and setting the backlight works, and the hardware change event is also connected up now. I'll work on proper error handling and making sure it's upstream-able when I can.

@PJungkamp Since I don't have Intel hardware, it would be incredibly helpful if you could tweak the values and test your system too. I'm not sure about the cleanest way to support both at the same time, but feel free to work on that :)

[PJungkamp]

I haven't tried it yet. But the general idea would be:

1. If there is a KBLC method the backlight is either HIGH/LOW/OFF or AUTO/HIGH/LOW/OFF.
2. Call KBLC with 0x1. 0x5 means HIGH/LOW/OFF and 0x7 means AUTO/HIGH/LOW/OFF.
3. Use 0xXXXX0023 commands for HIGH/LOW/OFF and 0xXXXX0033 commands for AUTO/HIGH/LOW/OFF.

All the values can actually be determined by some bitshifts. The return value ret of KBLC can be shifted by one type = ret >> 1 to obtain the value needed in the set-brightness commands. type = 2 would then indicate your tristate and type = 3 the tristate + auto. This coincides nicely with the backlight-type enum you created in your patch.

• A set-brightness command then looks like (value << 16) | (type << 4) | BL_SET with BL_SET = 0x3. We should check value > 0 && value <= type for the input.
• A query-brightness command is then (type << 4) | BL_GET with BL_GET = 0x2. You can get the backlight value from the return value ret of the query by shifting and masking (ret & 0xFFFF) >> 1. We should also do a sanity bounds check here.
• A least-significant-bit of zero in any command indicates failure and should be checked and reported.

When I get around to it I'll slightly adapt your patch to these values, put some checks on those and post it here.

[PJungkamp]

Another small thing I remembered from my experiments. There is another way to get the current value of the backlight in the form of an integrated sensor sending events when the backlight state updates. For me it's this one. I found it while checking the Intel ISH sensors. There should be a similar hub for sensors on AMD platforms, but I don't know what Lenovo used there.

[stuarthayhurst]

Ah cool, thanks. What should be reported when the state is AUTO? I'd assume 0 as a default, because an error code implies a failure of some sort.

I can't remember what the current patch will do in that case, I threw it together right before I had to leave.

Speaking of throwing it together, I kind of just threw the new code paths in to test they worked. They could probably be integrated into the existing code much better, and any error reporting in there so far was for debug.

On my AMD system, I had a quick look through some of the sensors present and couldn't see an obvious one to handle it, but I didn't look very thoroughly.

[PJungkamp]

The firmware handles AUTO as the 4th state, reporting 0x3.

[stuarthayhurst]

Sure, I meant what should the brightness sysfs attribute report when the keyboard is in auto?

Giving it a number isn't necessarily correct, as we don't know the actual brightness, only that it's in auto mode. Giving it 0 seems most appropriate, as an error code implies an issue, when everything is behaving.

[stuarthayhurst]

0001-platform-x86-ideapad-laptop-Add-support-for-keyboard.patch.txt
Alright, new patch. Changes:

• An actual commit message, this probably needs to be improved before upstreaming though
• Error handling while setting the backlight, and while checking the features
• Detection for the Intel variant (KBD_BL_TRISTATE_AUTO)
• Return -EINVAL instead of the raw value if it gets an unexpected brightness level
• Interpret auto as 0 brightness, as it's got to report something
• Support for the AMD and Intel variants using the same code paths (thanks to your bitshifting)
  • I haven't done any manipulation on the returned value from the get command yet, it's been left as a switch-case for now

I haven't tested this patch yet, but it applies to the current git fine, I'm just compiling a test build.

EDIT: Tested it, works just fine for my system. Hopefully it works well on Intel too :)

EDIT 2: This patch v2 has better comments, extra whitespace removed and a shared path in the set call (previously it was a bit rigged together for testing)
Attaching it separately, as I haven't verified it compiles and works yet

v2-0001-platform-x86-ideapad-laptop-Add-support-for-keyboard.patch.txt

EDIT 3: Tested, working fine

[PJungkamp]

Some small notes upon reading it:

• Does not quite work for me as a query for my machine returns a 1 in the 17th bit, e.g. 0x00010003.

• I'd also like to note that returning 0 for auto isn't too nice as auto becomes indistinguishable.

[stuarthayhurst]

Whoops, forgot to fix that part, thanks
v3-0001-platform-x86-ideapad-laptop-Add-support-for-keyboard.patch.txt

I also don't really like returning 0 for auto, but I'm probably going to have to ask a mailing list to see what would be accepted upstream.

Possibilities:

• Numerical value [0, 2]: If we go with any of these, AUTO would appear as an otherwise valid brightness, which is misleading
• brightness = 3, max_brightness = 2: This is out of the range of supported brightnesses reported
  • I guess this indicates special meaning, but it's confusing for userspace and I doubt it would ever be accepted as it really feels like a programming error
• brightness = 3, max_brightness = 3: This would essentially be adding a 4th state that can be reported, but userspace would interpret this as HIGH brightness
  • It would also mean programs trying to set max brightness would actually enable AUTO instead
• An error code: This doesn't give any false information at least
  • But it also doesn't convey that it's in AUTO mode, and somewhat implies something went wrong, when nothing did go wrong

None of these feel like a good fit, so I just chucked 0 in there for now, until we have a better answer :)

[stuarthayhurst]

v5-0001-platform-x86-ideapad-laptop-Add-support-for-keyboard.patch.txt

This one uses bit shifting for reading the brightness of the keyboard, and validates the returned value. It could do with testing for both Intel and AMD users, but it's all working on my system.

If everyone else is happy with it, I'll send it upstream, and ask about how to handle the auto value, in a way that would be accepted upstream.

[sidevesh]

@stuarthayhurst is this sent to upstream ?

[stuarthayhurst]

No, so far I don't know if anyone's tested it other than me, ideally I'd be sure it works on Intel before I send it.

[sidevesh]

I think @PJungkamp has an Intel system, so they can confirm this.

[stuarthayhurst]

@sidevesh I noticed you have the 14ACN6, which is different from my 14ARB7. If possible, could you also test please?
Any testing is good, testing on different platforms is even better :)

[sidevesh]

I can try, I have never compiled linux from scratch though, I am using arch with linux-zen kernel, will it be possible to apply this on top of linux-zen ?
If yes then I guess it should be possible to apply this patch on top of it from arch build system right ?

[stuarthayhurst]

I don't see why not, linux-zen doesn't seem to modify the driver this patch is for. I haven't rebuilt arch packages before, only Debian.

This seems like a good place to start: https://wiki.archlinux.org/title/Kernel/Arch_build_system

[sidevesh]

@stuarthayhurst I tested it out and it worked, only issue I saw was that there was no indicator when it shifted to AUTO

[stuarthayhurst]

As long as there are no errors in sudo dmesg -w when it shifts to AUTO, that's expected, as it treats AUTO as off. I'll ask upstream about this, as I don't know how to report AUTO, but someone upstream will have some insight.

[stuarthayhurst]

Forgot to update this thread, it's been sent upstream: https://lore.kernel.org/platform-driver-x86/20230825122925.7941-1-stuart.a.hayhurst@gmail.com/T/#u

Up to the 3rd revision on that mailing list now, but hopefully it'll land in time for 6.6, as platform-driver-x86 haven't sent their pull request off yet.

Feel free to test the patch again if you have a keyboard with the AUTO state, it should still work, but it doesn't hurt to be safe.

[stuarthayhurst]

Pulled into for-next of the platform-drivers-x86 repo, Linux 6.6 unless anyone finds an issue.

Upstream didn't comment on the auto handling, so if anyone wants to fix that, you'll probably need to add a sysfs attribute or something to communicate that.

[sidevesh]

looks to me like auto brightness setting is just not a thing anywhere in the stack, from the kernel to upower dbus to DE (GNOME atleast, including with the newly added quick toggle in 45)

Question, do we get a report from the acpi when the backlight changes automatically ?

https://upower.freedesktop.org/docs/KbdBacklight.html supports BrightnessChangedWithSource which can be used to indicate if brightness was changed internally,

maybe when auto gets set we just pull the current brightness and notify that,
and if the brightness changes automatically we just notify again with new value ?

[stuarthayhurst]

Pulled into mainline: torvalds/linux@ecaa186

Since the 14ARB7 doesn't have auto handling, can this be closed?

[sidevesh]

Sure, if you have any pointers on the AUTO handling then will appreciate it so I can try getting it set up for my system which does support AUTO.
Closing this and thanks for the work to get this working!

[stuarthayhurst]

Sure, if you have any pointers on the AUTO handling then will appreciate it so I can try getting it set up for my system which does support AUTO.

Personally, I'd ask on a mailing list about how it should be handled, as I don't know myself. If you figure it out, you'll have to remove the condition or two in there to treat it as OFF, or convert them to report something else.

Closing this and thanks for the work to get this working!

No problem :)

[sidevesh]

I recently installed the Linix 6.6 release and the keyboard backlight pop up does not seem to be showing for me, I had tested the patch earlier when it was in development and it was working, did this not get releaeed in 6.6 and is it working for you @stuarthayhurst ?

[stuarthayhurst]

It made it to 6.6, it was working last time I tested a 6.6 kernel, but 6.6 hasn't made it to Debian Sid / Experimental yet, so I'm still running 6.5 day-to-day

The patch was changed during review for the kernel, the final commit can be found here: torvalds/linux@ecaa186

[RayOfLight1]

I'm using 6.6 and Plasma, the power applet works wonderfully, I can adjust backlight from there, and it reports real time any change done by keyboard shortcut. Cool job. Atentament: Jordi Paradell ***@***.*** Missatge de Stuart Hayhurst ***@***.***> del dia dv., 10 de nov. 2023 a les 1:41:

…

It made it to 6.6, it was working last time I tested a 6.6 kernel, but 6.6 hasn't made it to Debian Sid / Experimental yet, so I'm still running 6.5 day-to-day The patch was changed during review for the kernel, the final commit can be found here: ***@***.*** <torvalds/linux@ecaa186> — Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJ52UMLH2L3WY7GKLRUBJLYDVZ5JAVCNFSM6AAAAAAYVUVSO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUHA4TMOJZHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[stuarthayhurst]

@sidevesh If it's still not working, can you double check that you're running 6.6 (uname -a), and that the driver is loaded (sudo lsmod |grep ideapad)?

[sidevesh]

@stuarthayhurst
uname -r: 6.6.1-zen1-1-zen

sudo lsmod |grep ideapad:

ideapad_laptop         61440  0
sparse_keymap          12288  2 ideapad_laptop,lenovo_ymc
platform_profile       12288  1 ideapad_laptop
rfkill                 40960  10 iwlmvm,bluetooth,ideapad_laptop,cfg80211
i8042                  57344  1 ideapad_laptop
video                  77824  2 amdgpu,ideapad_laptop
wmi                    45056  4 video,wmi_bmof,ideapad_laptop,lenovo_ymc

[stuarthayhurst]

Do you have anything in /sys/class/leds/platform::kbd_backlight/? If you do, what's the value of /sys/class/leds/platform::kbd_backlight/max_brightness?

[sidevesh]

/sys/class/leds/platform::kbd_backlight does not exist on my system

[stuarthayhurst]

Do you have any messages about Unknown keyboard type in dmesg?

EDIT: Unknown keyboard might also be useful too

[sidevesh]

[sidevesh@Lenovo-Yoga-7 ~]$ sudo dmesg | grep 'Unknown'
[    5.601817] lenovo-ymc 06129D99-6083-4164-81AD-F092F9D773A6: Unknown key 0 pressed
[    5.605022] ideapad_acpi VPC2004:00: Unknown keyboard backlight value: 3

[stuarthayhurst]

Ah, I think you had it in auto mode when the device started, causing the get function to fail, so the initial value was an error, so the init failed and the keyboard wasn't registered.

I guess my code to handle AUTO is broken :/ I'll try patch this and send you something for testing, thanks for catching this

EDIT: Never mind, it seems we use max_brightness before it gets set. No idea how this works on my system...

[stuarthayhurst]

Here's a completely untested patch written for 6.6:
0001-platform-x86-ideapad-laptop-Set-max_brightness-befor.patch.txt

@sidevesh Can you give that a test please? If it doesn't work for you I'll write another :)

EDIT: Tested on my system, no regressions. Sent upstream anyway, as it needed to be fixed, whether or not it actually caused your issue. As I mentioned earlier, if it doesn't work for you I'll keep troubleshooting :)
https://lore.kernel.org/platform-driver-x86/20231112152851.17687-2-stuart.a.hayhurst@gmail.com/T/#u

EDIT 2: Accepted into platform-drivers-x86

[sidevesh]

@stuarthayhurst sorry for the delay, I will test out the patch this weekend and let you know if the issue gets fixed or if I encounter any issues, thanks for resolving this so quickly!

[PJungkamp]

[sidevesh@Lenovo-Yoga-7 ~]$ sudo dmesg | grep 'Unknown'
[    5.601817] lenovo-ymc 06129D99-6083-4164-81AD-F092F9D773A6: Unknown key 0 pressed
[    5.605022] ideapad_acpi VPC2004:00: Unknown keyboard backlight value: 3

Same problem here. I totally forgot about this thread as I'm kind of busy recently. I'll try your patch right away and report back.

[PJungkamp]

It's working beautifully! GNOME 45 just hit nixpkgs-unstable and upon reading the release notes I remembered this issue. Thank you very much for your work on this.

The ACPI does not seem to provide an event when the keyboard backlight is changed using the keyboard shortcut (fn+space).
I have seen that there's another "custom" HID device on the Intel ISH that's able to report these changes. (The Lenovo tools in Windows show updates of the backlight)

Maybe I'll give this another go myself if I've got some time (this and some sensible reporting of an auto mode).