Message-ID: <544484b9-c0ac-2fd0-1f41-8fa94cb94d4b@redhat.com>
Date:   Mon, 6 Feb 2023 15:32:54 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Rishit Bansal <rishitbansal0@...il.com>,
        Mark Gross <markgross@...nel.org>,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Dan Murphy <dmurphy@...com>
Subject: API for setting colors of RGB backlit keyboard zones (was [PATCH V3]
 platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods)

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@...f.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;
>>>>>    }
>>>>>    
> 

Powered by blists - more mailing lists