On 2025-12-01 18:18, [email protected] wrote:
> From: Leo Li <[email protected]>
>
> [Why]
>
> APU DCN generations since DCN3.5 have the capability to power down
> almost all of the DCN hw block during idle periods. This is referred to
> as IPS -- idle power states. In combination with a panel remote-buffer
> feature (like PSR or Panel Replay), IPS can save additional power.
>
> Once DCN is in an IPS, no register access can occur. This includes
> control registers for vblank interrupts; IPS must first be exited.
>
> Transitioning in or out of IPS requires synchronization with the rest of
> DC, as it powers up or down DCN, and may communicate with other MCUs on
> the SOC to do so. This is done via the dc_lock mutex.
>
> While calling enable_vblank, the DRM vblank core holds spinlocks that
> prevent blocking operations. Yet acquiring the dc_lock mutex is
> blocking. Thus, IPS can not be exited piror to programming vblank
> interrupt registers from within enable_vblank. At least not in a
> race-free way.
>
> Prior to this change, amdgpu_dm was exiting IPS(*) without holding the
> dc_lock, opening the door for races:
> https://gitlab.freedesktop.org/drm/amd/-/issues/5233
>
> (*) From touching the interrupt registers. All register reads today have
> an implicit IPS exit, see dm_read_reg_func()
>
> To solve this, the prepare_vblank_enable callback can be implemented to
> exit IPS, as it is called from process context.
>
> [How]
>
> Implement the prepare_vblank_enable callback for amdgpu_dm. In it,
> the dc_lock mutex is acquired, and IPS is exited.
>
> v2: Add missing semicolon, add docstring for prepare_vbl_disallow_idle
>
> Signed-off-by: Leo Li <[email protected]>
Reviewed-by: Harry Wentland <[email protected]>
Harry
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 9 +++++
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +++
> .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 36 +++++++++++++++++--
> 4 files changed, 49 insertions(+), 3 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 0346052f2e574..842a93e2d6ce0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9682,6 +9682,7 @@ static void amdgpu_dm_handle_vrr_transition(struct
> dm_crtc_state *old_state,
> * We also need vupdate irq for the actual core vblank handling
> * at end of vblank.
> */
> + WARN_ON(drm_crtc_vblank_prepare(new_state->base.crtc) != 0);
> WARN_ON(amdgpu_dm_crtc_set_vupdate_irq(new_state->base.crtc,
> true) != 0);
> WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
> drm_dbg_driver(new_state->base.crtc->dev, "%s: crtc=%u VRR
> off->on: Get vblank ref\n",
> @@ -10108,6 +10109,7 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
> */
> if (acrtc_attach->base.state->event &&
> acrtc_state->active_planes > 0) {
> + drm_crtc_vblank_prepare(pcrtc);
> drm_crtc_vblank_get(pcrtc);
>
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> @@ -10124,6 +10126,7 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
> &acrtc_state->stream->vrr_infopacket;
> }
> } else if (cursor_update && acrtc_state->active_planes > 0) {
> + drm_crtc_vblank_prepare(pcrtc);
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> if (acrtc_attach->base.state->event) {
> drm_crtc_vblank_get(pcrtc);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 7065b20aa2e6b..a99612fb3467a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -587,6 +587,15 @@ struct amdgpu_display_manager {
> */
> uint32_t active_vblank_irq_count;
>
> + /**
> + * @prepare_vbl_disallow_idle:
> + *
> + * Set to true when idle has been disallowed. Set to false when vblank
> + * interrupts have been enabled. i.e. idle re-allow on vblank disable is
> + * blocked if this is true.
> + */
> + bool prepare_vbl_disallow_idle;
> +
> #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
> /**
> * @secure_display_ctx:
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index e20aa74380665..7839b56859391 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -656,6 +656,10 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *src_name)
> */
> enabled = amdgpu_dm_is_valid_crc_source(cur_crc_src);
> if (!enabled && enable) {
> + ret = drm_crtc_vblank_prepare(crtc);
> + if (ret)
> + goto cleanup;
> +
> ret = drm_crtc_vblank_get(crtc);
> if (ret)
> goto cleanup;
> 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 38f9ea313dcbb..dd693419111db 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
> @@ -258,8 +258,8 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct
> work_struct *work)
> else if (dm->active_vblank_irq_count)
> dm->active_vblank_irq_count--;
>
> - if (dm->active_vblank_irq_count > 0)
> - dc_allow_idle_optimizations(dm->dc, false);
> + /* prepare_vblank_enable must disallow idle first */
> + ASSERT(dm->dc->idle_optimizations_allowed == false);
>
> /*
> * Control PSR based on vblank requirements from OS
> @@ -277,7 +277,13 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct
> work_struct *work)
> vblank_work->acrtc->dm_irq_params.allow_sr_entry);
> }
>
> - if (dm->active_vblank_irq_count == 0) {
> + /*
> + * If this worker runs disable between prepare_vblank and enable_vblank,
> + * we need to block idle re-allow. Leave it to the next vblank disable
> + * to re-allow idle.
> + */
> + if (dm->active_vblank_irq_count == 0 &&
> + !READ_ONCE(dm->prepare_vbl_disallow_idle)) {
> dc_post_update_surfaces_to_stream(dm->dc);
>
> r = amdgpu_dpm_pause_power_profile(adev, true);
> @@ -308,6 +314,8 @@ static inline int amdgpu_dm_crtc_set_vblank(struct
> drm_crtc *crtc, bool enable)
> int irq_type;
> int rc = 0;
>
> + ASSERT(dm->dc->idle_optimizations_allowed == false);
> +
> if (enable && !acrtc->base.enabled) {
> drm_dbg_vbl(crtc->dev,
> "Reject vblank enable on unconfigured CRTC %d
> (enabled=%d)\n",
> @@ -399,6 +407,9 @@ static inline int amdgpu_dm_crtc_set_vblank(struct
> drm_crtc *crtc, bool enable)
> }
> #endif
>
> + /* Ensure compiler emits the write before worker is queued */
> + WRITE_ONCE(dm->prepare_vbl_disallow_idle, false);
> +
> if (amdgpu_in_reset(adev))
> return 0;
>
> @@ -423,6 +434,24 @@ static inline int amdgpu_dm_crtc_set_vblank(struct
> drm_crtc *crtc, bool enable)
> return 0;
> }
>
> +static int amdgpu_prepare_enable_vblank(struct drm_crtc *crtc)
> +{
> + struct amdgpu_device *adev = drm_to_adev(crtc->dev);
> + struct amdgpu_display_manager *dm = &adev->dm;
> +
> + guard(mutex)(&adev->dm.dc_lock);
> +
> + if (dm->dc->idle_optimizations_allowed) {
> + /* Prevent the disable worker from re-allowing idle until
> + * interrupts are enabled. Ensure compiler emits the write
> + * before disallowing idle. */
> + WRITE_ONCE(dm->prepare_vbl_disallow_idle, true);
> + dc_allow_idle_optimizations(dm->dc, false);
> + }
> +
> + return 0;
> +}
> +
> int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
> {
> return amdgpu_dm_crtc_set_vblank(crtc, true);
> @@ -590,6 +619,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs =
> {
> .verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
> .get_crc_sources = amdgpu_dm_crtc_get_crc_sources,
> .get_vblank_counter = amdgpu_get_vblank_counter_kms,
> + .prepare_enable_vblank = amdgpu_prepare_enable_vblank,
> .enable_vblank = amdgpu_dm_crtc_enable_vblank,
> .disable_vblank = amdgpu_dm_crtc_disable_vblank,
> .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
