Re: API for setting colors of RGB backlit keyboard zones (was [PATCH V3] platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods)

[Rishit Bansal <rishitbansal0@gmail.com>]

On 06/02/23 20:02, Hans de Goede wrote:
> Hi Rishit,
> 
> On 2/2/23 20:59, Rishit Bansal wrote:
>> Hi,
>>
>> On 02/02/23 18:13, Hans de Goede wrote:
> 
> 
>>> 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
> 
> ?

Sounds great! I'll make a v4 patch with this approach, and another patch 
for the zone documentation.


On a side note: While using this patch on my machine for a while, I also 
noticed another edge case where it would automatically turn on my 
backlight even if I explicitly turned it off using the hardware button 
(Fn+F4). I tracked this down to not supporting the 
"brightness_hw_change" 
(https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L297) 
attribute correctly, which upower uses to detect changes in the state of 
the backlight. I will also add a fix for this in my next patch.

(Sorry if this got sent twice, it seems an HTML tag made its way into 
the previous email and it got rejected by the mailing list>

> 
> 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@srcf.ucam.org>");
>>>>>>     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;
>>>>>>     }
>>>>>>     
>>
>