Re: [PATCH v2 2/2] drm/amd/display: Implement prepare_vblank_enable callback

[Harry Wentland <harry.wentland@amd.com>]

On 2025-12-01 18:18, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [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 <sunpeng.li@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

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,