[PATCH v5] drm/amd/display: Fix vblank refcount in vrr transition
Rodrigo Siqueira Jordao
Rodrigo.Siqueira at amd.com
Mon Oct 3 19:12:01 UTC 2022
On 2022-09-21 17:20, 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.
>
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1247
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1380
>
> Signed-off-by: Yunxiang Li <Yunxiang.Li at amd.com>
> ---
> v2: check the return code for calls that might fail and warn on them
> v3/v4: make the sequence closer to the original and remove redundant local variables
> v5: add bug tracking info
>
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 55 +++++++++----------
> 1 file changed, 26 insertions(+), 29 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 ece2003a74cc..97cc8ceaeea0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7484,15 +7484,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);
> @@ -8257,23 +8257,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
> @@ -8283,16 +8266,29 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> #ifdef CONFIG_DEBUG_FS
> - bool configure_crc = false;
> enum amdgpu_dm_pipe_crc_source cur_crc_src;
> #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
> - struct crc_rd_work *crc_rd_wrk = dm->crc_rd_wrk;
> + struct crc_rd_work *crc_rd_wrk;
> +#endif
> +#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);
> +
> +#ifdef CONFIG_DEBUG_FS
> +#if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
> + crc_rd_wrk = dm->crc_rd_wrk;
> #endif
> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
> cur_crc_src = acrtc->dm_irq_params.crc_src;
> spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
> #endif
> - dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>
> if (new_crtc_state->active &&
> (!old_crtc_state->active ||
> @@ -8300,16 +8296,19 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> dc_stream_retain(dm_new_crtc_state->stream);
> acrtc->dm_irq_params.stream = dm_new_crtc_state->stream;
> manage_dm_interrupts(adev, acrtc, true);
> + }
> + /* Handle vrr on->off / off->on transitions */
> + amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state);
>
> #ifdef CONFIG_DEBUG_FS
> + if (new_crtc_state->active &&
> + (!old_crtc_state->active ||
> + drm_atomic_crtc_needs_modeset(new_crtc_state))) {
> /**
> * Frontend may have changed so reapply the CRC capture
> * settings for the stream.
> */
> - dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -
> if (amdgpu_dm_is_valid_crc_source(cur_crc_src)) {
> - configure_crc = true;
> #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
> if (amdgpu_dm_crc_window_is_activated(crtc)) {
> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
> @@ -8321,12 +8320,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
> }
> #endif
> - }
> -
> - if (configure_crc)
> if (amdgpu_dm_crtc_configure_crc_source(
> crtc, dm_new_crtc_state, cur_crc_src))
> DRM_DEBUG_DRIVER("Failed to configure crc source");
> + }
> #endif
> }
> }
Hi Yunxiang,
This patch lgtm:
Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
Daniel also tested it a few days ago in multiple ASICs and with IGT.
I already applied this change to amd-staging-drm-next.
Thanks.
Siqueira
More information about the amd-gfx
mailing list