Keyboard backlight change UI

sidevesh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

stuarthayhurst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/PJungkamp/yoga9-linux/issues/4#issuecomment-1331489516 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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

If there is a KBLC method the backlight is either HIGH/LOW/OFF or AUTO/HIGH/LOW/OFF.
Call KBLC with 0x1. 0x5 means HIGH/LOW/OFF and 0x7 means AUTO/HIGH/LOW/OFF.
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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

stuarthayhurst commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

@stuarthayhurst is this sent to upstream ?

stuarthayhurst commented 10 months ago

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 commented 10 months ago

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

stuarthayhurst commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

stuarthayhurst commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Pulled into mainline: https://github.com/torvalds/linux/commit/ecaa1867b5243cf99e6ce9b46e372a66bd7cbfa2

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

sidevesh commented 10 months ago

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 commented 10 months ago

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 commented 8 months ago

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 commented 8 months ago

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: https://github.com/torvalds/linux/commit/ecaa1867b5243cf99e6ce9b46e372a66bd7cbfa2

RayOfLight1 commented 8 months ago

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: @.*** https://github.com/torvalds/linux/commit/ecaa1867b5243cf99e6ce9b46e372a66bd7cbfa2

— Reply to this email directly, view it on GitHub https://github.com/tomsom/yoga-linux/issues/39#issuecomment-1804896999, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJ52UMLH2L3WY7GKLRUBJLYDVZ5JAVCNFSM6AAAAAAYVUVSO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUHA4TMOJZHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

stuarthayhurst commented 8 months ago

@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 commented 8 months ago

@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

tomsom / yoga-linux

Keyboard backlight change UI #39