On 5/4/26 19:55, Mario Limonciello wrote:
On 5/4/26 08:55, Louis Chauvet wrote:
[You don't often get email from [email protected]. Learn why
this is important at https://aka.ms/LearnAboutSenderIdentification ]
On 4/25/26 00:09, Mario Limonciello wrote:
From: David Herrmann <[email protected]>
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 <[email protected]>
V2: Marta Lofstedt <[email protected]>
- rebase
- minor edit for checkpatch warning
Signed-off-by: Marta Lofstedt <[email protected]>
V3: Mario Limonciello <[email protected]>
- rebase
- Use guard(mutex)
Signed-off-by: Mario Limonciello <[email protected]>
---
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.
You are right, this is not a good idea to make this synchronous.
After looking a bit more at the code, I think you can simply not clamp
at all and use a warn. In all cases:
- drm should never ask for a value outside 0..U16_MAX
- the conversion from 0..U16_MAX to 0..bd->props.max_brightness should
never be bigger than bd->props.max_brightness
So if you have to clamp, it means there is a bug somewhere in the process.
I imagine something like:
b_set_brightness(value)
if value > max:
return -EINVAL
[...]
__drm_backlight_worker()
r = b_set_brightness(new_value)
if r == -EINVAL: # This should never be the case, new_value should be
clamped to 0..max_value before
WARN("Invalid backlight value requested")
# maybe add a fallback to at least have a coherent on/off state and
avoid complete black screens
if new_value==0: b_set_brightness(0)
else: b_set_brightness(max_value)
__drm_backlight_prop_changed(u16 requested_value)
new_value = requested_value * max_value / U16_MAX
schedule_work(new_value)
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?
That also a solution, but I think it is better to have a clear interface
for drm_backlight_set_luminance and the backlight property:
drm_backlight_set_luminance must get a 0..U16_MAX value, any other value
is invalid.
But I have a question for this: is the scale of luminance linear or not?
I think this information should be given to userspace.
And if lunimance is not linear, I don't think you can easily scale
between 0..U16MAX to 0..max_value.
Same thing for backlight=0, IIRC sometimes the backlight driver switch
off the light, sometimes it is just the minimum luminosity.
I think that can be a topic for discussion at drm hackfest, but it may
also require more precise information / specification from the backlight
subsystem / to the userspace interface.
+ 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.
(note: this is only valid if you don't clamp in
backlight_set_brightness, current code return -EINVAL if it is outside
the range)
/**
* 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.
