Hi Rishit, On 2/2/23 20:59, Rishit Bansal wrote: > Hi, > > On 02/02/23 18:13, Hans de Goede wrote: <snip> >> I have been thinking a bit about this and I still think that having separate per-zone >> files would be better. You can speedup things about 2x by only doing the call to read >> the buffer once and cache the result. At least assuming the non kbd zone related bits >> of the buffer never change (which should be easy enough to check). >> >> Actually my "thinking about this" includes a new alternate proposal. Rather then >> making up our own userspace API, as I did for the logitech 510 USB keyboard >> new support for multi-color backlights really should use the new standardized >> multi-color LED API: >> >> https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/includ >> >> I have been thinking about how to use this with a 4 zone keyboard and I believe >> that the best way to do that is to: >> >> >> 1. Forget about the global on/off control, individual zones can be turned off by >> setting the brightness of the zone to 0. >> >> This does require the driver to at least turn on the global control once, or >> you could: >> >> 1a) cache the global control value >> 1b) on zone changes check if all zones are off, if they are all off, use the >> global control to turn everything off, else turn the global control on; >> and only make the actual WMI call for this if the global control state >> changes vs the last cached value >> >> 2. Create 4 separate multi-color LED sysfs devices for each zone: >> >> /sys/class/leds/hp_omen::kbd_backlight-zone1/ >> /sys/class/leds/hp_omen::kbd_backlight-zone2/ >> /sys/class/leds/hp_omen::kbd_backlight-zone3/ >> /sys/class/leds/hp_omen::kbd_backlight-zone4/ >> >> This way we are using standard existing userspace APIs rather then inventing >> new userspace API which IMHO is a much better approach. >> >> Note this is just a suggestion, if you disagree (and can motivate >> why you think this is a bad idea) please do speak up about this. >> >> And please let me know if you need any help with converting the code >> to the ed-class-multicolor inetnal kernel APIs there are not that >> much users yet, so I have been unable to find a good example to >> point you to. >> >> A downside of this is that it lacks e.g. support in upower. But the >> kbd_backlight code in upower needs work anyways. E.g. upower does not >> work with backlit USB keyboards if these are plugged in after boot, >> or unplugged + re-plugged after boot. So someone really needs to >> spend some time to improve the upower keyboard backlight code anyways. >> >> Regards, >> >> Hans >> > > I agree the multi-color class is the correct thing to use here, but I am not completely sure if we should have multiple files in /sys/class/leds with the string "kbd_backlight" in them. UPower seems to take the first occurence of kbd_backlight and assume that's the keyboard backlight (https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L263-L269). I completely agree that this implementation needs more work on it, but it may have unintended consequences with software that uses UPower's kbd_backlight to control the keyboard. > > For example, Ubuntu (and most gnome based distros) by default ships with gnome-settings-daemon, which by default attempts to dim the keyboard backlight after a short duration when on the "Low Power" ACPI platform profile. (https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/power/gsd-power-manager.c#L1671). This was currently working as intended with the v2 patch, but if we introduce 4 different files for each zone, this may start dimming only one keyboard zone on low power instead of all 4 of them, which is certainly not intended. There are also multiple projects (mostly gnome extensions) that interact with UPower which might also function incorrectly in other ways. I don't think we should release a feature in the driver which caused unintended consequences like the ones mentioned, especially if the software is popular. What is your opinion on this? I was hoping / expecting that using $foo::kbd_backlight-$postfix would make current upower code ignore the LED class devices, but you are right, upower does not parse the string and then checks that the part after the last ':' is kbd_backlight it simply does a strstr for kbd_backlight in the LED class-device's name. So it would indeed pick one zone and use that. So one thing which we could do is change the name to e.g. : /sys/class/leds/hp_omen::kbd_zoned_backlight-1/ /sys/class/leds/hp_omen::kbd_zoned_backlight-2/ /sys/class/leds/hp_omen::kbd_zoned_backlight-3/ /sys/class/leds/hp_omen::kbd_zoned_backlight-4/ To make upower ignore all zones (until it learns about such setups with new code). > One alternative I can think of to have the "best of both worlds" (maintain support with Upower, and conform with the muti-color led specification), is to use the multi-color led class, and put all the indexes/brightness under one file. (Please correct me if the multi led specification does not allow this, but I don't see any limitation for having indexes other then just "red", "green" and "blue"): > > > $ cat /sys/class/leds/hp_omen::kbd_backlight/multi_index > > zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue zone_3_red zone_3_green zone_3_blue zone_4_red zone_4_green zone_4_blue > > > And we can set it accordingly by doing: > > $ echo 255 255 255 255 255 255 255 255 255 255 255 255 > /sys/class/leds/hp_omen::kbd_backlight/multi_intensity > > > And then I can use "led_mc_calc_color_components" when the brightness is changed to directly compute the brightness of each index value and pass it to the keyboard through the WMI method. > > > I know this suggestion goes back to us putting the all zones under a single file (sort of, we are atleast a bit closer to atleast following a spec now), but what are your thoughts on doing it this way with multi_index instead? That is quite an interesting proposal, it would still make the current kbd-backlight dimming in g-s-d + upower work and it means we only need 1 WMI call for changing all the zones, so this nicely matches with the actual firmware-API for this. So yes lets go this route, that seems the best way to do this to me. Note can you also add a separate patch to document the uses of: zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue ... As the way to set per-zone colors for RGB backlight keyboards with zones ? Either in: Documentation/ABI/testing/sysfs-class-led-multicolor or in: Documentation/leds/leds-class-multicolor.rst ? The idea here is to have a standard way of doing this to refer to if / when support for more zoned rgb backlight keyboards gets added to the kernel. I have added Dan Murphy, who is listed as contact for this in: Documentation/ABI/testing/sysfs-class-led-multicolor to the Cc. Regards, Hans >>>>> + >>>>> + >>>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c >>>>> index 0a99058be813..f86cb7feaad4 100644 >>>>> --- a/drivers/platform/x86/hp/hp-wmi.c >>>>> +++ b/drivers/platform/x86/hp/hp-wmi.c >>>>> @@ -27,6 +27,7 @@ >>>>> #include <linux/rfkill.h> >>>>> #include <linux/string.h> >>>>> #include <linux/dmi.h> >>>>> +#include <linux/leds.h> >>>>> MODULE_AUTHOR("Matthew Garrett <mjg59@xxxxxxxxxxxxx>"); >>>>> MODULE_DESCRIPTION("HP laptop WMI hotkeys driver"); >>>>> @@ -136,6 +137,7 @@ enum hp_wmi_command { >>>>> HPWMI_WRITE = 0x02, >>>>> HPWMI_ODM = 0x03, >>>>> HPWMI_GM = 0x20008, >>>>> + HPWMI_KB = 0x20009, >>>>> }; >>>>> enum hp_wmi_hardware_mask { >>>>> @@ -254,6 +256,9 @@ static const char * const tablet_chassis_types[] = { >>>>> #define DEVICE_MODE_TABLET 0x06 >>>>> +#define OMEN_ZONE_COLOR_OFFSET 0x19 >>>>> +#define OMEN_ZONE_COLOR_LEN 0x0c >>>>> + >>>>> /* map output size to the corresponding WMI method id */ >>>>> static inline int encode_outsize_for_pvsz(int outsize) >>>>> { >>>>> @@ -734,12 +739,56 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr, >>>>> return count; >>>>> } >>>>> +static ssize_t zone_colors_show(struct device *dev, >>>>> + struct device_attribute *attr, char *buf) >>>>> +{ >>>>> + u8 val[128]; >>>>> + >>>>> + int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val, >>>>> + zero_if_sup(val), sizeof(val)); >>>>> + >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + memcpy(buf, &val[OMEN_ZONE_COLOR_OFFSET], OMEN_ZONE_COLOR_LEN); >>>>> + >>>>> + return OMEN_ZONE_COLOR_LEN; >>>>> +} >>>>> + >>>>> +static ssize_t zone_colors_store(struct device *dev, >>>>> + struct device_attribute *attr, >>>>> + const char *buf, size_t count) >>>>> +{ >>>>> + u8 val[128]; >>>>> + int ret; >>>>> + >>>>> + ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val, >>>>> + zero_if_sup(val), sizeof(val)); >>>>> + >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + if (count != OMEN_ZONE_COLOR_LEN) >>>>> + return -1; >>>>> + >>>>> + memcpy(&val[OMEN_ZONE_COLOR_OFFSET], buf, count); >>>>> + >>>>> + ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_KB, &val, sizeof(val), >>>>> + 0); >>>>> + >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return OMEN_ZONE_COLOR_LEN; >>>>> +} >>>>> + >>>>> static DEVICE_ATTR_RO(display); >>>>> static DEVICE_ATTR_RO(hddtemp); >>>>> static DEVICE_ATTR_RW(als); >>>>> static DEVICE_ATTR_RO(dock); >>>>> static DEVICE_ATTR_RO(tablet); >>>>> static DEVICE_ATTR_RW(postcode); >>>>> +static DEVICE_ATTR_RW(zone_colors); >>>>> static struct attribute *hp_wmi_attrs[] = { >>>>> &dev_attr_display.attr, >>>>> @@ -752,6 +801,12 @@ static struct attribute *hp_wmi_attrs[] = { >>>>> }; >>>>> ATTRIBUTE_GROUPS(hp_wmi); >>>>> +static struct attribute *omen_kbd_led_attrs[] = { >>>>> + &dev_attr_zone_colors.attr, >>>>> + NULL, >>>>> +}; >>>>> +ATTRIBUTE_GROUPS(omen_kbd_led); >>>>> + >>>>> static void hp_wmi_notify(u32 value, void *context) >>>>> { >>>>> struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL }; >>>>> @@ -853,6 +908,10 @@ static void hp_wmi_notify(u32 value, void *context) >>>>> case HPWMI_PROXIMITY_SENSOR: >>>>> break; >>>>> case HPWMI_BACKLIT_KB_BRIGHTNESS: >>>>> + input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true); >>>>> + input_sync(hp_wmi_input_dev); >>>>> + input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false); >>>>> + input_sync(hp_wmi_input_dev); >>>>> break; >>>>> case HPWMI_PEAKSHIFT_PERIOD: >>>>> break; >>>>> @@ -1294,6 +1353,60 @@ static int thermal_profile_setup(void) >>>>> static int hp_wmi_hwmon_init(void); >>>>> +static enum led_brightness get_omen_backlight_brightness(struct led_classdev *cdev) >>>>> +{ >>>>> + u8 val; >>>>> + >>>>> + int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val)); >>>>> + >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return (val & 0x80) ? LED_ON : LED_OFF; >>>>> +} >>>>> + >>>>> +static void set_omen_backlight_brightness(struct led_classdev *cdev, enum led_brightness value) >>>>> +{ >>>>> + char buffer[4] = { (value == LED_OFF) ? 0x64 : 0xe4, 0, 0, 0 }; >>>>> + >>>>> + hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_KB, &buffer, >>>>> + sizeof(buffer), 0); >>>>> +} >>>>> + >>>>> +static struct led_classdev omen_kbd_led = { >>>>> + .name = "hp_omen::kbd_backlight", >>>>> + .brightness_set = set_omen_backlight_brightness, >>>>> + .brightness_get = get_omen_backlight_brightness, >>>>> + .max_brightness = 1, >>>>> + .groups = omen_kbd_led_groups, >>>>> +}; >>>>> + >>>>> +static bool is_omen_lighting_supported(void) >>>>> +{ >>>>> + u8 val; >>>>> + >>>>> + int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val)); >>>>> + >>>>> + if (ret) >>>>> + return false; >>>>> + >>>>> + return (val & 1) == 1; >>>>> +} >>>>> + >>>>> +static int omen_backlight_init(struct device *dev) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE); >>>>> + >>>>> + ret = devm_led_classdev_register(dev, &omen_kbd_led); >>>>> + >>>>> + if (ret < 0) >>>>> + return -1; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int __init hp_wmi_bios_setup(struct platform_device *device) >>>>> { >>>>> int err; >>>>> @@ -1321,6 +1434,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device) >>>>> thermal_profile_setup(); >>>>> + if (is_omen_lighting_supported()) >>>>> + omen_backlight_init(&device->dev); >>>>> + >>>>> return 0; >>>>> } >>>>> >
