[PATCH] drm/amd/display: Only allow idle optimizations when all CRTCs are idle

[PATCH] drm/amd/display: Only allow idle optimizations when all CRTCs are idle

[Claudio Forain]

[Why]
Idle optimizations are HW-wide, but the Idle State Manager (ISM) runs per
CRTC. When a CRTC's ISM reaches OPTIMIZED_IDLE it enables idle
optimizations globally as long as no CRTC holds a vblank reference
(dm->active_vblank_irq_count == 0). It does not check whether the ISMs
of the other CRTCs are idle.

With two displays on X11, one static and one receiving cursor updates,
the static CRTC's ISM enables idle optimizations while the cursor CRTC is
busy. Legacy cursor updates do not take a vblank reference, so nothing
vetoes it. On ASICs that set debug.exit_idle_opt_for_cursor_updates
(DCN30x, DCN32x, DCN401, ...), every call to
dc_stream_program_cursor_attributes() and
dc_stream_program_cursor_position() then exits and re-enters idle
optimizations itself. That is up to four synchronous DMUB commands per
cursor update, each taking up to ~1.6 ms on DCN401, all under dc_lock in
the X server's input thread. The cursor visibly speeds up and slows down
while the desktop is otherwise idle.

Measured on RX 9070 XT (DCN401), 240 Hz + 60 Hz displays, Xorg, cursor
moving continuously for 15 s (ftrace, cursor handler entry to
dc_stream_program_cursor_position entry):

                                 before      after
  cursor updates/s                1686        2572
  latency median / P95 / P99    144/858/927  17/19/87 us
  samples > 500 us                5460           0
  idle opt calls from cursor   58093           0
    programming
  dcn401_apply_idle_power_
    optimizations calls          58198           6

