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

[Jani Nikula <jani.nikula@linux.intel.com>]

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