On Tue, Aug 23, 2022 at 8:25 PM Yunxiang Li <Yunxiang.Li@xxxxxxx> 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. > + Rodrigo You might want to add: Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1380 This looks logical to me, but someone from the display team should take a look. Alex > Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx> > --- > 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 > > .../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 bc2493a2a90e..de80b61b8d8e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -7488,15 +7488,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); > @@ -8261,23 +8261,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,16 +8270,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 || > @@ -8304,16 +8300,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); > @@ -8325,12 +8324,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 > } > } > -- > 2.37.2 >