Re: [PATCH v8 02/14] backlight: add kernel-internal backlight API

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 22.09.26 um 13:34 schrieb Mario Limonciello:
>
>
> On 9/22/26 03:49, Javier Martinez Canillas wrote:
>> On Tue, Sep 22, 2026 at 10:24 AM Thomas Zimmermann 
>> <tzimmermann@suse.de> wrote:
>>>
>>> Hi Mario
>>>
>>> Am 08.09.26 um 06:40 schrieb Mario Limonciello:
>>>> So far backlights have only been controlled via sysfs. However, 
>>>> sysfs is
>>>> not a proper user-space API for runtime modifications, and never was
>>>> intended to provide such. The DRM drivers are now prepared to provide
>>>> such a backlight link so user-space can control backlight via DRM
>>>> connector properties. This allows us to employ the same 
>>>> access-management
>>>> we use for mode-setting.
>>>>
>>>> This patch adds a few kernel-internal backlight helpers so we can 
>>>> modify
>>>> backlights from within DRM, a brightness-changed notification, and a
>>>> per-device takeover count so that legacy sysfs writes can be inhibited
>>>> (-EBUSY) while a luminance-aware DRM client is in control.
>>>>
>>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>>>
>>
>> [...]
>>
>>>> @@ -150,6 +153,13 @@ static ssize_t bl_power_store(struct device 
>>>> *dev, struct device_attribute *attr,
>>>>        struct backlight_device *bd = to_backlight_device(dev);
>>>>        unsigned long power, old_power;
>>>>
>>>> +     /* A luminance-aware DRM client has taken over this 
>>>> backlight; the
>>>> +      * legacy sysfs interface is disabled until the last such client
>>>> +      * goes away.
>>>> +      */
>>>> +     if (atomic_read(&bd->drm_takeover) > 0)
>>>> +             return -EBUSY;
>>>> +
>>>>        rc = kstrtoul(buf, 0, &power);
>>>>        if (rc)
>>>>                return rc;
>>>> @@ -214,6 +224,13 @@ static ssize_t brightness_store(struct device 
>>>> *dev,
>>>>        struct backlight_device *bd = to_backlight_device(dev);
>>>>        unsigned long brightness;
>>>>
>>>> +     /* A luminance-aware DRM client has taken over this 
>>>> backlight; the
>>>> +      * legacy sysfs interface is disabled until the last such client
>>>> +      * goes away.
>>>> +      */
>>>> +     if (atomic_read(&bd->drm_takeover) > 0)
>>>> +             return -EBUSY;
>>>
>>> Can you really do that?  AFAIU sysfs is now a de-facto uapi for 
>>> backlights.
>>
>> And regardless if this can be done or not, I belive that would be
>> better if this is something that drivers decide to do and just return
>> -EBUSY from their struct backlight_ops..update_status.
>>
>> Ideally the sysfs interface should continue to be work as Thomas said
>> and a write could trigger a mode set for example, but I don't know if
>> that is feasible to do due locking.
>>
>
> Well the problem ends up being that something can change brightness 
> behind the compositor's back if you leave sysfs available.
>
> The whole design here is to put the compositor in control.  For 
> example if compositor wants to enforce brightness to be a certain 
> value when certain content is being displayed.

And as I mentioned, it might not be that easy. Sysfs has been the entry 
point to backlight brightness so far AFAIU.

Did you consider treating this similar to display hotplugging?  When the 
user plug a display in or out, the DRM driver sends a message to the 
compositor in user space. [1]  I think it might be possible to do a 
similar thing for changes to brightness.

[1] 
https://elixir.bootlin.com/linux/v7.2.6/source/drivers/gpu/drm/drm_probe_helper.c#L747

Best regards
Thomas



-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)