All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/amd/display: Fix vblank refcount in vrr transition
@ 2022-07-28 13:49 Yunxiang Li
  2022-08-03 20:39 ` Harry Wentland
  0 siblings, 1 reply; 2+ messages in thread
From: Yunxiang Li @ 2022-07-28 13:49 UTC (permalink / raw)
  To: amd-gfx; +Cc: Yunxiang Li

manage_dm_interrupts disable/enable vblank using drm_crtc_vblank_off/on
which causes drm_crtc_vblank_get in vrr_transition to fail, and later
when drm_crtc_vblank_put is called the refcount on vblank will be messed
up. Therefore move the call to after manage_dm_interrupts.

Unchecked calls to drm_crtc_vblank_get seems to be common in other
drivers as well so it may make sense to let get always succeed during
modset, see
https://lists.freedesktop.org/archives/dri-devel/2022-July/365589.html

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
---
v2: check the return code for calls that might fail and warn on them

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 ++++++++-----------
 1 file changed, 14 insertions(+), 20 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 92470a0e0262..9f3fdb92e7a4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7480,15 +7480,15 @@ 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.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, true);
-		drm_crtc_vblank_get(new_state->base.crtc);
+		WARN_ON(dm_set_vupdate_irq(new_state->base.crtc, true) != 0);
+		WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
 		DRM_DEBUG_DRIVER("%s: crtc=%u VRR off->on: Get vblank ref\n",
 				 __func__, new_state->base.crtc->base.id);
 	} else if (old_vrr_active && !new_vrr_active) {
 		/* Transition VRR active -> inactive:
 		 * Allow vblank irq disable again for fixed refresh rate.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, false);
+		WARN_ON(dm_set_vupdate_irq(new_state->base.crtc, false) != 0);
 		drm_crtc_vblank_put(new_state->base.crtc);
 		DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
 				 __func__, new_state->base.crtc->base.id);
@@ -8252,23 +8252,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
-	/* Count number of newly disabled CRTCs for dropping PM refs later. */
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
-				      new_crtc_state, i) {
-		if (old_crtc_state->active && !new_crtc_state->active)
-			crtc_disable_count++;
-
-		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
-
-		/* For freesync config update on crtc state and params for irq */
-		update_stream_irq_parameters(dm, dm_new_crtc_state);
-
-		/* Handle vrr on->off / off->on transitions */
-		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
-						dm_new_crtc_state);
-	}
-
 	/**
 	 * Enable interrupts for CRTCs that are newly enabled or went through
 	 * a modeset. It was intentionally deferred until after the front end
@@ -8287,7 +8270,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		cur_crc_src = acrtc->dm_irq_params.crc_src;
 		spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
 #endif
+		/* Count number of newly disabled CRTCs for dropping PM refs later. */
+		if (old_crtc_state->active && !new_crtc_state->active)
+			crtc_disable_count++;
+
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
+
+		/* For freesync config update on crtc state and params for irq */
+		update_stream_irq_parameters(dm, dm_new_crtc_state);
 
 		if (new_crtc_state->active &&
 		    (!old_crtc_state->active ||
@@ -8324,6 +8315,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 					DRM_DEBUG_DRIVER("Failed to configure crc source");
 #endif
 		}
+
+		/* Handle vrr on->off / off->on transitions */
+		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state);
 	}
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH v2] drm/amd/display: Fix vblank refcount in vrr transition
  2022-07-28 13:49 [PATCH v2] drm/amd/display: Fix vblank refcount in vrr transition Yunxiang Li
@ 2022-08-03 20:39 ` Harry Wentland
  0 siblings, 0 replies; 2+ messages in thread
From: Harry Wentland @ 2022-08-03 20:39 UTC (permalink / raw)
  To: Yunxiang Li, amd-gfx, Mario Kleiner, Nicholas Kazlauskas

On 2022-07-28 09:49, Yunxiang Li wrote:
> manage_dm_interrupts disable/enable vblank using drm_crtc_vblank_off/on
> which causes drm_crtc_vblank_get in vrr_transition to fail, and later
> when drm_crtc_vblank_put is called the refcount on vblank will be messed
> up. Therefore move the call to after manage_dm_interrupts.
> 
> Unchecked calls to drm_crtc_vblank_get seems to be common in other
> drivers as well so it may make sense to let get always succeed during
> modset, see
> https://lists.freedesktop.org/archives/dri-devel/2022-July/365589.html
> 

Thanks for this patch.

This change makes sense to me but I would love Mario and/or Nick to take
a look as well, if possible. Would prefer if we hold off on the merge and
give them a chance to chime in.

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

Harry

> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> ---
> v2: check the return code for calls that might fail and warn on them	
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 ++++++++-----------
>  1 file changed, 14 insertions(+), 20 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 92470a0e0262..9f3fdb92e7a4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7480,15 +7480,15 @@ 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.
>  		 */
> -		dm_set_vupdate_irq(new_state->base.crtc, true);
> -		drm_crtc_vblank_get(new_state->base.crtc);
> +		WARN_ON(dm_set_vupdate_irq(new_state->base.crtc, true) != 0);
> +		WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
>  		DRM_DEBUG_DRIVER("%s: crtc=%u VRR off->on: Get vblank ref\n",
>  				 __func__, new_state->base.crtc->base.id);
>  	} else if (old_vrr_active && !new_vrr_active) {
>  		/* Transition VRR active -> inactive:
>  		 * Allow vblank irq disable again for fixed refresh rate.
>  		 */
> -		dm_set_vupdate_irq(new_state->base.crtc, false);
> +		WARN_ON(dm_set_vupdate_irq(new_state->base.crtc, false) != 0);
>  		drm_crtc_vblank_put(new_state->base.crtc);
>  		DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
>  				 __func__, new_state->base.crtc->base.id);
> @@ -8252,23 +8252,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  		mutex_unlock(&dm->dc_lock);
>  	}
>  
> -	/* Count number of newly disabled CRTCs for dropping PM refs later. */
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> -				      new_crtc_state, i) {
> -		if (old_crtc_state->active && !new_crtc_state->active)
> -			crtc_disable_count++;
> -
> -		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> -
> -		/* For freesync config update on crtc state and params for irq */
> -		update_stream_irq_parameters(dm, dm_new_crtc_state);
> -
> -		/* Handle vrr on->off / off->on transitions */
> -		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
> -						dm_new_crtc_state);
> -	}
> -
>  	/**
>  	 * Enable interrupts for CRTCs that are newly enabled or went through
>  	 * a modeset. It was intentionally deferred until after the front end
> @@ -8287,7 +8270,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  		cur_crc_src = acrtc->dm_irq_params.crc_src;
>  		spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
>  #endif
> +		/* Count number of newly disabled CRTCs for dropping PM refs later. */
> +		if (old_crtc_state->active && !new_crtc_state->active)
> +			crtc_disable_count++;
> +
>  		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> +		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> +
> +		/* For freesync config update on crtc state and params for irq */
> +		update_stream_irq_parameters(dm, dm_new_crtc_state);
>  
>  		if (new_crtc_state->active &&
>  		    (!old_crtc_state->active ||
> @@ -8324,6 +8315,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  					DRM_DEBUG_DRIVER("Failed to configure crc source");
>  #endif
>  		}
> +
> +		/* Handle vrr on->off / off->on transitions */
> +		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state);
>  	}
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-08-03 20:39 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-28 13:49 [PATCH v2] drm/amd/display: Fix vblank refcount in vrr transition Yunxiang Li
2022-08-03 20:39 ` Harry Wentland

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.