[PATCH] drm/amd: Move adaptive backlight modulation property to drm core

Mario Limonciello posted 1 patch 2 months, 3 weeks ago
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
include/drm/drm_connector.h                 |  8 +++
4 files changed, 80 insertions(+), 67 deletions(-)
[PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Mario Limonciello 2 months, 3 weeks ago
The adaptive backlight modulation property is supported on AMD hardware but
compositors should be aware of it in standard DRM property documentation.

Move the helper to create the property and documentation into DRM.

Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
 drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
 include/drm/drm_connector.h                 |  8 +++
 4 files changed, 80 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index f8b35c487b6c..3d840bef77bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1363,67 +1363,9 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
 	{ AMDGPU_FMT_DITHER_ENABLE, "on" },
 };
 
-/**
- * DOC: property for adaptive backlight modulation
- *
- * The 'adaptive backlight modulation' property is used for the compositor to
- * directly control the adaptive backlight modulation power savings feature
- * that is part of DCN hardware.
- *
- * The property will be attached specifically to eDP panels that support it.
- *
- * The property is by default set to 'sysfs' to allow the sysfs file 'panel_power_savings'
- * to be able to control it.
- * If set to 'off' the compositor will ensure it stays off.
- * The other values 'min', 'bias min', 'bias max', and 'max' will control the
- * intensity of the power savings.
- *
- * Modifying this value can have implications on color accuracy, so tread
- * carefully.
- */
-static int amdgpu_display_setup_abm_prop(struct amdgpu_device *adev)
-{
-	const struct drm_prop_enum_list props[] = {
-		{ ABM_SYSFS_CONTROL, "sysfs" },
-		{ ABM_LEVEL_OFF, "off" },
-		{ ABM_LEVEL_MIN, "min" },
-		{ ABM_LEVEL_BIAS_MIN, "bias min" },
-		{ ABM_LEVEL_BIAS_MAX, "bias max" },
-		{ ABM_LEVEL_MAX, "max" },
-	};
-	struct drm_property *prop;
-	int i;
-
-	if (!adev->dc_enabled)
-		return 0;
-
-	prop = drm_property_create(adev_to_drm(adev), DRM_MODE_PROP_ENUM,
-				"adaptive backlight modulation",
-				6);
-	if (!prop)
-		return -ENOMEM;
-
-	for (i = 0; i < ARRAY_SIZE(props); i++) {
-		int ret;
-
-		ret = drm_property_add_enum(prop, props[i].type,
-						props[i].name);
-
-		if (ret) {
-			drm_property_destroy(adev_to_drm(adev), prop);
-
-			return ret;
-		}
-	}
-
-	adev->mode_info.abm_level_property = prop;
-
-	return 0;
-}
-
 int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 {
-	int sz;
+	int ret, sz;
 
 	adev->mode_info.coherent_mode_property =
 		drm_property_create_range(adev_to_drm(adev), 0, "coherent", 0, 1);
@@ -1467,7 +1409,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 					 "dither",
 					 amdgpu_dither_enum_list, sz);
 
-	return amdgpu_display_setup_abm_prop(adev);
+	adev->mode_info.abm_level_property = drm_create_abm_property(adev_to_drm(adev));
+	if (IS_ERR(adev->mode_info.abm_level_property)) {
+		ret = PTR_ERR(adev->mode_info.abm_level_property);
+		adev->mode_info.abm_level_property = NULL;
+		return ret;
+	}
+
+	return 0;
 }
 
 void amdgpu_display_update_priority(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
index 2b1536a16752..dfa0d642ac16 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
@@ -54,11 +54,4 @@ int amdgpu_display_resume_helper(struct amdgpu_device *adev);
 int amdgpu_display_get_scanout_buffer(struct drm_plane *plane,
 				      struct drm_scanout_buffer *sb);
 
-#define ABM_SYSFS_CONTROL	-1
-#define ABM_LEVEL_OFF		0
-#define ABM_LEVEL_MIN		1
-#define ABM_LEVEL_BIAS_MIN	2
-#define ABM_LEVEL_BIAS_MAX	3
-#define ABM_LEVEL_MAX		4
-
 #endif
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea47..376169dac247 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2603,6 +2603,69 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
 	return 0;
 }
 
