Re: [PATCH v3 1/8] backlight: add kernel-internal backlight API

[Mario Limonciello <mario.limonciello@amd.com>]

On 5/4/26 08:55, Louis Chauvet wrote:
> [You don't often get email from louis.chauvet@bootlin.com. Learn why 
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 4/25/26 00:09, Mario Limonciello wrote:
>> From: David Herrmann <dh.herrmann@gmail.com>
>>
>> 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 few kernel-internal backlight helpers so we can modify
>> backlights from within DRM.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>
>> V2: Marta Lofstedt <marta.lofstedt@intel.com>
>> - rebase
>> - minor edit for checkpatch warning
>>
>> Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com>
>>
>> V3: Mario Limonciello <mario.limonciello@amd.com>
>>   - rebase
>>   - Use guard(mutex)
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/video/backlight/backlight.c | 60 +++++++++++++++++++++++++++++
>>   include/linux/backlight.h           | 16 ++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/ 
>> backlight/backlight.c
>> index ab87a5e3dbf70..c3673bee6d9cf 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -513,6 +513,66 @@ static int devm_backlight_device_match(struct 
>> device *dev, void *res,
>>       return *r == data;
>>   }
>>
>> +/**
>> + * backlight_device_lookup - find a backlight device
>> + * @name: sysname of the backlight device
>> + *
>> + * @return Reference to the backlight device, NULL if not found.
>> + *
>> + * This searches through all registered backlight devices for a 
>> device with the
>> + * given device name. In case none is found, NULL is returned, 
>> otherwise a
>> + * new reference to the backlight device is returned. You must drop this
>> + * reference via backlight_device_unref() once done.
>> + * Note that the devices might get unregistered at any time. You need 
>> to lock
>> + * around this lookup and inside of your backlight-notifier if you 
>> need to know
>> + * when a device gets unregistered.
>> + *
>> + * This function can be safely called from IRQ context.
>> + */
>> +struct backlight_device *backlight_device_lookup(const char *name)
>> +{
>> +     struct backlight_device *bd;
>> +     const char *t;
>> +
>> +     guard(mutex)(&backlight_dev_list_mutex);
>> +     list_for_each_entry(bd, &backlight_dev_list, entry) {
>> +             t = dev_name(&bd->dev);
>> +             if (t && !strcmp(t, name)) {
>> +                     backlight_device_ref(bd);
>> +                     return bd;
>> +             }
>> +     }
>> +
>> +     return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(backlight_device_lookup);
>>
> 
> Hello,
> 
> I think this function can be repalced with backlight_device_get_by_name.

Yes; good call.

> 
>> +/**
>> + * backlight_set_brightness - set brightness on a backlight device
>> + * @bd: backlight device to operate on
>> + * @value: brightness value to set on the device
>> + * @reason: backlight-change reason to use for notifications
>> + *
>> + * This is the in-kernel API equivalent of writing into the 
>> 'brightness' sysfs
>> + * file. It calls into the underlying backlight driver to change the 
>> brightness
>> + * value. The value is clamped according to device bounds.
>> + * A uevent notification is sent with the reason set to @reason.
>> + */
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned 
>> int value,
>> +                           enum backlight_update_reason reason)
>> +{
>> +     guard(mutex)(&bd->ops_lock);
>> +     if (bd->ops) {
>> +             value = clamp(value, 0U,
>> +                           (unsigned int)bd->props.max_brightness);
> 
> Why did you use a clamping here? I think it is better to return error
> instead.
> 

This is called from a work queue.  This is the call path:

__drm_backlight_worker().
-> __drm_backlight_schedule()
->-> __drm_backlight_prop_changed()
->->-> drm_backlight_set_luminance().

So - I suppose that actually what you are suggesting is to plumb an 
error all the way from the work queue up to all the callers.  That might 
for a change to make things synchronous that weren't 'intended' to be 
synchronous.

Maybe a better solution is to try to look at the max brightness 
'directly' in drm_backlight_set_luminance() and then reject it before 
going down the work queue path.

Thoughts?

>> +             dev_dbg(&bd->dev, "set brightness to %u\n", value);
>> +             bd->props.brightness = value;
>> +             backlight_update_status(bd);
>> +     }
>> +     backlight_generate_event(bd, reason);
>> +}
>> +EXPORT_SYMBOL_GPL(backlight_set_brightness);
>>
> 
> I think this could be nice to update backlight_device_set_brightness to
> avoid code duplication:
> 
> int backlight_device_set_brightness(...) {
>         return backlight_set_brightness(..., BACKLIGHT_UPDATE_SYSFS);
> }

OK.

> 
>>   /**
>>    * backlight_register_notifier - get notified of backlight 
>> (un)registration
>>    * @nb: notifier block with the notifier to call on backlight 
>> (un)registration
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index d905173c7f73c..7e4fee65fddd9 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -429,6 +429,22 @@ static inline void 
>> backlight_notify_blank_all(struct device *display_dev,
>>   { }
>>   #endif
>>
>> +struct backlight_device *backlight_device_lookup(const char *name);
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned 
>> int value,
>> +                           enum backlight_update_reason reason);
>> +
>> +static inline void backlight_device_ref(struct backlight_device *bd)
>> +{
>> +     if (bd)
>> +             get_device(&bd->dev);
>> +}
>> +
>> +static inline void backlight_device_unref(struct backlight_device *bd)
>> +{
>> +     if (bd)
>> +             put_device(&bd->dev);
>> +}
>> +
> Most of the kernel use _put and _get functions, I think it could be nice
> to keep the same naming.

OK.