From: Ray Wu <[email protected]> [Why] ISM timers are not tied to the atomic commit, so a timer armed before a DPMS off can still fire after the stream is released. The external display check in dcn35_apply_idle_power_optimizations() loops over the active streams, so with none left it never runs and idle is allowed on an external-only system.
[How] Wait out pending and in-flight ISM work before dc_stream_release(); dc_lock is not held there, so the sync wait is safe. Rename amdgpu_dm_ism_fini() to amdgpu_dm_ism_flush() and assert dc_lock is not held. Assisted-by: Cursor:Claude-Opus-5 Reviewed-by: Leo Li <[email protected]> Signed-off-by: Ray Wu <[email protected]> Signed-off-by: Chenyu Chen <[email protected]> --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++ .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 6 +- .../drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c | 21 ++++- .../drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h | 2 +- .../amdgpu_dm/tests/amdgpu_dm_ism_test.c | 83 ++++++++++--------- 5 files changed, 72 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 9a585f8b3d67..607aa5ae9b67 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4612,6 +4612,13 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state, (!new_crtc_state->active || drm_atomic_crtc_needs_modeset(new_crtc_state))) { manage_dm_interrupts(adev, acrtc, NULL); + /* + * ISM hysteresis lives on system_dfl_wq, not the + * vblank workqueue. Wait it out so a timer armed while + * the stream existed cannot allow idle after the + * stream is released. + */ + amdgpu_dm_ism_flush(&acrtc->ism); dc_stream_release(dm_old_crtc_state->stream); } } diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c index 4b8530d734e5..a7979b418c2b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c @@ -480,9 +480,9 @@ EXPORT_IF_KUNIT(amdgpu_dm_crtc_duplicate_state); STATIC_IFN_KUNIT void amdgpu_dm_crtc_destroy(struct drm_crtc *crtc) { /* - * amdgpu_dm_ism_fini() is intentionally called in amdgpu_dm_fini(). - * It must be called before dc_destroy() in amdgpu_dm_fini() - * to avoid ISM accessing an invalid dc handle once dc is released. + * ISM workers are intentionally quiesced by amdgpu_dm_ism_disable() + * in amdgpu_dm_fini(). That must happen before dc_destroy() so ISM + * cannot access an invalid dc handle once dc is released. */ drm_crtc_cleanup(crtc); 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 4e57572e12b6..127eaba4de60 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 @@ -647,9 +647,26 @@ void amdgpu_dm_ism_init(struct amdgpu_dm_ism *ism, EXPORT_IF_KUNIT(amdgpu_dm_ism_init); -void amdgpu_dm_ism_fini(struct amdgpu_dm_ism *ism) +/** + * amdgpu_dm_ism_flush - Cancel any pending, or wait out in-flight ISM work + * + * @ism: The CRTC's idle state manager + * + * Cancels the hysteresis and SSO timers and waits for a running worker to + * finish. Callers that are about to drop the CRTC's stream use this so that a + * timer armed while the stream was still around cannot allow idle afterwards. + * + * Must not be called with dc_lock held: the workers take dc_lock themselves, + * so waiting for them under it would deadlock. + */ +void amdgpu_dm_ism_flush(struct amdgpu_dm_ism *ism) { + struct amdgpu_crtc *acrtc = ism_to_amdgpu_crtc(ism); + struct amdgpu_device *adev = drm_to_adev(acrtc->base.dev); + + lockdep_assert_not_held(&adev->dm.dc_lock); + cancel_delayed_work_sync(&ism->sso_delayed_work); cancel_delayed_work_sync(&ism->delayed_work); } -EXPORT_IF_KUNIT(amdgpu_dm_ism_fini); +EXPORT_IF_KUNIT(amdgpu_dm_ism_flush); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h index afce16f7085a..893e062bb281 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h @@ -142,7 +142,7 @@ struct amdgpu_dm_ism { void amdgpu_dm_ism_init(struct amdgpu_dm_ism *ism, struct amdgpu_dm_ism_config *config); -void amdgpu_dm_ism_fini(struct amdgpu_dm_ism *ism); +void amdgpu_dm_ism_flush(struct amdgpu_dm_ism *ism); void amdgpu_dm_ism_commit_event(struct amdgpu_dm_ism *ism, enum amdgpu_dm_ism_event event); void amdgpu_dm_ism_disable(struct amdgpu_display_manager *dm); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_ism_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_ism_test.c index b77df47d3095..a9c6485e2a9e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_ism_test.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_ism_test.c @@ -642,31 +642,6 @@ static void dm_test_ism_init_sets_initial_state(struct kunit *test) KUNIT_EXPECT_EQ(test, ism->config.sso_num_frames, config.sso_num_frames); } -/* ===== Tests for amdgpu_dm_ism_fini ===== */ - -/** - * dm_test_ism_fini_after_init - fini cancels never-scheduled work without error - * @test: KUnit test context - */ -static void dm_test_ism_fini_after_init(struct kunit *test) -{ - struct amdgpu_dm_ism *ism = alloc_test_ism(test); - struct amdgpu_dm_ism_config config = { - .filter_num_frames = 5, - .filter_entry_count = 3, - .activation_num_delay_frames = 10, - .sso_num_frames = 2, - }; - - amdgpu_dm_ism_init(ism, &config); - /* Work was never scheduled; cancel_delayed_work_sync is a no-op. */ - amdgpu_dm_ism_fini(ism); - - /* FSM state is untouched by fini */ - KUNIT_EXPECT_EQ(test, (int)ism->current_state, - (int)DM_ISM_STATE_FULL_POWER_RUNNING); -} - /* ===== Tests for dm_ism_set_last_idle_ts ===== */ /** @@ -897,6 +872,32 @@ static void register_test_acrtc(struct amdgpu_device *adev, list_add_tail(&acrtc->base.head, &adev->ddev.mode_config.crtc_list); } +/* ===== Tests for amdgpu_dm_ism_flush ===== */ + +/** + * dm_test_ism_flush_after_init - flush cancels never-scheduled work without error + * @test: KUnit test context + */ +static void dm_test_ism_flush_after_init(struct kunit *test) +{ + struct amdgpu_crtc *acrtc = alloc_test_acrtc(test, NULL); + struct amdgpu_dm_ism *ism = &acrtc->ism; + struct amdgpu_dm_ism_config config = { + .filter_num_frames = 5, + .filter_entry_count = 3, + .activation_num_delay_frames = 10, + .sso_num_frames = 2, + }; + + amdgpu_dm_ism_init(ism, &config); + /* Work was never scheduled; cancel_delayed_work_sync is a no-op. */ + amdgpu_dm_ism_flush(ism); + + /* FSM state is untouched by flush */ + KUNIT_EXPECT_EQ(test, (int)ism->current_state, + (int)DM_ISM_STATE_FULL_POWER_RUNNING); +} + /* ===== Tests for amdgpu_dm_ism_commit_event ===== */ /** @@ -922,7 +923,7 @@ static void dm_test_ism_commit_event_no_state(struct kunit *test) KUNIT_EXPECT_EQ(test, (int)acrtc->ism.current_state, (int)DM_ISM_STATE_FULL_POWER_RUNNING); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -958,7 +959,7 @@ static void dm_test_ism_commit_event_cursor_transition(struct kunit *test) KUNIT_EXPECT_EQ(test, (int)acrtc->ism.current_state, (int)DM_ISM_STATE_FULL_POWER_RUNNING); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -988,7 +989,7 @@ static void dm_test_ism_commit_event_invalid_event(struct kunit *test) KUNIT_EXPECT_EQ(test, (int)acrtc->ism.current_state, (int)DM_ISM_STATE_FULL_POWER_RUNNING); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /* ===== Tests for amdgpu_dm_ism_force_full_power ===== */ @@ -1020,7 +1021,7 @@ static void dm_test_ism_force_full_power(struct kunit *test) KUNIT_EXPECT_EQ(test, (int)acrtc->ism.current_state, (int)DM_ISM_STATE_FULL_POWER_RUNNING); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /* ===== Tests for amdgpu_dm_ism_disable / amdgpu_dm_ism_enable ===== */ @@ -1048,7 +1049,7 @@ static void dm_test_ism_disable_enable_cycle(struct kunit *test) KUNIT_EXPECT_EQ(test, (int)acrtc->ism.current_state, (int)DM_ISM_STATE_FULL_POWER_RUNNING); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /* ===== Tests for dm_ism_dispatch_power_state (via commit_event) ===== */ @@ -1127,7 +1128,7 @@ static void dm_test_ism_dispatch_hysteresis_schedule_and_cancel(struct kunit *te (int)DM_ISM_STATE_HYSTERESIS_BUSY); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1183,7 +1184,7 @@ static void dm_test_ism_dispatch_optimized_idle_defers_sso(struct kunit *test) cancel_delayed_work(&acrtc->ism.sso_delayed_work); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /* @@ -1285,7 +1286,7 @@ static void dm_test_ism_commit_allows_idle_on_optimized_idle(struct kunit *test) cancel_delayed_work(&acrtc->ism.sso_delayed_work); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1322,7 +1323,7 @@ static void dm_test_ism_commit_enables_sso_on_optimized_idle_sso(struct kunit *t KUNIT_EXPECT_TRUE(test, adev->dm.dc->idle_optimizations_allowed); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1371,7 +1372,7 @@ static void dm_test_ism_commit_disallows_idle_on_timer_aborted(struct kunit *tes KUNIT_EXPECT_FALSE(test, adev->dm.dc->idle_optimizations_allowed); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1413,7 +1414,7 @@ static void dm_test_ism_exit_from_optimized_idle_disallows_idle(struct kunit *te KUNIT_EXPECT_EQ(test, acrtc->ism.next_record_idx, 1); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1452,7 +1453,7 @@ static void dm_test_ism_exit_from_sso_disallows_idle(struct kunit *test) KUNIT_EXPECT_EQ(test, acrtc->ism.next_record_idx, 1); } - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1496,7 +1497,7 @@ static void dm_test_ism_delayed_work_runs_timer_elapsed(struct kunit *test) KUNIT_EXPECT_EQ(test, dm_ism_test_idle.calls, 1); KUNIT_EXPECT_TRUE(test, adev->dm.dc->idle_optimizations_allowed); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } /** @@ -1534,7 +1535,7 @@ static void dm_test_ism_sso_delayed_work_runs_sso_elapsed(struct kunit *test) KUNIT_EXPECT_EQ(test, dm_ism_test_idle.calls, 3); KUNIT_EXPECT_TRUE(test, adev->dm.dc->idle_optimizations_allowed); - amdgpu_dm_ism_fini(&acrtc->ism); + amdgpu_dm_ism_flush(&acrtc->ism); } static struct kunit_case dm_ism_test_cases[] = { @@ -1582,8 +1583,8 @@ static struct kunit_case dm_ism_test_cases[] = { KUNIT_CASE(dm_test_ism_idle_delay_entry_count_exceeds_history_size), /* amdgpu_dm_ism_init */ KUNIT_CASE(dm_test_ism_init_sets_initial_state), - /* amdgpu_dm_ism_fini */ - KUNIT_CASE(dm_test_ism_fini_after_init), + /* amdgpu_dm_ism_flush */ + KUNIT_CASE(dm_test_ism_flush_after_init), /* dm_ism_set_last_idle_ts */ KUNIT_CASE(dm_test_ism_set_last_idle_ts_updates_timestamp), /* dm_ism_insert_record */ -- 2.43.0