With a single display, or with any vblank requestor active, the issue
does not occur. Commit a1fc7bf6677e ("drm/amd/display: Restore 5s vbl
offdelay for NV3x+ DGPUs") hid the issue by keeping vblank enabled on the
cursor CRTC; it reappeared with commit f39283eab44f ("Revert
"drm/amd/display: Restore 5s vbl offdelay for NV3x+ DGPUs"").

[How]
Before allowing idle optimizations, also require every CRTC with an
active stream to be in OPTIMIZED_IDLE or OPTIMIZED_IDLE_SSO. The ISM
updates current_state before dispatching, so the last CRTC to go idle
still allows idle optimizations and no idle entry is lost. Idle
optimizations are still entered on a static desktop (173 entries in a
15 s static capture with the patch applied), idle GPU power is
unchanged (25.9 W before, 25.8 W after), and vblank enable/disable is
unchanged.

The problem was found and the fix written with an AI coding assistant,
working from ftrace captures taken on the affected system: function
tracer counts and caller attribution, and function_graph durations of
the cursor, ISM and DMUB paths. The assistant proposed the change and
drafted this changelog; the submitter reproduced the issue, built and
ran each test kernel, and confirmed the results.

Fixes: 754003486c3c ("drm/amd/display: Add Idle state manager(ISM)")
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/6115
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4906
Assisted-by: claude-opus-5-5
Signed-off-by: Claudio Forain <claudioforain@gmail.com>
---
Testing and limitations:
- Reproduced and tested on v7.3.3 (Gentoo) with RX 9070 XT (DCN401),
  Xorg, two DP displays (240 Hz + 60 Hz). The issue reproduces only with
  both displays active; disabling one makes stock 7.3.3 behave.
- Built and booted on v7.3.3. This posting is rebased onto
  amd-staging-drm-next, where it applies cleanly, but it has not been
  built or run on that tree.
- Not tested: other DCN generations, APUs, PSR/Replay panels (the
  OPTIMIZED_IDLE_SSO path), Wayland compositors, hotplug and
  suspend/resume with the patch, and the ISM KUnit tests.
- Idle GPU power on a static desktop (hwmon power1_average, 40 x 1 s):
  25.9 W before, 25.8 W after. Only checked on this setup.
- crtc->state is read under dc_lock only, like the existing
  to_dm_crtc_state(acrtc->base.state) in amdgpu_dm_ism_commit_event().

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
index 127eaba..79546ad 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
@@ -276,6 +276,32 @@ bool dm_ism_trigger_event(struct amdgpu_dm_ism *ism,
 EXPORT_IF_KUNIT(dm_ism_trigger_event);
 
 
+/*
+ * Idle optimizations are HW-wide, but each CRTC runs its own ISM. Only allow
+ * them once every active CRTC's ISM has reached an optimized idle state, so a
+ * static CRTC cannot enable idle optimizations while another CRTC is still busy
+ * (e.g. receiving cursor updates without a vblank reference). Otherwise every
+ * cursor update on the busy CRTC must exit and re-enter idle through DMUB.
+ */
+static bool dm_ism_all_crtcs_idle(struct amdgpu_display_manager *dm)
+{
+	struct drm_crtc *crtc;
+
+	drm_for_each_crtc(crtc, adev_to_drm(dm->adev)) {
+		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+		struct dm_crtc_state *state = to_dm_crtc_state(crtc->state);
+
+		if (!state || !state->stream)
+			continue;
+
+		if (acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE &&
+		    acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE_SSO)
+			return false;
+	}
+
+	return true;
+}
+
 static void dm_ism_commit_idle_optimization_state(struct amdgpu_dm_ism *ism,
 					     struct dc_stream_state *stream,
 					     bool vblank_enabled,
@@ -326,7 +352,8 @@ static void dm_ism_commit_idle_optimization_state(struct amdgpu_dm_ism *ism,
 	 * optimizations at the start of this func -- we should disallow
 	 * whenever there's *an* active CRTC.
 	 */
-	if (!vblank_enabled && dm->active_vblank_irq_count == 0) {
+	if (!vblank_enabled && dm->active_vblank_irq_count == 0 &&
+	    dm_ism_all_crtcs_idle(dm)) {
 		dc_post_update_surfaces_to_stream(dm->dc);
 		dc_allow_idle_optimizations(dm->dc, true);
 	}
-- 
2.x

Re: [PATCH] drm/amd/display: Only allow idle optimizations when all CRTCs are idle

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/amd/display: idle optimization stall in
  dm_ism_all_crtcs_idle()
- [High] drm/amd/display: unlocked crtc->state access in
  dm_ism_all_crtcs_idle()
- [Medium] drm/amd/display: NULL dereference in dm_ism_all_crtcs_idle()
  in KUnit tests
--

commit 41942a97fda7dd8d4e331b05631c7ae4c3d2cd19
Author: Claudio Forain <claudioforain@gmail.com>

drm/amd/display: Only allow idle optimizations when all CRTCs are idle

This commit adds dm_ism_all_crtcs_idle() so that hardware-wide idle
optimizations are only enabled when every active CRTC's Idle State Manager
is in an optimized idle state.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4906

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> index 127eaba4de60b..79546ad592874 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> @@ -276,6 +276,32 @@ bool dm_ism_trigger_event(struct amdgpu_dm_ism *ism,
>  EXPORT_IF_KUNIT(dm_ism_trigger_event);
>  
>  
> +/*
> + * Idle optimizations are HW-wide, but each CRTC runs its own ISM. Only allow
> + * them once every active CRTC's ISM has reached an optimized idle state, so a
> + * static CRTC cannot enable idle optimizations while another CRTC is still busy
> + * (e.g. receiving cursor updates without a vblank reference). Otherwise every
> + * cursor update on the busy CRTC must exit and re-enter idle through DMUB.
> + */
> +static bool dm_ism_all_crtcs_idle(struct amdgpu_display_manager *dm)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, adev_to_drm(dm->adev)) {

[Severity: Medium]
Does walking adev_to_drm(dm->adev)->mode_config.crtc_list here cause a NULL
pointer dereference in the amdgpu_dm_ism KUnit test suite?

Unlike amdgpu_dm_ism_disable(), amdgpu_dm_ism_force_full_power() and
amdgpu_dm_ism_enable(), which pass dm->ddev to drm_for_each_crtc(), this
uses adev_to_drm(dm->adev).  In amdgpu_dm_ism_test.c, alloc_test_acrtc() and
setup_commit_test() set adev->dm.ddev = &adev->ddev, but leave adev->dm.adev
NULL and do not initialize mode_config.crtc_list or register the test CRTC on
it, crashing the six behavioral commit and worker KUnit tests when they reach
dm_ism_commit_idle_optimization_state().

> +		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +		struct dm_crtc_state *state = to_dm_crtc_state(crtc->state);
> +
> +		if (!state || !state->stream)
> +			continue;

[Severity: High]
Can reading crtc->state and dereferencing state->stream across all CRTCs
under only dm->dc_lock race with an atomic commit on another CRTC and cause a
use-after-free?

dm_ism_all_crtcs_idle() runs from ISM delayed work or
amdgpu_dm_crtc_vblank_control_worker() holding dm->dc_lock, while crtc->state
is protected by crtc->mutex.  An atomic commit on another CRTC that does not
take dm->dc_lock in amdgpu_dm_atomic_commit_tail() can swap and free the old
CRTC state concurrently:

CPU0 (ISM worker on CRTC 0)           CPU1 (atomic commit on CRTC 1)
dm_ism_delayed_work_func()
  mutex_lock(&dm->dc_lock)
  ...
  dm_ism_all_crtcs_idle(dm)
    state = to_dm_crtc_state(
              crtc1->state)
                                      drm_atomic_helper_swap_state()
                                        crtc1->state = new_crtc_state
                                      ...
                                      drm_atomic_commit_put()
                                        amdgpu_dm_crtc_destroy_state()
                                          kfree(old_crtc_state)
    if (!state || !state->stream)  <-- use-after-free

> +
> +		if (acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE &&
> +		    acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE_SSO)
> +			return false;

[Severity: High]
Can requiring every active CRTC to be in DM_ISM_STATE_OPTIMIZED_IDLE or
DM_ISM_STATE_OPTIMIZED_IDLE_SSO permanently block hardware-wide idle
optimizations?

Both amdgpu_dm_ism_init() and amdgpu_dm_ism_force_full_power() (called on
suspend and GPU reset) leave each CRTC in DM_ISM_STATE_FULL_POWER_RUNNING.
The only transition toward DM_ISM_STATE_OPTIMIZED_IDLE is
DM_ISM_EVENT_ENTER_IDLE_REQUESTED, which is only sent from
amdgpu_dm_crtc_vblank_control_worker() when vblank is disabled.

When a CRTC is enabled from off via a modeset without a pageflip event, or
restored on resume or GPU reset, manage_dm_interrupts() calls
drm_crtc_vblank_on_config() without enabling vblank.  If that display stays
static and never takes and drops a vblank reference, its ISM remains in
DM_ISM_STATE_FULL_POWER_RUNNING and dm_ism_all_crtcs_idle() returns false
forever.

Similarly, in amdgpu_dm_commit_streams(), a modeset or disable on a CRTC in
DM_ISM_STATE_HYSTERESIS_WAITING calls amdgpu_dm_ism_flush(), which cancels
delayed_work without updating current_state or re-evaluating idle state for
the remaining active CRTCs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109230147.16302-1-claudioforain@gmail.com?part=1

Re: [PATCH] drm/amd/display: Only allow idle optimizations when all CRTCs are idle

[Cláudio Forain]

[-- Attachment #1: Type: text/plain, Size: 7459 bytes --]

Please don't apply this version.


The automated review at sashiko.dev found three real problems, which I
have confirmed against the source:

- dm_ism_all_crtcs_idle() reads crtc->state of other CRTCs under only
  dc_lock. A concurrent atomic commit on another CRTC can swap and free
  that state, so this can be a use-after-free.
- A CRTC that is enabled or resumed and stays static without ever taking
  a vblank reference remains in FULL_POWER_RUNNING, so idle
  optimizations could never be allowed again (a power regression).
- It uses adev_to_drm(dm->adev) instead of dm->ddev, which breaks the
  ISM KUnit commit tests.

The diagnosis in the changelog still stands: a static CRTC's ISM
enables HW-wide idle optimizations while another CRTC is receiving
legacy cursor updates. I'll send a v2 with a different approach, tested
with KUnit and across suspend/resume.

Thanks,
Claudio


On Mon, Nov 9, 2026 at 5:02 PM Claudio Forain <claudioforain@gmail.com>
wrote:

> [Why]
> Idle optimizations are HW-wide, but the Idle State Manager (ISM) runs per
> CRTC. When a CRTC's ISM reaches OPTIMIZED_IDLE it enables idle
> optimizations globally as long as no CRTC holds a vblank reference
> (dm->active_vblank_irq_count == 0). It does not check whether the ISMs
> of the other CRTCs are idle.
>
> With two displays on X11, one static and one receiving cursor updates,
> the static CRTC's ISM enables idle optimizations while the cursor CRTC is
> busy. Legacy cursor updates do not take a vblank reference, so nothing
> vetoes it. On ASICs that set debug.exit_idle_opt_for_cursor_updates
> (DCN30x, DCN32x, DCN401, ...), every call to
> dc_stream_program_cursor_attributes() and
> dc_stream_program_cursor_position() then exits and re-enters idle
> optimizations itself. That is up to four synchronous DMUB commands per
> cursor update, each taking up to ~1.6 ms on DCN401, all under dc_lock in
> the X server's input thread. The cursor visibly speeds up and slows down
> while the desktop is otherwise idle.
>
> Measured on RX 9070 XT (DCN401), 240 Hz + 60 Hz displays, Xorg, cursor
> moving continuously for 15 s (ftrace, cursor handler entry to
> dc_stream_program_cursor_position entry):
>
>                                  before      after
>   cursor updates/s                1686        2572
>   latency median / P95 / P99    144/858/927  17/19/87 us
>   samples > 500 us                5460           0
>   idle opt calls from cursor   58093           0
>     programming
>   dcn401_apply_idle_power_
>     optimizations calls          58198           6
>
> With a single display, or with any vblank requestor active, the issue
> does not occur. Commit a1fc7bf6677e ("drm/amd/display: Restore 5s vbl
> offdelay for NV3x+ DGPUs") hid the issue by keeping vblank enabled on the
> cursor CRTC; it reappeared with commit f39283eab44f ("Revert
> "drm/amd/display: Restore 5s vbl offdelay for NV3x+ DGPUs"").
>
> [How]
> Before allowing idle optimizations, also require every CRTC with an
> active stream to be in OPTIMIZED_IDLE or OPTIMIZED_IDLE_SSO. The ISM
> updates current_state before dispatching, so the last CRTC to go idle
> still allows idle optimizations and no idle entry is lost. Idle
> optimizations are still entered on a static desktop (173 entries in a
> 15 s static capture with the patch applied), idle GPU power is
> unchanged (25.9 W before, 25.8 W after), and vblank enable/disable is
> unchanged.
>
> The problem was found and the fix written with an AI coding assistant,
> working from ftrace captures taken on the affected system: function
> tracer counts and caller attribution, and function_graph durations of
> the cursor, ISM and DMUB paths. The assistant proposed the change and
> drafted this changelog; the submitter reproduced the issue, built and
> ran each test kernel, and confirmed the results.
>
> Fixes: 754003486c3c ("drm/amd/display: Add Idle state manager(ISM)")
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/6115
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4906
> Assisted-by: claude-opus-5-5
> Signed-off-by: Claudio Forain <claudioforain@gmail.com>
> ---
> Testing and limitations:
> - Reproduced and tested on v7.3.3 (Gentoo) with RX 9070 XT (DCN401),
>   Xorg, two DP displays (240 Hz + 60 Hz). The issue reproduces only with
>   both displays active; disabling one makes stock 7.3.3 behave.
> - Built and booted on v7.3.3. This posting is rebased onto
>   amd-staging-drm-next, where it applies cleanly, but it has not been
>   built or run on that tree.
> - Not tested: other DCN generations, APUs, PSR/Replay panels (the
>   OPTIMIZED_IDLE_SSO path), Wayland compositors, hotplug and
>   suspend/resume with the patch, and the ISM KUnit tests.
> - Idle GPU power on a static desktop (hwmon power1_average, 40 x 1 s):
>   25.9 W before, 25.8 W after. Only checked on this setup.
> - crtc->state is read under dc_lock only, like the existing
>   to_dm_crtc_state(acrtc->base.state) in amdgpu_dm_ism_commit_event().
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> index 127eaba..79546ad 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> @@ -276,6 +276,32 @@ bool dm_ism_trigger_event(struct amdgpu_dm_ism *ism,
>  EXPORT_IF_KUNIT(dm_ism_trigger_event);
>
>
> +/*
> + * Idle optimizations are HW-wide, but each CRTC runs its own ISM. Only
> allow
> + * them once every active CRTC's ISM has reached an optimized idle state,
> so a
> + * static CRTC cannot enable idle optimizations while another CRTC is
> still busy
> + * (e.g. receiving cursor updates without a vblank reference). Otherwise
> every
> + * cursor update on the busy CRTC must exit and re-enter idle through
> DMUB.
> + */
> +static bool dm_ism_all_crtcs_idle(struct amdgpu_display_manager *dm)
> +{
> +       struct drm_crtc *crtc;
> +
> +       drm_for_each_crtc(crtc, adev_to_drm(dm->adev)) {
> +               struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +               struct dm_crtc_state *state =
> to_dm_crtc_state(crtc->state);
> +
> +               if (!state || !state->stream)
> +                       continue;
> +
> +               if (acrtc->ism.current_state !=
> DM_ISM_STATE_OPTIMIZED_IDLE &&
> +                   acrtc->ism.current_state !=
> DM_ISM_STATE_OPTIMIZED_IDLE_SSO)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
>  static void dm_ism_commit_idle_optimization_state(struct amdgpu_dm_ism
> *ism,
>                                              struct dc_stream_state
> *stream,
>                                              bool vblank_enabled,
> @@ -326,7 +352,8 @@ static void
> dm_ism_commit_idle_optimization_state(struct amdgpu_dm_ism *ism,
>          * optimizations at the start of this func -- we should disallow
>          * whenever there's *an* active CRTC.
>          */
> -       if (!vblank_enabled && dm->active_vblank_irq_count == 0) {
> +       if (!vblank_enabled && dm->active_vblank_irq_count == 0 &&
> +           dm_ism_all_crtcs_idle(dm)) {
>                 dc_post_update_surfaces_to_stream(dm->dc);
>                 dc_allow_idle_optimizations(dm->dc, true);
>         }
> --
> 2.x
>

[-- Attachment #2: Type: text/html, Size: 9118 bytes --]