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

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

On 9/8/26 10:45, Jani Nikula wrote:
> On Mon, 07 Sep 2026, Mario Limonciello <mario.limonciello@amd.com> wrote:
>> 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>
>>
>> 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 (AMD) <superm1@kernel.org>
>>   - rebase
>>   - Use guard(mutex)
>>
>> V4: Mario Limonciello (AMD) <superm1@kernel.org>
>>   - Adjust return type for backlight_set_brightness() to return errors
>>   - Stop clamping in backlight_set_brightness()
>>   - Drop backlight_device_lookup()
>>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>>   drivers/video/backlight/backlight.c | 66 +++++++++++++++++++++++++++++
>>   include/linux/backlight.h           | 45 ++++++++++++++++++++
>>   2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 4401f6294ccc8..1c700c1e77c3f 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -126,6 +126,9 @@ static void backlight_generate_event(struct backlight_device *bd,
>>   	case BACKLIGHT_UPDATE_HOTKEY:
>>   		envp[0] = "SOURCE=hotkey";
>>   		break;
>> +	case BACKLIGHT_UPDATE_DRM:
>> +		envp[0] = "SOURCE=drm";
>> +		break;
>>   	default:
>>   		envp[0] = "SOURCE=unknown";
>>   		break;
>> @@ -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;
>> +
>>   	rc = kstrtoul(buf, 0, &brightness);
>>   	if (rc)
>>   		return rc;
>> @@ -514,6 +531,41 @@ static int devm_backlight_device_match(struct device *dev, void *res,
>>   	return *r == data;
>>   }
>>   
>> +/**
>> + * 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.
>> + * A uevent notification is sent with the reason set to @reason.
>> + * Return: 0 if successfully notified, -EINVAL for invalid values, -ENXIO if the
>> + * device has no ops (e.g. it is being unregistered)
>> + */
>> +int backlight_set_brightness(struct backlight_device *bd, unsigned int value,
>> +			      enum backlight_update_reason reason)
>> +{
>> +	int rc = -ENXIO;
>> +
>> +	scoped_guard(mutex, &bd->ops_lock) {
>> +		if (bd->ops) {
>> +			if (value > bd->props.max_brightness)
>> +				return -EINVAL;
>> +
>> +			dev_dbg(&bd->dev, "set brightness to %u\n", value);
>> +			bd->props.brightness = value;
>> +			rc = backlight_update_status(bd);
>> +		}
>> +	}
>> +	if (rc == 0)
>> +		backlight_generate_event(bd, reason);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(backlight_set_brightness);
>> +
>>   /**
>>    * backlight_register_notifier - get notified of backlight (un)registration
>>    * @nb: notifier block with the notifier to call on backlight (un)registration
>> @@ -548,6 +600,20 @@ int backlight_unregister_notifier(struct notifier_block *nb)
>>   }
>>   EXPORT_SYMBOL(backlight_unregister_notifier);
>>   
>> +/**
>> + * backlight_notify_brightness - notify brightness change to listeners
>> + * @bd: backlight device that changed
>> + *
>> + * Notify registered listeners that the backlight brightness has changed.
>> + * This is called automatically after successful brightness updates.
>> + */
>> +void backlight_notify_brightness(struct backlight_device *bd)
>> +{
>> +	blocking_notifier_call_chain(&backlight_notifier,
>> +				     BACKLIGHT_BRIGHTNESS_CHANGED, bd);
>> +}
>> +EXPORT_SYMBOL(backlight_notify_brightness);
>> +
>>   /**
>>    * devm_backlight_device_register - register a new backlight device
>>    * @dev: the device to register
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index 015183d129f96..fac1acbc698a1 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -31,6 +31,12 @@ enum backlight_update_reason {
>>   	 * @BACKLIGHT_UPDATE_SYSFS: The backlight was updated using sysfs.
>>   	 */
>>   	BACKLIGHT_UPDATE_SYSFS,
>> +
>> +	/**
>> +	 * @BACKLIGHT_UPDATE_DRM: The backlight was updated from DRM, i.e. through
>> +	 * a connector LUMINANCE property rather than the legacy sysfs interface.
>> +	 */
>> +	BACKLIGHT_UPDATE_DRM,
>>   };
>>   
>>   /**
>> @@ -82,6 +88,11 @@ enum backlight_notification {
>>   	 * @BACKLIGHT_UNREGISTERED: The backlight revice is unregistered.
>>   	 */
>>   	BACKLIGHT_UNREGISTERED,
>> +
>> +	/**
>> +	 * @BACKLIGHT_BRIGHTNESS_CHANGED: The backlight brightness has changed.
>> +	 */
>> +	BACKLIGHT_BRIGHTNESS_CHANGED,
>>   };
>>   
>>   /** enum backlight_scale - the type of scale used for brightness values
>> @@ -310,8 +321,23 @@ struct backlight_device {
>>   	 * @use_count: The number of unblanked displays.
>>   	 */
>>   	int use_count;
>> +
>> +	/**
>> +	 * @drm_takeover: Number of luminance-aware DRM clients that have
>> +	 * taken over brightness control of this device. When non-zero,
>> +	 * writes to the legacy sysfs ``brightness`` attribute return
>> +	 * ``-EBUSY``. Managed by the DRM backlight helpers.
>> +	 */
>> +	atomic_t drm_takeover;
>>   };
>>   
>> +/* Forward declaration for backlight_update_status */
>> +#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
> 
> I think IS_REACHABLE() is almost always a mistake. It's a hack to avoid
> link errors when something in a module needs something that is
> built-in. It's a combo that should be handled at Kconfig level, not at
> build level.
> 
> BR,
> Jani.

That came from this feedback from Thomas Zimmerman (I originally had it 
as select:

https://lore.kernel.org/dri-devel/8224920a-3a10-4400-9503-f014b7059329@suse.de/

I was trying to use IS_REACHABLE to make it optional, but it sounds like 
that's not the right direction then?

> 
> 
>> +void backlight_notify_brightness(struct backlight_device *bd);
>> +#else
>> +static inline void backlight_notify_brightness(struct backlight_device *bd) {}
>> +#endif
>> +
>>   /**
>>    * backlight_update_status - force an update of the backlight device status
>>    * @bd: the backlight device
>> @@ -325,6 +351,10 @@ static inline int backlight_update_status(struct backlight_device *bd)
>>   		ret = bd->ops->update_status(bd);
>>   	mutex_unlock(&bd->update_lock);
>>   
>> +	/* Notify DRM and other listeners that brightness changed */
>> +	if (ret == 0)
>> +		backlight_notify_brightness(bd);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -431,6 +461,21 @@ static inline void backlight_notify_blank_all(struct device *display_dev,
>>   { }
>>   #endif
>>   
>> +int 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);
>> +}
>> +
>>   #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
>>   
>>   /**
>