+/**
+ * DOC: integrated panel properties
+ *
+ * adaptive backlight modulation:
+ *	Adaptive backlight modulation (ABM) is a power savings feature that
+ *	dynamically adjusts the backlight brightness based on the content
+ *	displayed on the screen. By reducing the backlight brightness for
+ *	darker images and increasing it for brighter images, ABM helps to
+ *	conserve energy and extend battery life on devices with integrated
+ *	displays.  This feature is part of AMD DCN hardware.
+ *
+ *	sysfs
+ *		The ABM property is exposed to userspace via sysfs interface
+ *		located at 'amdgpu/panel_power_savings' under the DRM device.
+ *	off
+ *		Adaptive backlight modulation is disabled.
+ *	min
+ *		Adaptive backlight modulation is enabled at minimum intensity.
+ *	bias min
+ *		Adaptive backlight modulation is enabled at a more intense
+ *		level than 'min'.
+ *	bias max
+ *		Adaptive backlight modulation is enabled at a more intense
+ *		level than 'bias min'.
+ *	max
+ *		Adaptive backlight modulation is enabled at maximum intensity.
+ */
+struct drm_property *drm_create_abm_property(struct drm_device *dev)
+{
+	const struct drm_prop_enum_list props[] = {
+		{ ABM_SYSFS_CONTROL, "sysfs" },
+		{ ABM_LEVEL_OFF, "off" },
+		{ ABM_LEVEL_MIN, "min" },
+		{ ABM_LEVEL_BIAS_MIN, "bias min" },
+		{ ABM_LEVEL_BIAS_MAX, "bias max" },
+		{ ABM_LEVEL_MAX, "max" },
+	};
+	struct drm_property *prop;
+	int i;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+				"adaptive backlight modulation",
+				6);
+	if (!prop)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		int ret;
+
+		ret = drm_property_add_enum(prop, props[i].type,
+						props[i].name);
+
+		if (ret) {
+			drm_property_destroy(dev, prop);
+
+			return ERR_PTR(ret);
+		}
+	}
+
+	return prop;
+}
+EXPORT_SYMBOL(drm_create_abm_property);
+
 /**
  * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
  * @connector: connector to create the Colorspace property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d..644c0d49500f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2454,6 +2454,7 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
 					     struct drm_connector_state *new_state);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
+struct drm_property *drm_create_abm_property(struct drm_device *dev);
 int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
 					     u32 supported_colorspaces);
 int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
@@ -2563,4 +2564,11 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
 	drm_for_each_encoder_mask(encoder, (connector)->dev, \
 				  (connector)->possible_encoders)
 
+#define ABM_SYSFS_CONTROL	-1
+#define ABM_LEVEL_OFF		0
+#define ABM_LEVEL_MIN		1
+#define ABM_LEVEL_BIAS_MIN	2
+#define ABM_LEVEL_BIAS_MAX	3
+#define ABM_LEVEL_MAX		4
+
 #endif
-- 
2.51.2
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Simona Vetter 2 months, 3 weeks ago
On Wed, Nov 12, 2025 at 04:26:46PM -0600, Mario Limonciello wrote:
> The adaptive backlight modulation property is supported on AMD hardware but
> compositors should be aware of it in standard DRM property documentation.
> 
> Move the helper to create the property and documentation into DRM.
> 
> Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
>  drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
>  include/drm/drm_connector.h                 |  8 +++
>  4 files changed, 80 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index f8b35c487b6c..3d840bef77bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1363,67 +1363,9 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>  	{ AMDGPU_FMT_DITHER_ENABLE, "on" },
>  };
>  
> -/**
> - * DOC: property for adaptive backlight modulation
> - *
> - * The 'adaptive backlight modulation' property is used for the compositor to
> - * directly control the adaptive backlight modulation power savings feature
> - * that is part of DCN hardware.
> - *
> - * The property will be attached specifically to eDP panels that support it.
> - *
> - * The property is by default set to 'sysfs' to allow the sysfs file 'panel_power_savings'
> - * to be able to control it.
> - * If set to 'off' the compositor will ensure it stays off.
> - * The other values 'min', 'bias min', 'bias max', and 'max' will control the
> - * intensity of the power savings.
> - *
> - * Modifying this value can have implications on color accuracy, so tread
> - * carefully.
> - */
> -static int amdgpu_display_setup_abm_prop(struct amdgpu_device *adev)
> -{
> -	const struct drm_prop_enum_list props[] = {
> -		{ ABM_SYSFS_CONTROL, "sysfs" },
> -		{ ABM_LEVEL_OFF, "off" },
> -		{ ABM_LEVEL_MIN, "min" },
> -		{ ABM_LEVEL_BIAS_MIN, "bias min" },
> -		{ ABM_LEVEL_BIAS_MAX, "bias max" },
> -		{ ABM_LEVEL_MAX, "max" },
> -	};
> -	struct drm_property *prop;
> -	int i;
> -
> -	if (!adev->dc_enabled)
> -		return 0;
> -
> -	prop = drm_property_create(adev_to_drm(adev), DRM_MODE_PROP_ENUM,
> -				"adaptive backlight modulation",
> -				6);
> -	if (!prop)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < ARRAY_SIZE(props); i++) {
> -		int ret;
> -
> -		ret = drm_property_add_enum(prop, props[i].type,
> -						props[i].name);
> -
> -		if (ret) {
> -			drm_property_destroy(adev_to_drm(adev), prop);
> -
> -			return ret;
> -		}
> -	}
> -
> -	adev->mode_info.abm_level_property = prop;
> -
> -	return 0;
> -}
> -
>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  {
> -	int sz;
> +	int ret, sz;
>  
>  	adev->mode_info.coherent_mode_property =
>  		drm_property_create_range(adev_to_drm(adev), 0, "coherent", 0, 1);
> @@ -1467,7 +1409,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  					 "dither",
>  					 amdgpu_dither_enum_list, sz);
>  
> -	return amdgpu_display_setup_abm_prop(adev);
> +	adev->mode_info.abm_level_property = drm_create_abm_property(adev_to_drm(adev));
> +	if (IS_ERR(adev->mode_info.abm_level_property)) {
> +		ret = PTR_ERR(adev->mode_info.abm_level_property);
> +		adev->mode_info.abm_level_property = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  void amdgpu_display_update_priority(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 2b1536a16752..dfa0d642ac16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -54,11 +54,4 @@ int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>  int amdgpu_display_get_scanout_buffer(struct drm_plane *plane,
>  				      struct drm_scanout_buffer *sb);
>  
> -#define ABM_SYSFS_CONTROL	-1
> -#define ABM_LEVEL_OFF		0
> -#define ABM_LEVEL_MIN		1
> -#define ABM_LEVEL_BIAS_MIN	2
> -#define ABM_LEVEL_BIAS_MAX	3
> -#define ABM_LEVEL_MAX		4
> -
>  #endif
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea47..376169dac247 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2603,6 +2603,69 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +/**
> + * DOC: integrated panel properties

Thanks for doing this, but just moving the function isn't enough. Aside
from what Jani said there's a few more things that are more about the
technicallities of making good docs for uabi.

- We want proper kerneldoc for the function drm_create_abm_property() like
  for any other function the drm core/helper code exports to drivers.

- The property documentation needs to be moved (or included, but I think
  moving is better) so it shows up in the generated html docs at the right
  place with all the other connector properties.

- We need a mention from the 2nd place to the function (which should
  result in a working hyperlink in the generated docs, please check that
  by generating the docs and confirming with the output) so that driver
  authors can find this. Als the function needs to link to the enum (which
  also needs kerneldoc) and the other direction so that docs are easily
  discoverable.

Thanks a lot!

Cheers, Sima

> + *
> + * adaptive backlight modulation:
> + *	Adaptive backlight modulation (ABM) is a power savings feature that
> + *	dynamically adjusts the backlight brightness based on the content
> + *	displayed on the screen. By reducing the backlight brightness for
> + *	darker images and increasing it for brighter images, ABM helps to
> + *	conserve energy and extend battery life on devices with integrated
> + *	displays.  This feature is part of AMD DCN hardware.
> + *
> + *	sysfs
> + *		The ABM property is exposed to userspace via sysfs interface
> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
> + *	off
> + *		Adaptive backlight modulation is disabled.
> + *	min
> + *		Adaptive backlight modulation is enabled at minimum intensity.
> + *	bias min
> + *		Adaptive backlight modulation is enabled at a more intense
> + *		level than 'min'.
> + *	bias max
> + *		Adaptive backlight modulation is enabled at a more intense
> + *		level than 'bias min'.
> + *	max
> + *		Adaptive backlight modulation is enabled at maximum intensity.
> + */
> +struct drm_property *drm_create_abm_property(struct drm_device *dev)
> +{
> +	const struct drm_prop_enum_list props[] = {
> +		{ ABM_SYSFS_CONTROL, "sysfs" },
> +		{ ABM_LEVEL_OFF, "off" },
> +		{ ABM_LEVEL_MIN, "min" },
> +		{ ABM_LEVEL_BIAS_MIN, "bias min" },
> +		{ ABM_LEVEL_BIAS_MAX, "bias max" },
> +		{ ABM_LEVEL_MAX, "max" },
> +	};
> +	struct drm_property *prop;
> +	int i;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> +				"adaptive backlight modulation",
> +				6);
> +	if (!prop)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		int ret;
> +
> +		ret = drm_property_add_enum(prop, props[i].type,
> +						props[i].name);
> +
> +		if (ret) {
> +			drm_property_destroy(dev, prop);
> +
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	return prop;
> +}
> +EXPORT_SYMBOL(drm_create_abm_property);
> +
>  /**
>   * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
>   * @connector: connector to create the Colorspace property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..644c0d49500f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2454,6 +2454,7 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
>  bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
>  					     struct drm_connector_state *new_state);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> +struct drm_property *drm_create_abm_property(struct drm_device *dev);
>  int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
>  					     u32 supported_colorspaces);
>  int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
> @@ -2563,4 +2564,11 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
>  	drm_for_each_encoder_mask(encoder, (connector)->dev, \
>  				  (connector)->possible_encoders)
>  
> +#define ABM_SYSFS_CONTROL	-1
> +#define ABM_LEVEL_OFF		0
> +#define ABM_LEVEL_MIN		1
> +#define ABM_LEVEL_BIAS_MIN	2
> +#define ABM_LEVEL_BIAS_MAX	3
> +#define ABM_LEVEL_MAX		4
> +
>  #endif
> -- 
> 2.51.2
> 

-- 
Simona Vetter
Software Engineer
http://blog.ffwll.ch
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Jani Nikula 2 months, 3 weeks ago
On Wed, 12 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
> The adaptive backlight modulation property is supported on AMD hardware but
> compositors should be aware of it in standard DRM property documentation.

I think the reason for adding a property in drm core code should be to
share the ABI between drivers.

> Move the helper to create the property and documentation into DRM.
>

Link: to the userspace implementation according to
Documentation/gpu/drm-uapi.rst

> Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
>  drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
>  include/drm/drm_connector.h                 |  8 +++
>  4 files changed, 80 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index f8b35c487b6c..3d840bef77bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1363,67 +1363,9 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>  	{ AMDGPU_FMT_DITHER_ENABLE, "on" },
>  };
>  
> -/**
> - * DOC: property for adaptive backlight modulation
> - *
> - * The 'adaptive backlight modulation' property is used for the compositor to
> - * directly control the adaptive backlight modulation power savings feature
> - * that is part of DCN hardware.
> - *
> - * The property will be attached specifically to eDP panels that support it.
> - *
> - * The property is by default set to 'sysfs' to allow the sysfs file 'panel_power_savings'
> - * to be able to control it.
> - * If set to 'off' the compositor will ensure it stays off.
> - * The other values 'min', 'bias min', 'bias max', and 'max' will control the
> - * intensity of the power savings.
> - *
> - * Modifying this value can have implications on color accuracy, so tread
> - * carefully.
> - */
> -static int amdgpu_display_setup_abm_prop(struct amdgpu_device *adev)
> -{
> -	const struct drm_prop_enum_list props[] = {
> -		{ ABM_SYSFS_CONTROL, "sysfs" },
> -		{ ABM_LEVEL_OFF, "off" },
> -		{ ABM_LEVEL_MIN, "min" },
> -		{ ABM_LEVEL_BIAS_MIN, "bias min" },
> -		{ ABM_LEVEL_BIAS_MAX, "bias max" },
> -		{ ABM_LEVEL_MAX, "max" },
> -	};
> -	struct drm_property *prop;
> -	int i;
> -
> -	if (!adev->dc_enabled)
> -		return 0;
> -
> -	prop = drm_property_create(adev_to_drm(adev), DRM_MODE_PROP_ENUM,
> -				"adaptive backlight modulation",
> -				6);
> -	if (!prop)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < ARRAY_SIZE(props); i++) {
> -		int ret;
> -
> -		ret = drm_property_add_enum(prop, props[i].type,
> -						props[i].name);
> -
> -		if (ret) {
> -			drm_property_destroy(adev_to_drm(adev), prop);
> -
> -			return ret;
> -		}
> -	}
> -
> -	adev->mode_info.abm_level_property = prop;
> -
> -	return 0;
> -}
> -
>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  {
> -	int sz;
> +	int ret, sz;
>  
>  	adev->mode_info.coherent_mode_property =
>  		drm_property_create_range(adev_to_drm(adev), 0, "coherent", 0, 1);
> @@ -1467,7 +1409,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  					 "dither",
>  					 amdgpu_dither_enum_list, sz);
>  
> -	return amdgpu_display_setup_abm_prop(adev);
> +	adev->mode_info.abm_level_property = drm_create_abm_property(adev_to_drm(adev));
> +	if (IS_ERR(adev->mode_info.abm_level_property)) {
> +		ret = PTR_ERR(adev->mode_info.abm_level_property);
> +		adev->mode_info.abm_level_property = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  void amdgpu_display_update_priority(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 2b1536a16752..dfa0d642ac16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -54,11 +54,4 @@ int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>  int amdgpu_display_get_scanout_buffer(struct drm_plane *plane,
>  				      struct drm_scanout_buffer *sb);
>  
> -#define ABM_SYSFS_CONTROL	-1
> -#define ABM_LEVEL_OFF		0
> -#define ABM_LEVEL_MIN		1
> -#define ABM_LEVEL_BIAS_MIN	2
> -#define ABM_LEVEL_BIAS_MAX	3
> -#define ABM_LEVEL_MAX		4
> -
>  #endif
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea47..376169dac247 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2603,6 +2603,69 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +/**
> + * DOC: integrated panel properties
> + *
> + * adaptive backlight modulation:
> + *	Adaptive backlight modulation (ABM) is a power savings feature that
> + *	dynamically adjusts the backlight brightness based on the content
> + *	displayed on the screen. By reducing the backlight brightness for
> + *	darker images and increasing it for brighter images, ABM helps to
> + *	conserve energy and extend battery life on devices with integrated
> + *	displays.  This feature is part of AMD DCN hardware.

So this is basically Content Adaptive Brightness Control, but with the
technology ("backlight" and "modulation") unnecessarily encoded in the
ABI.

You could have the same knob for adjusting CABC implemented in an OLED
panel, controlled via DPCD.

> + *
> + *	sysfs
> + *		The ABM property is exposed to userspace via sysfs interface
> + *		located at 'amdgpu/panel_power_savings' under the DRM device.

Err what? Seriously suggesting that to the common ABI? We shouldn't have
sysfs involved at all, let alone vendor specific sysfs.

> + *	off
> + *		Adaptive backlight modulation is disabled.
> + *	min
> + *		Adaptive backlight modulation is enabled at minimum intensity.
> + *	bias min
> + *		Adaptive backlight modulation is enabled at a more intense
> + *		level than 'min'.
> + *	bias max
> + *		Adaptive backlight modulation is enabled at a more intense
> + *		level than 'bias min'.
> + *	max
> + *		Adaptive backlight modulation is enabled at maximum intensity.

So values 0-4 but with names. I don't know what "bias" means here, and I
probably shouldn't even have to know. It's an implementation detail
leaking to the ABI.

In the past I've encountered CABC with different modes based on the use
case, e.g. "video" or "game", but I don't know how those would map to
the intensities.

I'm concerned the ABI serves AMD hardware, no one else, and everyone
else coming after this is forced to shoehorn their implementation into
this.

> + */
> +struct drm_property *drm_create_abm_property(struct drm_device *dev)
> +{
> +	const struct drm_prop_enum_list props[] = {
> +		{ ABM_SYSFS_CONTROL, "sysfs" },
> +		{ ABM_LEVEL_OFF, "off" },
> +		{ ABM_LEVEL_MIN, "min" },
> +		{ ABM_LEVEL_BIAS_MIN, "bias min" },
> +		{ ABM_LEVEL_BIAS_MAX, "bias max" },
> +		{ ABM_LEVEL_MAX, "max" },
> +	};
> +	struct drm_property *prop;
> +	int i;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> +				"adaptive backlight modulation",
> +				6);

Please use drm_property_create_enum(), see
e.g. drm_connector_attach_broadcast_rgb_property().

> +	if (!prop)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		int ret;
> +
> +		ret = drm_property_add_enum(prop, props[i].type,
> +						props[i].name);
> +
> +		if (ret) {
> +			drm_property_destroy(dev, prop);
> +
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	return prop;
> +}
> +EXPORT_SYMBOL(drm_create_abm_property);
> +
>  /**
>   * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
>   * @connector: connector to create the Colorspace property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..644c0d49500f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2454,6 +2454,7 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
>  bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
>  					     struct drm_connector_state *new_state);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> +struct drm_property *drm_create_abm_property(struct drm_device *dev);
>  int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
>  					     u32 supported_colorspaces);
>  int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
> @@ -2563,4 +2564,11 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
>  	drm_for_each_encoder_mask(encoder, (connector)->dev, \
>  				  (connector)->possible_encoders)
>  
> +#define ABM_SYSFS_CONTROL	-1
> +#define ABM_LEVEL_OFF		0
> +#define ABM_LEVEL_MIN		1
> +#define ABM_LEVEL_BIAS_MIN	2
> +#define ABM_LEVEL_BIAS_MAX	3
> +#define ABM_LEVEL_MAX		4

These need to be an enum, with documentation, name prefixes, the works.

BR,
Jani.

> +
>  #endif

-- 
Jani Nikula, Intel
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Jani Nikula 2 months, 3 weeks ago
On Thu, 13 Nov 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 12 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
>> The adaptive backlight modulation property is supported on AMD hardware but
>> compositors should be aware of it in standard DRM property documentation.
>
> I think the reason for adding a property in drm core code should be to
> share the ABI between drivers.
>
>> Move the helper to create the property and documentation into DRM.
>>
>
> Link: to the userspace implementation according to
> Documentation/gpu/drm-uapi.rst
>
>> Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
>> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
>>  drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
>>  include/drm/drm_connector.h                 |  8 +++
>>  4 files changed, 80 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> index f8b35c487b6c..3d840bef77bf 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> @@ -1363,67 +1363,9 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>>  	{ AMDGPU_FMT_DITHER_ENABLE, "on" },
>>  };
>>  
>> -/**
>> - * DOC: property for adaptive backlight modulation
>> - *
>> - * The 'adaptive backlight modulation' property is used for the compositor to
>> - * directly control the adaptive backlight modulation power savings feature
>> - * that is part of DCN hardware.
>> - *
>> - * The property will be attached specifically to eDP panels that support it.
>> - *
>> - * The property is by default set to 'sysfs' to allow the sysfs file 'panel_power_savings'
>> - * to be able to control it.
>> - * If set to 'off' the compositor will ensure it stays off.
>> - * The other values 'min', 'bias min', 'bias max', and 'max' will control the
>> - * intensity of the power savings.
>> - *
>> - * Modifying this value can have implications on color accuracy, so tread
>> - * carefully.
>> - */
>> -static int amdgpu_display_setup_abm_prop(struct amdgpu_device *adev)
>> -{
>> -	const struct drm_prop_enum_list props[] = {
>> -		{ ABM_SYSFS_CONTROL, "sysfs" },
>> -		{ ABM_LEVEL_OFF, "off" },
>> -		{ ABM_LEVEL_MIN, "min" },
>> -		{ ABM_LEVEL_BIAS_MIN, "bias min" },
>> -		{ ABM_LEVEL_BIAS_MAX, "bias max" },
>> -		{ ABM_LEVEL_MAX, "max" },
>> -	};
>> -	struct drm_property *prop;
>> -	int i;
>> -
>> -	if (!adev->dc_enabled)
>> -		return 0;
>> -
>> -	prop = drm_property_create(adev_to_drm(adev), DRM_MODE_PROP_ENUM,
>> -				"adaptive backlight modulation",
>> -				6);
>> -	if (!prop)
>> -		return -ENOMEM;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(props); i++) {
>> -		int ret;
>> -
>> -		ret = drm_property_add_enum(prop, props[i].type,
>> -						props[i].name);
>> -
>> -		if (ret) {
>> -			drm_property_destroy(adev_to_drm(adev), prop);
>> -
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	adev->mode_info.abm_level_property = prop;
>> -
>> -	return 0;
>> -}
>> -
>>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>>  {
>> -	int sz;
>> +	int ret, sz;
>>  
>>  	adev->mode_info.coherent_mode_property =
>>  		drm_property_create_range(adev_to_drm(adev), 0, "coherent", 0, 1);
>> @@ -1467,7 +1409,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>>  					 "dither",
>>  					 amdgpu_dither_enum_list, sz);
>>  
>> -	return amdgpu_display_setup_abm_prop(adev);
>> +	adev->mode_info.abm_level_property = drm_create_abm_property(adev_to_drm(adev));
>> +	if (IS_ERR(adev->mode_info.abm_level_property)) {
>> +		ret = PTR_ERR(adev->mode_info.abm_level_property);
>> +		adev->mode_info.abm_level_property = NULL;
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  
>>  void amdgpu_display_update_priority(struct amdgpu_device *adev)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>> index 2b1536a16752..dfa0d642ac16 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>> @@ -54,11 +54,4 @@ int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>>  int amdgpu_display_get_scanout_buffer(struct drm_plane *plane,
>>  				      struct drm_scanout_buffer *sb);
>>  
>> -#define ABM_SYSFS_CONTROL	-1
>> -#define ABM_LEVEL_OFF		0
>> -#define ABM_LEVEL_MIN		1
>> -#define ABM_LEVEL_BIAS_MIN	2
>> -#define ABM_LEVEL_BIAS_MAX	3
>> -#define ABM_LEVEL_MAX		4
>> -
>>  #endif
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 272d6254ea47..376169dac247 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -2603,6 +2603,69 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
>>  	return 0;
>>  }
>>  
>> +/**
>> + * DOC: integrated panel properties
>> + *
>> + * adaptive backlight modulation:
>> + *	Adaptive backlight modulation (ABM) is a power savings feature that
>> + *	dynamically adjusts the backlight brightness based on the content
>> + *	displayed on the screen. By reducing the backlight brightness for
>> + *	darker images and increasing it for brighter images, ABM helps to
>> + *	conserve energy and extend battery life on devices with integrated
>> + *	displays.  This feature is part of AMD DCN hardware.
>
> So this is basically Content Adaptive Brightness Control, but with the
> technology ("backlight" and "modulation") unnecessarily encoded in the
> ABI.
>
> You could have the same knob for adjusting CABC implemented in an OLED
> panel, controlled via DPCD.
>
>> + *
>> + *	sysfs
>> + *		The ABM property is exposed to userspace via sysfs interface
>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
>
> Err what? Seriously suggesting that to the common ABI? We shouldn't have
> sysfs involved at all, let alone vendor specific sysfs.
>
>> + *	off
>> + *		Adaptive backlight modulation is disabled.
>> + *	min
>> + *		Adaptive backlight modulation is enabled at minimum intensity.
>> + *	bias min
>> + *		Adaptive backlight modulation is enabled at a more intense
>> + *		level than 'min'.
>> + *	bias max
>> + *		Adaptive backlight modulation is enabled at a more intense
>> + *		level than 'bias min'.
>> + *	max
>> + *		Adaptive backlight modulation is enabled at maximum intensity.
>
> So values 0-4 but with names. I don't know what "bias" means here, and I
> probably shouldn't even have to know. It's an implementation detail
> leaking to the ABI.
>
> In the past I've encountered CABC with different modes based on the use
> case, e.g. "video" or "game", but I don't know how those would map to
> the intensities.
>
> I'm concerned the ABI serves AMD hardware, no one else, and everyone
> else coming after this is forced to shoehorn their implementation into
> this.

Apparently Harry had the same concerns [1].

BR,
Jani.


[1] https://lore.kernel.org/r/2d4cbf1e-f8f7-4f6e-9e7e-15fb05234248@amd.com


>
>> + */
>> +struct drm_property *drm_create_abm_property(struct drm_device *dev)
>> +{
>> +	const struct drm_prop_enum_list props[] = {
>> +		{ ABM_SYSFS_CONTROL, "sysfs" },
>> +		{ ABM_LEVEL_OFF, "off" },
>> +		{ ABM_LEVEL_MIN, "min" },
>> +		{ ABM_LEVEL_BIAS_MIN, "bias min" },
>> +		{ ABM_LEVEL_BIAS_MAX, "bias max" },
>> +		{ ABM_LEVEL_MAX, "max" },
>> +	};
>> +	struct drm_property *prop;
>> +	int i;
>> +
>> +	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
>> +				"adaptive backlight modulation",
>> +				6);
>
> Please use drm_property_create_enum(), see
> e.g. drm_connector_attach_broadcast_rgb_property().
>
>> +	if (!prop)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
>> +		int ret;
>> +
>> +		ret = drm_property_add_enum(prop, props[i].type,
>> +						props[i].name);
>> +
>> +		if (ret) {
>> +			drm_property_destroy(dev, prop);
>> +
>> +			return ERR_PTR(ret);
>> +		}
>> +	}
>> +
>> +	return prop;
>> +}
>> +EXPORT_SYMBOL(drm_create_abm_property);
>> +
>>  /**
>>   * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
>>   * @connector: connector to create the Colorspace property on.
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 8f34f4b8183d..644c0d49500f 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -2454,6 +2454,7 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
>>  bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
>>  					     struct drm_connector_state *new_state);
>>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>> +struct drm_property *drm_create_abm_property(struct drm_device *dev);
>>  int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
>>  					     u32 supported_colorspaces);
>>  int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
>> @@ -2563,4 +2564,11 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
>>  	drm_for_each_encoder_mask(encoder, (connector)->dev, \
>>  				  (connector)->possible_encoders)
>>  
>> +#define ABM_SYSFS_CONTROL	-1
>> +#define ABM_LEVEL_OFF		0
>> +#define ABM_LEVEL_MIN		1
>> +#define ABM_LEVEL_BIAS_MIN	2
>> +#define ABM_LEVEL_BIAS_MAX	3
>> +#define ABM_LEVEL_MAX		4
>
> These need to be an enum, with documentation, name prefixes, the works.
>
> BR,
> Jani.
>
>> +
>>  #endif

-- 
Jani Nikula, Intel
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Mario Limonciello 2 months, 3 weeks ago
+Xaver

On 11/14/2025 2:39 AM, Jani Nikula wrote:
<snip>

>>
>> So this is basically Content Adaptive Brightness Control, but with the
>> technology ("backlight" and "modulation") unnecessarily encoded in the
>> ABI.
>>
>> You could have the same knob for adjusting CABC implemented in an OLED
>> panel, controlled via DPCD.
>>
>>> + *
>>> + *	sysfs
>>> + *		The ABM property is exposed to userspace via sysfs interface
>>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
>>
>> Err what? Seriously suggesting that to the common ABI? We shouldn't have
>> sysfs involved at all, let alone vendor specific sysfs.
>>
>>> + *	off
>>> + *		Adaptive backlight modulation is disabled.
>>> + *	min
>>> + *		Adaptive backlight modulation is enabled at minimum intensity.
>>> + *	bias min
>>> + *		Adaptive backlight modulation is enabled at a more intense
>>> + *		level than 'min'.
>>> + *	bias max
>>> + *		Adaptive backlight modulation is enabled at a more intense
>>> + *		level than 'bias min'.
>>> + *	max
>>> + *		Adaptive backlight modulation is enabled at maximum intensity.
>>
>> So values 0-4 but with names. I don't know what "bias" means here, and I
>> probably shouldn't even have to know. It's an implementation detail
>> leaking to the ABI.
>>
>> In the past I've encountered CABC with different modes based on the use
>> case, e.g. "video" or "game", but I don't know how those would map to
>> the intensities.
>>
>> I'm concerned the ABI serves AMD hardware, no one else, and everyone
>> else coming after this is forced to shoehorn their implementation into
>> this.
> 
> Apparently Harry had the same concerns [1].
> 
So let me explain how we got here.  At the display next hackfest last 
year (2024) we talked about how to get compositors to indicate they want 
technologies like this to get out the way.  A patch series was made that 
would allow compositor to say "Require color accuracy" or "Require low 
latency" are required.

https://lore.kernel.org/amd-gfx/20240703051722.328-1-mario.limonciello@amd.com/

This got reverted because userspace didn't have an implementation ready 
to go at the time.  One was created and so I rebased/resent the series 
earlier this year.

https://lore.kernel.org/amd-gfx/20250621152657.1048807-1-superm1@kernel.org/

Xaver had some change of heart and wanted to talk about it at the next 
hackfest.

https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@mail.gmail.com/

So we talked about it again at the hackfest this year and the decision 
was not everyone can an octagon into a peg hole, so we're better off 
re-introducing vendor properties for this.  So series was respun per 
that discussion.

https://lore.kernel.org/amd-gfx/20250718192045.2091650-1-superm1@kernel.org/

Userspace implementation was done and so we merged this for 6.19.

https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@mail.gmail.com/

Then Simona suggested we need to make some changes where the propertye 
should be in generic documentation etc:

https://lore.kernel.org/amd-gfx/aQUz-mbM_WlXn_uZ@phenom.ffwll.local/

So that's where we are now with this patch.  I can clean it up per the 
feedback so far - but I think we need to be in agreement that this 
property is actually the way forward or we should revert the property in 
amdgpu instead of this moving approach and keep discussing.
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Jani Nikula 2 months, 3 weeks ago
On Fri, 14 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
> +Xaver
>
> On 11/14/2025 2:39 AM, Jani Nikula wrote:
> <snip>
>
>>>
>>> So this is basically Content Adaptive Brightness Control, but with the
>>> technology ("backlight" and "modulation") unnecessarily encoded in the
>>> ABI.
>>>
>>> You could have the same knob for adjusting CABC implemented in an OLED
>>> panel, controlled via DPCD.
>>>
>>>> + *
>>>> + *	sysfs
>>>> + *		The ABM property is exposed to userspace via sysfs interface
>>>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
>>>
>>> Err what? Seriously suggesting that to the common ABI? We shouldn't have
>>> sysfs involved at all, let alone vendor specific sysfs.
>>>
>>>> + *	off
>>>> + *		Adaptive backlight modulation is disabled.
>>>> + *	min
>>>> + *		Adaptive backlight modulation is enabled at minimum intensity.
>>>> + *	bias min
>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>> + *		level than 'min'.
>>>> + *	bias max
>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>> + *		level than 'bias min'.
>>>> + *	max
>>>> + *		Adaptive backlight modulation is enabled at maximum intensity.
>>>
>>> So values 0-4 but with names. I don't know what "bias" means here, and I
>>> probably shouldn't even have to know. It's an implementation detail
>>> leaking to the ABI.
>>>
>>> In the past I've encountered CABC with different modes based on the use
>>> case, e.g. "video" or "game", but I don't know how those would map to
>>> the intensities.
>>>
>>> I'm concerned the ABI serves AMD hardware, no one else, and everyone
>>> else coming after this is forced to shoehorn their implementation into
>>> this.
>> 
>> Apparently Harry had the same concerns [1].
>> 
> So let me explain how we got here.  At the display next hackfest last 
> year (2024) we talked about how to get compositors to indicate they want 
> technologies like this to get out the way.  A patch series was made that 
> would allow compositor to say "Require color accuracy" or "Require low 
> latency" are required.
>
> https://lore.kernel.org/amd-gfx/20240703051722.328-1-mario.limonciello@amd.com/
>
> This got reverted because userspace didn't have an implementation ready 
> to go at the time.  One was created and so I rebased/resent the series 
> earlier this year.
>
> https://lore.kernel.org/amd-gfx/20250621152657.1048807-1-superm1@kernel.org/
>
> Xaver had some change of heart and wanted to talk about it at the next 
> hackfest.
>
> https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@mail.gmail.com/
>
> So we talked about it again at the hackfest this year and the decision 
> was not everyone can an octagon into a peg hole, so we're better off 
> re-introducing vendor properties for this.  So series was respun per 
> that discussion.
>
> https://lore.kernel.org/amd-gfx/20250718192045.2091650-1-superm1@kernel.org/
>
> Userspace implementation was done and so we merged this for 6.19.
>
> https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@mail.gmail.com/
>
> Then Simona suggested we need to make some changes where the propertye 
> should be in generic documentation etc:
>
> https://lore.kernel.org/amd-gfx/aQUz-mbM_WlXn_uZ@phenom.ffwll.local/
>
> So that's where we are now with this patch.  I can clean it up per the 
> feedback so far - but I think we need to be in agreement that this 
> property is actually the way forward or we should revert the property in 
> amdgpu instead of this moving approach and keep discussing.

IMO we should either

- admit we can't do a generic property for this *and* keep the vendor
  specific property details hidden in drivers, or

- figure out a generic property and add that in drm core

But I'm pretty much against adding an AMD vendor specific property in
drm core.


BR,
Jani.


-- 
Jani Nikula, Intel
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Maxime Ripard 2 months, 3 weeks ago
On Mon, Nov 17, 2025 at 11:05:10AM +0200, Jani Nikula wrote:
> On Fri, 14 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
> > +Xaver
> >
> > On 11/14/2025 2:39 AM, Jani Nikula wrote:
> > <snip>
> >
> >>>
> >>> So this is basically Content Adaptive Brightness Control, but with the
> >>> technology ("backlight" and "modulation") unnecessarily encoded in the
> >>> ABI.
> >>>
> >>> You could have the same knob for adjusting CABC implemented in an OLED
> >>> panel, controlled via DPCD.
> >>>
> >>>> + *
> >>>> + *	sysfs
> >>>> + *		The ABM property is exposed to userspace via sysfs interface
> >>>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
> >>>
> >>> Err what? Seriously suggesting that to the common ABI? We shouldn't have
> >>> sysfs involved at all, let alone vendor specific sysfs.
> >>>
> >>>> + *	off
> >>>> + *		Adaptive backlight modulation is disabled.
> >>>> + *	min
> >>>> + *		Adaptive backlight modulation is enabled at minimum intensity.
> >>>> + *	bias min
> >>>> + *		Adaptive backlight modulation is enabled at a more intense
> >>>> + *		level than 'min'.
> >>>> + *	bias max
> >>>> + *		Adaptive backlight modulation is enabled at a more intense
> >>>> + *		level than 'bias min'.
> >>>> + *	max
> >>>> + *		Adaptive backlight modulation is enabled at maximum intensity.
> >>>
> >>> So values 0-4 but with names. I don't know what "bias" means here, and I
> >>> probably shouldn't even have to know. It's an implementation detail
> >>> leaking to the ABI.
> >>>
> >>> In the past I've encountered CABC with different modes based on the use
> >>> case, e.g. "video" or "game", but I don't know how those would map to
> >>> the intensities.
> >>>
> >>> I'm concerned the ABI serves AMD hardware, no one else, and everyone
> >>> else coming after this is forced to shoehorn their implementation into
> >>> this.
> >> 
> >> Apparently Harry had the same concerns [1].
> >> 
> > So let me explain how we got here.  At the display next hackfest last 
> > year (2024) we talked about how to get compositors to indicate they want 
> > technologies like this to get out the way.  A patch series was made that 
> > would allow compositor to say "Require color accuracy" or "Require low 
> > latency" are required.
> >
> > https://lore.kernel.org/amd-gfx/20240703051722.328-1-mario.limonciello@amd.com/
> >
> > This got reverted because userspace didn't have an implementation ready 
> > to go at the time.  One was created and so I rebased/resent the series 
> > earlier this year.
> >
> > https://lore.kernel.org/amd-gfx/20250621152657.1048807-1-superm1@kernel.org/
> >
> > Xaver had some change of heart and wanted to talk about it at the next 
> > hackfest.
> >
> > https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@mail.gmail.com/
> >
> > So we talked about it again at the hackfest this year and the decision 
> > was not everyone can an octagon into a peg hole, so we're better off 
> > re-introducing vendor properties for this.  So series was respun per 
> > that discussion.
> >
> > https://lore.kernel.org/amd-gfx/20250718192045.2091650-1-superm1@kernel.org/
> >
> > Userspace implementation was done and so we merged this for 6.19.
> >
> > https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@mail.gmail.com/
> >
> > Then Simona suggested we need to make some changes where the propertye 
> > should be in generic documentation etc:
> >
> > https://lore.kernel.org/amd-gfx/aQUz-mbM_WlXn_uZ@phenom.ffwll.local/
> >
> > So that's where we are now with this patch.  I can clean it up per the 
> > feedback so far - but I think we need to be in agreement that this 
> > property is actually the way forward or we should revert the property in 
> > amdgpu instead of this moving approach and keep discussing.
> 
> IMO we should either
> 
> - admit we can't do a generic property for this *and* keep the vendor
>   specific property details hidden in drivers, or
> 
> - figure out a generic property and add that in drm core
> 
> But I'm pretty much against adding an AMD vendor specific property in
> drm core.

Agreed

Maxime
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Mario Limonciello 2 months, 3 weeks ago

On 11/18/2025 2:47 AM, Maxime Ripard wrote:
> On Mon, Nov 17, 2025 at 11:05:10AM +0200, Jani Nikula wrote:
>> On Fri, 14 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
>>> +Xaver
>>>
>>> On 11/14/2025 2:39 AM, Jani Nikula wrote:
>>> <snip>
>>>
>>>>>
>>>>> So this is basically Content Adaptive Brightness Control, but with the
>>>>> technology ("backlight" and "modulation") unnecessarily encoded in the
>>>>> ABI.
>>>>>
>>>>> You could have the same knob for adjusting CABC implemented in an OLED
>>>>> panel, controlled via DPCD.
>>>>>
>>>>>> + *
>>>>>> + *	sysfs
>>>>>> + *		The ABM property is exposed to userspace via sysfs interface
>>>>>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
>>>>>
>>>>> Err what? Seriously suggesting that to the common ABI? We shouldn't have
>>>>> sysfs involved at all, let alone vendor specific sysfs.
>>>>>
>>>>>> + *	off
>>>>>> + *		Adaptive backlight modulation is disabled.
>>>>>> + *	min
>>>>>> + *		Adaptive backlight modulation is enabled at minimum intensity.
>>>>>> + *	bias min
>>>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>>>> + *		level than 'min'.
>>>>>> + *	bias max
>>>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>>>> + *		level than 'bias min'.
>>>>>> + *	max
>>>>>> + *		Adaptive backlight modulation is enabled at maximum intensity.
>>>>>
>>>>> So values 0-4 but with names. I don't know what "bias" means here, and I
>>>>> probably shouldn't even have to know. It's an implementation detail
>>>>> leaking to the ABI.
>>>>>
>>>>> In the past I've encountered CABC with different modes based on the use
>>>>> case, e.g. "video" or "game", but I don't know how those would map to
>>>>> the intensities.
>>>>>
>>>>> I'm concerned the ABI serves AMD hardware, no one else, and everyone
>>>>> else coming after this is forced to shoehorn their implementation into
>>>>> this.
>>>>
>>>> Apparently Harry had the same concerns [1].
>>>>
>>> So let me explain how we got here.  At the display next hackfest last
>>> year (2024) we talked about how to get compositors to indicate they want
>>> technologies like this to get out the way.  A patch series was made that
>>> would allow compositor to say "Require color accuracy" or "Require low
>>> latency" are required.
>>>
>>> https://lore.kernel.org/amd-gfx/20240703051722.328-1-mario.limonciello@amd.com/
>>>
>>> This got reverted because userspace didn't have an implementation ready
>>> to go at the time.  One was created and so I rebased/resent the series
>>> earlier this year.
>>>
>>> https://lore.kernel.org/amd-gfx/20250621152657.1048807-1-superm1@kernel.org/
>>>
>>> Xaver had some change of heart and wanted to talk about it at the next
>>> hackfest.
>>>
>>> https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@mail.gmail.com/
>>>
>>> So we talked about it again at the hackfest this year and the decision
>>> was not everyone can an octagon into a peg hole, so we're better off
>>> re-introducing vendor properties for this.  So series was respun per
>>> that discussion.
>>>
>>> https://lore.kernel.org/amd-gfx/20250718192045.2091650-1-superm1@kernel.org/
>>>
>>> Userspace implementation was done and so we merged this for 6.19.
>>>
>>> https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@mail.gmail.com/
>>>
>>> Then Simona suggested we need to make some changes where the propertye
>>> should be in generic documentation etc:
>>>
>>> https://lore.kernel.org/amd-gfx/aQUz-mbM_WlXn_uZ@phenom.ffwll.local/
>>>
>>> So that's where we are now with this patch.  I can clean it up per the
>>> feedback so far - but I think we need to be in agreement that this
>>> property is actually the way forward or we should revert the property in
>>> amdgpu instead of this moving approach and keep discussing.
>>
>> IMO we should either
>>
>> - admit we can't do a generic property for this *and* keep the vendor
>>    specific property details hidden in drivers, or
>>
>> - figure out a generic property and add that in drm core
>>
>> But I'm pretty much against adding an AMD vendor specific property in
>> drm core.
> 
> Agreed
> 
> Maxime

OK - I will discard this patch.

Simona, please any comments if you still want to see anything change 
from the vendor property.
Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core
Posted by Alex Deucher 2 months, 3 weeks ago
On Wed, Nov 12, 2025 at 5:27 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> The adaptive backlight modulation property is supported on AMD hardware but
> compositors should be aware of it in standard DRM property documentation.
>
> Move the helper to create the property and documentation into DRM.
>
> Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 69 +++------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |  7 ---
>  drivers/gpu/drm/drm_connector.c             | 63 +++++++++++++++++++
>  include/drm/drm_connector.h                 |  8 +++
>  4 files changed, 80 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index f8b35c487b6c..3d840bef77bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1363,67 +1363,9 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>         { AMDGPU_FMT_DITHER_ENABLE, "on" },
>  };
>
> -/**
> - * DOC: property for adaptive backlight modulation
> - *
> - * The 'adaptive backlight modulation' property is used for the compositor to
> - * directly control the adaptive backlight modulation power savings feature
> - * that is part of DCN hardware.
> - *
> - * The property will be attached specifically to eDP panels that support it.
> - *
> - * The property is by default set to 'sysfs' to allow the sysfs file 'panel_power_savings'
> - * to be able to control it.
> - * If set to 'off' the compositor will ensure it stays off.
> - * The other values 'min', 'bias min', 'bias max', and 'max' will control the
> - * intensity of the power savings.
> - *
> - * Modifying this value can have implications on color accuracy, so tread
> - * carefully.
> - */
> -static int amdgpu_display_setup_abm_prop(struct amdgpu_device *adev)
> -{
> -       const struct drm_prop_enum_list props[] = {
> -               { ABM_SYSFS_CONTROL, "sysfs" },
> -               { ABM_LEVEL_OFF, "off" },
> -               { ABM_LEVEL_MIN, "min" },
> -               { ABM_LEVEL_BIAS_MIN, "bias min" },
> -               { ABM_LEVEL_BIAS_MAX, "bias max" },
> -               { ABM_LEVEL_MAX, "max" },
> -       };
> -       struct drm_property *prop;
> -       int i;
> -
> -       if (!adev->dc_enabled)
> -               return 0;
> -
> -       prop = drm_property_create(adev_to_drm(adev), DRM_MODE_PROP_ENUM,
> -                               "adaptive backlight modulation",
> -                               6);
> -       if (!prop)
> -               return -ENOMEM;
> -
> -       for (i = 0; i < ARRAY_SIZE(props); i++) {
> -               int ret;
> -
> -               ret = drm_property_add_enum(prop, props[i].type,
> -                                               props[i].name);
> -
> -               if (ret) {
> -                       drm_property_destroy(adev_to_drm(adev), prop);
> -
> -                       return ret;
> -               }
> -       }
> -
> -       adev->mode_info.abm_level_property = prop;
> -
> -       return 0;
> -}
> -
>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  {
> -       int sz;
> +       int ret, sz;
>
>         adev->mode_info.coherent_mode_property =
>                 drm_property_create_range(adev_to_drm(adev), 0, "coherent", 0, 1);
> @@ -1467,7 +1409,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>                                          "dither",
>                                          amdgpu_dither_enum_list, sz);
>
> -       return amdgpu_display_setup_abm_prop(adev);
> +       adev->mode_info.abm_level_property = drm_create_abm_property(adev_to_drm(adev));
> +       if (IS_ERR(adev->mode_info.abm_level_property)) {
> +               ret = PTR_ERR(adev->mode_info.abm_level_property);
> +               adev->mode_info.abm_level_property = NULL;
> +               return ret;
> +       }
> +
> +       return 0;
>  }
>
>  void amdgpu_display_update_priority(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 2b1536a16752..dfa0d642ac16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -54,11 +54,4 @@ int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>  int amdgpu_display_get_scanout_buffer(struct drm_plane *plane,
>                                       struct drm_scanout_buffer *sb);
>
> -#define ABM_SYSFS_CONTROL      -1
> -#define ABM_LEVEL_OFF          0
> -#define ABM_LEVEL_MIN          1
> -#define ABM_LEVEL_BIAS_MIN     2
> -#define ABM_LEVEL_BIAS_MAX     3
> -#define ABM_LEVEL_MAX          4
> -
>  #endif
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea47..376169dac247 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2603,6 +2603,69 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
>         return 0;
>  }
>
> +/**
> + * DOC: integrated panel properties
> + *
> + * adaptive backlight modulation:
> + *     Adaptive backlight modulation (ABM) is a power savings feature that
> + *     dynamically adjusts the backlight brightness based on the content
> + *     displayed on the screen. By reducing the backlight brightness for
> + *     darker images and increasing it for brighter images, ABM helps to
> + *     conserve energy and extend battery life on devices with integrated
> + *     displays.  This feature is part of AMD DCN hardware.
> + *
> + *     sysfs
> + *             The ABM property is exposed to userspace via sysfs interface
> + *             located at 'amdgpu/panel_power_savings' under the DRM device.
> + *     off
> + *             Adaptive backlight modulation is disabled.
> + *     min
> + *             Adaptive backlight modulation is enabled at minimum intensity.
> + *     bias min
> + *             Adaptive backlight modulation is enabled at a more intense
> + *             level than 'min'.
> + *     bias max
> + *             Adaptive backlight modulation is enabled at a more intense
> + *             level than 'bias min'.
> + *     max
> + *             Adaptive backlight modulation is enabled at maximum intensity.
> + */
> +struct drm_property *drm_create_abm_property(struct drm_device *dev)
> +{
> +       const struct drm_prop_enum_list props[] = {
> +               { ABM_SYSFS_CONTROL, "sysfs" },
> +               { ABM_LEVEL_OFF, "off" },
> +               { ABM_LEVEL_MIN, "min" },
> +               { ABM_LEVEL_BIAS_MIN, "bias min" },
> +               { ABM_LEVEL_BIAS_MAX, "bias max" },
> +               { ABM_LEVEL_MAX, "max" },
> +       };
> +       struct drm_property *prop;
> +       int i;
> +
> +       prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> +                               "adaptive backlight modulation",
> +                               6);
> +       if (!prop)
> +               return ERR_PTR(-ENOMEM);
> +
> +       for (i = 0; i < ARRAY_SIZE(props); i++) {
> +               int ret;
> +
> +               ret = drm_property_add_enum(prop, props[i].type,
> +                                               props[i].name);
> +
> +               if (ret) {
> +                       drm_property_destroy(dev, prop);
> +
> +                       return ERR_PTR(ret);
> +               }
> +       }
> +
> +       return prop;
> +}
> +EXPORT_SYMBOL(drm_create_abm_property);
> +
>  /**
>   * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
>   * @connector: connector to create the Colorspace property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..644c0d49500f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2454,6 +2454,7 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
>  bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
>                                              struct drm_connector_state *new_state);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> +struct drm_property *drm_create_abm_property(struct drm_device *dev);
>  int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
>                                              u32 supported_colorspaces);
>  int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
> @@ -2563,4 +2564,11 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
>         drm_for_each_encoder_mask(encoder, (connector)->dev, \
>                                   (connector)->possible_encoders)
>
> +#define ABM_SYSFS_CONTROL      -1
> +#define ABM_LEVEL_OFF          0
> +#define ABM_LEVEL_MIN          1
> +#define ABM_LEVEL_BIAS_MIN     2
> +#define ABM_LEVEL_BIAS_MAX     3
> +#define ABM_LEVEL_MAX          4
> +
>  #endif
> --
> 2.51.2
>