Re: [PATCH v3 4/8] DRM: Add support for client and driver indicating support for luminance

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 4/25/26 00:09, Mario Limonciello wrote:
> The legacy backlight control interface can only be disabled when both
> the client and driver have agreed that the luminance can be set during
> a modeset. Add capability for the client to register and for the driver
> to indicate support.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 24 ++++++++++++++++++++++++
>   drivers/gpu/drm/drm_connector.c   |  4 ++--
>   drivers/gpu/drm/drm_ioctl.c       | 10 ++++++++++
>   include/drm/drm_connector.h       |  5 +++++
>   include/drm/drm_drv.h             |  7 +++++++
>   include/drm/drm_file.h            |  8 ++++++++
>   include/uapi/drm/drm.h            | 10 ++++++++++
>   7 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 5bd5bf6661df7..de218206cef7e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -30,6 +30,7 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_atomic_uapi.h>
> +#include <drm/drm_backlight.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_drv.h>
> @@ -935,6 +936,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   		state->privacy_screen_sw_state = val;
>   	} else if (property == connector->broadcast_rgb_property) {
>   		state->hdmi.broadcast_rgb = val;
> +	} else if (property == config->luminance_property) {
> +		state->luminance = val;
> +		/* Update hardware backlight only when DPMS is ON.
> +		 * Property value is always updated to remember the user's
> +		 * desired brightness.
> +		 */
> +		if (connector->dpms == DRM_MODE_DPMS_ON)
> +			drm_backlight_set_luminance(connector->backlight, val);
>   	} else if (connector->funcs->atomic_set_property) {
>   		return connector->funcs->atomic_set_property(connector,
>   				state, property, val);
> @@ -1020,6 +1029,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>   		*val = state->privacy_screen_sw_state;
>   	} else if (property == connector->broadcast_rgb_property) {
>   		*val = state->hdmi.broadcast_rgb;
> +	} else if (property == config->luminance_property) {
> +		*val = state->luminance;

I think this is a bad idea to have two "source of truth" for the 
luminance value. Is there a reason to not get the backlight value 
directly from the backlight driver?

>   	} else if (connector->funcs->atomic_get_property) {
>   		return connector->funcs->atomic_get_property(connector,
>   				state, property, val);
> @@ -1126,8 +1137,21 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>   	if (connector->dpms == mode)
>   		goto out;
>   
> +	/* Handle backlight brightness coordination with DPMS state changes */
> +	if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
> +		/* DPMS ON -> OFF: dim backlight to 0 to save power */
> +		drm_backlight_set_luminance(connector->backlight, 0);
> +	}
> +
>   	connector->dpms = mode;
>   
> +	/* DPMS OFF -> ON: restore brightness to property value */
> +	if (old_mode == DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_ON &&
> +	    connector->state) {
> +		drm_backlight_set_luminance(connector->backlight,
> +					    connector->state->luminance);
> +	}
> +

According to the documentation, DPMS is a deprecated property and most 
of the work should be handled using the crtc->active property.

I think this will not change the backlight of other connector using the 
same CRTC (CRTC outputs on connector1 and connector2, userspace change 
DPMS only for connector1, for me it makes sense to change connector1 and 
connector2).

After looking a bit more, I think you need to keep this code, but:
- add something to also disable backlight of other connector using the 
same crtc
- handle the backlight when CRTC is disconnected from the connector (i.e 
when you set CRTC=0 on a connector)

>   	crtc = connector->state->crtc;
>   	if (!crtc)
>   		goto out;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index d76878548728a..5de33fc259b26 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -3268,9 +3268,9 @@ int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>   	/* Do DPMS ourselves */
>   	if (property == connector->dev->mode_config.dpms_property) {
>   		ret = (*connector->funcs->dpms)(connector, (int)value);
> -	} else if (property == config->brightness_property) {
> +	} else if (property == config->luminance_property) {

Can you directly name it luminance in the previous patch?

>   		if (connector->backlight && connector->dpms == DRM_MODE_DPMS_ON)
> -			drm_backlight_set_brightness(connector->backlight,
> +			drm_backlight_set_luminance(connector->backlight,
>   						     value);
>   		ret = 0;
>   	} else if (connector->funcs->set_property)
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ff193155129e7..b4435c2bd6091 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -28,6 +28,7 @@
>    * OTHER DEALINGS IN THE SOFTWARE.
>    */
>   
> +#include "drm/drm.h"
>   #include <linux/export.h>
>   #include <linux/nospec.h>
>   #include <linux/pci.h>
> @@ -380,6 +381,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>   			return -EINVAL;
>   		file_priv->plane_color_pipeline = req->value;
>   		break;
> +	case DRM_CLIENT_CAP_LUMINANCE:
> +		if (!drm_core_check_feature(dev, DRIVER_CONNECTOR_LUMINANCE))
> +			return -EOPNOTSUPP;
> +		if (!file_priv->atomic)
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->supports_luminance_control = req->value;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 10daf088b8f1a..762a9e2ef6e30 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1209,6 +1209,11 @@ struct drm_connector_state {
>   	 * @drm_atomic_helper_connector_hdmi_check().
>   	 */
>   	struct drm_connector_hdmi_state hdmi;
> +
> +	/**
> +	 * @luminance: Luminance for the connector
> +	 */
> +	u16 luminance;
>   };
>   
>   struct drm_connector_hdmi_audio_funcs {
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 42fc085f986de..a6b668cb68c5e 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -123,6 +123,13 @@ enum drm_driver_feature {
>   	 */
>   	DRIVER_CURSOR_HOTSPOT           = BIT(9),
>   
> +	/**
> +	 * @DRIVER_CONNECTOR_LUMINANCE:
> +	 *
> +	 * Driver supports luminance control on a per connector basis.
> +	 */
> +	DRIVER_CONNECTOR_LUMINANCE           = BIT(10),
> +
>   	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
>   
>   	/**
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 6ee70ad65e1fd..0bb1e53f36bec 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -248,6 +248,14 @@ struct drm_file {
>   	 */
>   	bool supports_virtualized_cursor_plane;
>   
> +	/**
> +	 * @supports_luminance_control:
> +	 *
> +	 * This client is capable of setting the luminance for connectors.
> +	 *
> +	 */
> +	bool supports_luminance_control;
> +
>   	/**
>   	 * @master:
>   	 *
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 27cc159c1d275..b5e6d940f2816 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -921,6 +921,16 @@ struct drm_get_cap {
>    */
>   #define DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE	7
>   
> +/**
> + * DRM_CLIENT_CAP_LUMINANCE
> + *
> + * If set to 1, legacy sysfs interface for controlling backlight brightness will
> + * be disabled.  The client will include luminance values as part of the modeset.
> +
> + * This capability is supported starting in kernel 7.2
> + */
> +#define DRM_CLIENT_CAP_LUMINANCE		8
> +
>   /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>   struct drm_set_client_cap {
>   	__u64 capability;