On 10/30/26 15:58, Shivam Kalra via B4 Relay wrote:
[Some people who received this message don't often get email from
[email protected]. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]
From: Shivam Kalra <[email protected]>
panel_power_savings_store() mutates connector->state directly and sends a
hotplug event. This bypasses atomic commit handling, so the new ABM level
is only applied when userspace happens to perform another modeset, and it
races with concurrent commits.
Build an atomic state, set the level on the connector state and commit it.
If the connector is not attached to a CRTC, keep the level in the current
state so the next modeset picks it up, as before.
A previous attempt to avoid the modeset by calling into DMUB directly was
reverted because it broke the abm_gradual IGT test (8d20a066fa9b,
95aaa207e9ef). This version instead uses the normal atomic stream update
path.
Expose the atomic state update helper for KUnit and extend the tests to
cover the atomic update and the inactive sysfs path.
Fixes: 63d0b87213a0 ("drm/amd/display: add panel_power_savings sysfs entry to eDP
connectors")
Signed-off-by: Shivam Kalra <[email protected]>
---
Note: This patch works standalone (applying ABM changes via a modeset), and
pairs cleanly with David Weber's unmerged series [1] ("drm/amd/display: Skip
connector updates without a stream" and "drm/amd/display: Avoid modeset for
ABM changes") to allow non-modeset ABM updates via the stream update path.
Hi Shivam,
Thanks for the patch. Moving this to an atomic commit is the right
direction, but as it stands every write still ends in a full modeset.
In amdgpu_dm_atomic_check(), any abm_level difference between the old
and new connector state sets new_crtc_state->connectors_changed = true.
That makes drm_atomic_crtc_needs_modeset() true, and commit_tail skips
the abm_changed / stream_update.abm_level fast path. Users would see a
blank/flicker on the eDP panel on every power-profile change.
Could you try keeping drm_atomic_get_crtc_state() and setting
connectors_changed only for scaling? ABM would then go through the
existing stream update with no modeset, like David's [PATCH 2/2]
drm/amd/display: Avoid modeset for ABM changes -
https://lore.kernel.org/amd-gfx/[email protected]/
Please run IGT amd_abm, including abm_gradual, before v2.
Also see inline comments.
[1]
https://lore.kernel.org/amd-gfx/[email protected]/
Testing:
- KUnit: the amdgpu_dm_backlight suite passes on amd-staging-drm-next
(75/75 tests passed), covering the new atomic update and inactive sysfs
tests.
- Hardware: tested on a 7.1.13-based kernel backport alongside [1]. Before
the patch, the store called drm_kms_helper_hotplug_event(), produced two
HOTPLUG=1 uevents (triggering 36 GetConnector probes / 20.8 ms stall in
Xorg), and the backlight PWM sweep stayed flat at 6998 for levels 0..4.
With the patch, ftrace shows panel_power_savings_store() ->
drm_atomic_commit() -> amdgpu_dm_atomic_commit_tail() ->
dc_update_planes_and_stream() -> dmub_abm_set_level_ex(), with no hotplug
event or connector reprobe. The PWM sweep tracks the requested level
(11130, 8304, 5782, 3815 and 2782 target for levels 0..4; 75% target
reduction at level 4, 73.8% measured).
---
.../amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 85 ++++++++++++++++---
.../amd/display/amdgpu_dm/amdgpu_dm_backlight.h | 3 +
.../amdgpu_dm/tests/amdgpu_dm_backlight_test.c | 95 +++++++++++++++++++++-
3 files changed, 172 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index 27546febf90d..f8d797324319 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -712,6 +712,31 @@ ssize_t panel_power_savings_show(struct device *device,
}
EXPORT_IF_KUNIT(panel_power_savings_show);
+static int
+panel_power_savings_set_level(struct dm_connector_state *dm_conn_state, long
val)
+{
+ if (dm_conn_state->abm_sysfs_forbidden)
+ return -EBUSY;
+
+ dm_conn_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE;
+
+ return 0;
+}
+
+STATIC_IFN_KUNIT int
+panel_power_savings_update_state(struct drm_atomic_commit *state,
+ struct drm_connector *connector, long val)
+{
+ struct drm_connector_state *conn_state;
+
+ conn_state = drm_atomic_get_connector_state(state, connector);
+ if (IS_ERR(conn_state))
+ return PTR_ERR(conn_state);
+
+ return panel_power_savings_set_level(to_dm_connector_state(conn_state),
val);
+}
+EXPORT_IF_KUNIT(panel_power_savings_update_state);
+
STATIC_IFN_KUNIT
ssize_t panel_power_savings_store(struct device *device,
struct device_attribute *attr,
@@ -719,6 +744,9 @@ ssize_t panel_power_savings_store(struct device *device,
{
struct drm_connector *connector = dev_get_drvdata(device);
struct drm_device *dev = connector->dev;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_connector_state *conn_state;
+ struct drm_atomic_commit *state;
long val;
int ret;
@@ -730,20 +758,57 @@ ssize_t panel_power_savings_store(struct device *device,
if (val < 0 || val > 4)
return -EINVAL;
- drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
- if (to_dm_connector_state(connector->state)->abm_sysfs_forbidden)
- ret = -EBUSY;
- else
- to_dm_connector_state(connector->state)->abm_level = val ?:
- ABM_LEVEL_IMMEDIATE_DISABLE;
- drm_modeset_unlock(&dev->mode_config.connection_mutex);
+ drm_modeset_acquire_init(&ctx, 0);
Please use drm_modeset_acquire_init(&ctx,
DRM_MODESET_ACQUIRE_INTERRUPTIBLE). With 0, the sysfs write can't be
interrupted while it waits for the modeset locks.
+ state = drm_atomic_commit_alloc(dev);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out_ctx;
+ }
+ state->acquire_ctx = &ctx;
+retry:
+ ret = panel_power_savings_update_state(state, connector, val);
nitpick - panel_power_savings_update_state is used once; it can be
folded here.
+ if (ret == -EDEADLK)
+ goto backoff;
if (ret)
- return ret;
+ goto out_state;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
drm_atomic_get_new_connector_state() only reads back that same slot as
in panel_power_savings_update_state, so the below check is likely not needed
+ if (!conn_state) {
+ ret = -EINVAL;
+ goto out_state;
+ }
+
+ /*
+ * Without a CRTC there is no stream to update and nothing to commit.
+ * Keep the level in the current state so the next modeset picks it
+ * up, matching the behavior of the sysfs interface.
+ */
+ if (!conn_state->crtc) {
+ to_dm_connector_state(connector->state)->abm_level =
+ to_dm_connector_state(conn_state)->abm_level;
+ goto out_state;
+ }
+
+ ret = drm_atomic_commit(state);
+ if (ret == -EDEADLK)
+ goto backoff;
+
+out_state:
+ drm_atomic_commit_put(state);
+out_ctx:
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+
+ return ret ? ret : count;
- drm_kms_helper_hotplug_event(dev);
+backoff:
+ drm_atomic_commit_clear(state);
+ ret = drm_modeset_backoff(&ctx);
+ if (!ret)
+ goto retry;
- return count;
+ goto out_state;
}
EXPORT_IF_KUNIT(panel_power_savings_store);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
index 9ad888d3ae7b..d92ec0789f15 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
@@ -34,6 +34,7 @@ struct device;
struct device_attribute;
struct drm_connector;
struct attribute_group;
+struct drm_atomic_commit;
#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
#define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
@@ -65,6 +66,8 @@ ssize_t panel_power_savings_show(struct device *device,
ssize_t panel_power_savings_store(struct device *device,
struct device_attribute *attr,
const char *buf, size_t count);
+int panel_power_savings_update_state(struct drm_atomic_commit *state,
+ struct drm_connector *connector, long val);
int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
unsigned int *min, unsigned int *max);
void convert_custom_brightness(const struct amdgpu_dm_backlight_caps *caps,
diff --git
a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
index c6d15fb8ad98..a482261e85d2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
@@ -514,6 +514,37 @@ static void
dm_test_register_backlight_device_success(struct kunit *test)
adev->dm.backlight_dev[0]), 0);
}
+static struct drm_connector_state *
+dm_test_panel_power_savings_duplicate_state(struct drm_connector *connector)
+{
+ struct dm_connector_state *state;
+
+ state = kmemdup(to_dm_connector_state(connector->state),
+ sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ __drm_atomic_helper_connector_duplicate_state(connector, &state->base);
+
+ return &state->base;
+}
+
+static void
+dm_test_panel_power_savings_destroy_state(struct drm_connector *connector,
+ struct drm_connector_state *state)
+{
+ __drm_atomic_helper_connector_destroy_state(state);
+ kfree(to_dm_connector_state(state));
+}
+
+static const struct drm_connector_funcs
dm_test_panel_power_savings_connector_funcs = {
+ .atomic_duplicate_state = dm_test_panel_power_savings_duplicate_state,
+ .atomic_destroy_state = dm_test_panel_power_savings_destroy_state,
+};
+
+static const struct drm_mode_config_funcs
dm_test_panel_power_savings_mode_funcs = {
+};
+
static struct drm_connector *setup_panel_power_savings_connector(struct kunit
*test,
struct device
**device_out,
struct
dm_connector_state **state_out)
@@ -527,6 +558,7 @@ static struct drm_connector
*setup_panel_power_savings_connector(struct kunit *t
adev = dm_kunit_alloc_adev(test);
ret = drmm_mode_config_init(&adev->ddev);
KUNIT_ASSERT_EQ(test, ret, 0);
+ adev->ddev.mode_config.funcs = &dm_test_panel_power_savings_mode_funcs;
connector = kunit_kzalloc(test, sizeof(*connector), GFP_KERNEL);
device = kunit_kzalloc(test, sizeof(*device), GFP_KERNEL);
@@ -536,7 +568,11 @@ static struct drm_connector
*setup_panel_power_savings_connector(struct kunit *t
KUNIT_ASSERT_NOT_NULL(test, state);
connector->dev = &adev->ddev;
+ connector->funcs = &dm_test_panel_power_savings_connector_funcs;
+ connector->index = 0;
+ kref_init(&connector->base.refcount);
connector->state = &state->base;
+ state->base.connector = connector;
dev_set_drvdata(device, connector);
*device_out = device;
*state_out = state;
@@ -614,10 +650,40 @@ static void
dm_test_panel_power_savings_store_sets_disable(struct kunit *test)
setup_panel_power_savings_connector(test, &device, &state);
KUNIT_EXPECT_EQ(test, panel_power_savings_store(device, NULL, "0",
count),
- (ssize_t)count);
+ (ssize_t)count);
KUNIT_EXPECT_EQ(test, state->abm_level, ABM_LEVEL_IMMEDIATE_DISABLE);
}
+static void dm_test_panel_power_savings_update_state_sets_disable(struct kunit
*test)
+{
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_connector_state *conn_state;
+ struct drm_atomic_commit *atomic;
+ struct dm_connector_state *state;
+ struct drm_connector *connector;
+ struct device *device;
+ int ret;
+
+ connector = setup_panel_power_savings_connector(test, &device, &state);
+ atomic = drm_atomic_commit_alloc(connector->dev);
+ KUNIT_ASSERT_NOT_NULL(test, atomic);
+
+ drm_modeset_acquire_init(&ctx, 0);
+ atomic->acquire_ctx = &ctx;
+
+ ret = panel_power_savings_update_state(atomic, connector, 0);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ conn_state = drm_atomic_get_new_connector_state(atomic, connector);
+ KUNIT_ASSERT_NOT_NULL(test, conn_state);
+ KUNIT_EXPECT_EQ(test, to_dm_connector_state(conn_state)->abm_level,
+ ABM_LEVEL_IMMEDIATE_DISABLE);
+
+ drm_atomic_commit_put(atomic);
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+}
+
/**
* dm_test_panel_power_savings_store_forbidden - Test forbidden update
* @test: The KUnit test context
@@ -633,6 +699,31 @@ static void
dm_test_panel_power_savings_store_forbidden(struct kunit *test)
KUNIT_EXPECT_EQ(test, panel_power_savings_store(device, NULL, "1", 1),
-EBUSY);
}
+static void dm_test_panel_power_savings_update_state_forbidden(struct kunit
*test)
+{
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_commit *atomic;
+ struct dm_connector_state *state;
+ struct drm_connector *connector;
+ struct device *device;
+ int ret;
+
+ connector = setup_panel_power_savings_connector(test, &device, &state);
+ state->abm_sysfs_forbidden = true;
+ atomic = drm_atomic_commit_alloc(connector->dev);
+ KUNIT_ASSERT_NOT_NULL(test, atomic);
+
+ drm_modeset_acquire_init(&ctx, 0);
+ atomic->acquire_ctx = &ctx;
+
+ ret = panel_power_savings_update_state(atomic, connector, 1);
+ KUNIT_EXPECT_EQ(test, ret, -EBUSY);
+
+ drm_atomic_commit_put(atomic);
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+}
+
/**
* dm_test_panel_power_savings_store_rejects_invalid_text - Test parse failure
* @test: The KUnit test context
@@ -1939,6 +2030,8 @@ static struct kunit_case dm_backlight_test_cases[] = {
KUNIT_CASE(dm_test_panel_power_savings_show_reports_level),
KUNIT_CASE(dm_test_panel_power_savings_store_sets_disable),
KUNIT_CASE(dm_test_panel_power_savings_store_forbidden),
+ KUNIT_CASE(dm_test_panel_power_savings_update_state_sets_disable),
+ KUNIT_CASE(dm_test_panel_power_savings_update_state_forbidden),
KUNIT_CASE(dm_test_panel_power_savings_store_rejects_invalid_text),
KUNIT_CASE(dm_test_panel_power_savings_store_rejects_out_of_range),
/* amdgpu_dm_backlight_get_device_index */
---
base-commit: ba496bb837882eb1af58f41572fc289e46c77b8c
change-id: 20261031-abm-sysfs-atomic-c79505f63a2e
Best regards,
--
Shivam Kalra <[email protected]>
