| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <449ee5ba065e1ceee8f7a04038442cff24772df9@intel.com>
Date: Fri, 14 Nov 2025 10:39:32 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Mario Limonciello <mario.limonciello@....com>, Simona Vetter
<simona@...ll.ch>
Cc: Alex Deucher <alexander.deucher@....com>, Christian König
<christian.koenig@....com>, David Airlie <airlied@...il.com>, "open
list:RADEON and AMDGPU DRM DRIVERS" <amd-gfx@...ts.freedesktop.org>, "open
list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>, open list
<linux-kernel@...r.kernel.org>, Mario Limonciello
<mario.limonciello@....com>, Simona Vetter <simona.vetter@...ll.ch>, Harry
Wentland <Harry.Wentland@....com>, Dmitry Baryshkov
<dmitry.baryshkov@....qualcomm.com>, Thomas Zimmermann
<tzimmermann@...e.de>, Maxime Ripard <mripard@...nel.org>
Subject: Re: [PATCH] drm/amd: Move adaptive backlight modulation property to
drm core
On Thu, 13 Nov 2025, Jani Nikula <jani.nikula@...ux.intel.com> wrote:
> On Wed, 12 Nov 2025, Mario Limonciello <mario.limonciello@....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@...ll.ch>
>> Reviewed-by: Harry Wentland <Harry.Wentland@....com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....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
Powered by blists - more mailing lists