* [PATCH] drm/amd/display: use atomic commit for panel power savings
@ 2026-10-30 21:58 ` Shivam Kalra
0 siblings, 0 replies; 3+ messages in thread
From: Shivam Kalra via B4 Relay @ 2026-10-30 21:58 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, David Airlie, Simona Vetter, Hamza Mahfooz,
Mario Limonciello
Cc: amd-gfx, dri-devel, linux-kernel, David Weber, Shivam Kalra
From: Shivam Kalra <shivamkalra98@zohomail.in>
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 <shivamkalra98@zohomail.in>
---
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.
[1] https://lore.kernel.org/amd-gfx/20260813112634.11455-1-weber.aulendorf@gmail.com/
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);
+ 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);
+ if (ret == -EDEADLK)
+ goto backoff;
if (ret)
- return ret;
+ goto out_state;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ 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 <shivamkalra98@zohomail.in>
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH] drm/amd/display: use atomic commit for panel power savings
@ 2026-10-30 21:58 ` Shivam Kalra
0 siblings, 0 replies; 3+ messages in thread
From: Shivam Kalra @ 2026-10-30 21:58 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, David Airlie, Simona Vetter, Hamza Mahfooz,
Mario Limonciello
Cc: amd-gfx, dri-devel, linux-kernel, David Weber, Shivam Kalra
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 <shivamkalra98@zohomail.in>
---
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.
[1] https://lore.kernel.org/amd-gfx/20260813112634.11455-1-weber.aulendorf@gmail.com/
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);
+ 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);
+ if (ret == -EDEADLK)
+ goto backoff;
if (ret)
- return ret;
+ goto out_state;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ 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 <shivamkalra98@zohomail.in>
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] drm/amd/display: use atomic commit for panel power savings
2026-10-30 21:58 ` Shivam Kalra
(?)
@ 2026-10-30 22:19 ` sashiko-bot
-1 siblings, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-30 22:19 UTC (permalink / raw)
To: Shivam Kalra; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drm/amd/display: commit message claims non-modeset stream update path
- [Low] drm/amd/display: duplicate connector state helpers in backlight KUnit test
--
commit 60bc0c08268c70e96bfcf62cb19e21a33e151366
Author: Shivam Kalra <shivamkalra98@zohomail.in>
drm/amd/display: use atomic commit for panel power savings
This commit updates panel_power_savings_store() to apply ABM level changes
through an atomic commit instead of mutating connector->state directly and
emitting a hotplug uevent, and extends the backlight KUnit test suite.
> drm/amd/display: use atomic commit for panel power savings
>
> 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 <shivamkalra98@zohomail.in>
[Severity: Low]
This isn't a bug, but is the commit message accurate when it says that this
version avoids a modeset by using the normal atomic stream update path?
When panel_power_savings_store() calls drm_atomic_commit(state) in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c,
amdgpu_dm_atomic_check() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
marks connectors_changed when abm_level changes:
if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
dm_old_con_state->scaling != dm_new_con_state->scaling)
new_crtc_state->connectors_changed = true;
Because connectors_changed is set, drm_atomic_crtc_needs_modeset() returns
true, causing amdgpu_dm_atomic_commit_tail() to perform a full modeset and
skip the non-modeset connector stream update loop:
/* Skip any modesets/resets */
if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
continue;
The note below the "---" separator mentions that standalone operation still
performs a modeset and depends on an unmerged companion series to use the
non-modeset stream update path, but that note is stripped when the patch is
applied. Should the commit message itself describe the standalone modeset
behavior?
> 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 c6d15fb8ad989..a482261e85d25 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,
> +};
[Severity: Low]
This isn't a bug, but could dm_test_panel_power_savings_connector_funcs in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c reuse
amdgpu_dm_connector_atomic_duplicate_state() and
drm_atomic_helper_connector_destroy_state() instead of defining
dm_test_panel_power_savings_duplicate_state() and
dm_test_panel_power_savings_destroy_state()?
In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c,
amdgpu_dm_connector_atomic_duplicate_state() is already exported with
EXPORT_IF_KUNIT and performs the same kmemdup() and
__drm_atomic_helper_connector_duplicate_state() sequence, and
dm_test_connector_funcs in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c uses
both helpers directly:
static const struct drm_connector_funcs dm_test_connector_funcs = {
.reset = amdgpu_dm_connector_funcs_reset,
.atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-abm-sysfs-atomic-v1-1-a99068f3b49c@zohomail.in?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-30 22:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-30 21:58 [PATCH] drm/amd/display: use atomic commit for panel power savings Shivam Kalra via B4 Relay
2026-10-30 21:58 ` Shivam Kalra
2026-10-30 22:19 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.