<html dir="ltr"><head></head><body style="text-align: left; direction: ltr; word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" bgcolor="#ffffff" text="#333333" link="#2679db" vlink="#202020"><div>On Wed, 2020-09-09 at 11:00 -0400, Kazlauskas, Nicholas wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>On 2020-09-09 10:28 a.m., Aurabindo Pillai wrote:</pre><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>[Why&How]</pre><pre>Currently commit_tail holds global locks and wait for dependencies which is</pre><pre>against the DRM API contracts. Inorder to fix this, IRQ handler should be able</pre><pre>to run without having to access crtc state. Required parameters are copied over</pre><pre>so that they can be directly accessed from the interrupt handler</pre><pre> </pre><pre>Signed-off-by: Aurabindo Pillai <</pre><a href="mailto:aurabindo.pillai@amd.com"><pre>aurabindo.pillai@amd.com</pre></a><pre>></pre><pre>---</pre><pre> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 115 ++++++++++--------</pre><pre> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -</pre><pre> .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 4 +</pre><pre> 3 files changed, 68 insertions(+), 52 deletions(-)</pre><pre> </pre><pre>diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c</pre><pre>index 40814cdd8c92..0603436a3313 100644</pre><pre>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c</pre><pre>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c</pre><pre>@@ -192,17 +192,14 @@ static u32 dm_vblank_get_counter(struct amdgpu_device *adev, int crtc)</pre><pre> return 0;</pre><pre> else {</pre><pre> struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];</pre><pre>- struct dm_crtc_state *acrtc_state = to_dm_crtc_state(</pre><pre>- acrtc->base.state);</pre><pre> </pre><pre>-</pre><pre>- if (acrtc_state->stream == NULL) {</pre><pre>+ if (acrtc->dm_irq_params.stream == NULL) {</pre><pre> DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",</pre><pre> crtc);</pre><pre> return 0;</pre><pre> }</pre><pre> </pre><pre>- return dc_stream_get_vblank_counter(acrtc_state->stream);</pre><pre>+ return dc_stream_get_vblank_counter(acrtc->dm_irq_params.stream);</pre><pre> }</pre><pre> }</pre><pre> </pre><pre>@@ -215,10 +212,8 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,</pre><pre> return -EINVAL;</pre><pre> else {</pre><pre> struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];</pre><pre>- struct dm_crtc_state *acrtc_state = to_dm_crtc_state(</pre><pre>- acrtc->base.state);</pre><pre> </pre><pre>- if (acrtc_state->stream == NULL) {</pre><pre>+ if (acrtc->dm_irq_params.stream == NULL) {</pre><pre> DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",</pre><pre> crtc);</pre><pre> return 0;</pre><pre>@@ -228,7 +223,7 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,</pre><pre> * TODO rework base driver to use values directly.</pre><pre> * for now parse it back into reg-format</pre><pre> */</pre><pre>- dc_stream_get_scanoutpos(acrtc_state->stream,</pre><pre>+ dc_stream_get_scanoutpos(acrtc->dm_irq_params.stream,</pre><pre> &v_blank_start,</pre><pre> &v_blank_end,</pre><pre> &h_position,</pre><pre>@@ -287,6 +282,14 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,</pre><pre> return NULL;</pre><pre> }</pre><pre> </pre><pre>+static inline bool amdgpu_dm_vrr_active_irq(struct amdgpu_crtc *acrtc)</pre><pre>+{</pre><pre>+ return acrtc->dm_irq_params.freesync_config.state ==</pre><pre>+ VRR_STATE_ACTIVE_VARIABLE ||</pre><pre>+ acrtc->dm_irq_params.freesync_config.state ==</pre><pre>+ VRR_STATE_ACTIVE_FIXED;</pre><pre>+}</pre><pre>+</pre><pre> static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)</pre><pre> {</pre><pre> return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||</pre><pre>@@ -307,7 +310,6 @@ static void dm_pflip_high_irq(void *interrupt_params)</pre><pre> struct amdgpu_device *adev = irq_params->adev;</pre><pre> unsigned long flags;</pre><pre> struct drm_pending_vblank_event *e;</pre><pre>- struct dm_crtc_state *acrtc_state;</pre><pre> uint32_t vpos, hpos, v_blank_start, v_blank_end;</pre><pre> bool vrr_active;</pre><pre> </pre><pre>@@ -339,12 +341,11 @@ static void dm_pflip_high_irq(void *interrupt_params)</pre><pre> if (!e)</pre><pre> WARN_ON(1);</pre><pre> </pre><pre>- acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);</pre><pre>- vrr_active = amdgpu_dm_vrr_active(acrtc_state);</pre><pre>+ vrr_active = amdgpu_dm_vrr_active_irq(amdgpu_crtc);</pre><pre> </pre><pre> /* Fixed refresh rate, or VRR scanout position outside front-porch? */</pre><pre> if (!vrr_active ||</pre><pre>- !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,</pre><pre>+ !dc_stream_get_scanoutpos(amdgpu_crtc->dm_irq_params.stream, &v_blank_start,</pre><pre> &v_blank_end, &hpos, &vpos) ||</pre><pre> (vpos < v_blank_start)) {</pre><pre> /* Update to correct count and vblank timestamp if racing with</pre><pre>@@ -405,17 +406,17 @@ static void dm_vupdate_high_irq(void *interrupt_params)</pre><pre> struct common_irq_params *irq_params = interrupt_params;</pre><pre> struct amdgpu_device *adev = irq_params->adev;</pre><pre> struct amdgpu_crtc *acrtc;</pre><pre>- struct dm_crtc_state *acrtc_state;</pre><pre> unsigned long flags;</pre><pre>+ int vrr_active;</pre><pre> </pre><pre> acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);</pre><pre> </pre><pre> if (acrtc) {</pre><pre>- acrtc_state = to_dm_crtc_state(acrtc->base.state);</pre><pre>+ vrr_active = amdgpu_dm_vrr_active_irq(acrtc);</pre><pre> </pre><pre> DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d\n",</pre><pre> acrtc->crtc_id,</pre><pre>- amdgpu_dm_vrr_active(acrtc_state));</pre><pre>+ vrr_active);</pre><pre> </pre><pre> /* Core vblank handling is done here after end of front-porch in</pre><pre> * vrr mode, as vblank timestamping will give valid results</pre><pre>@@ -423,22 +424,22 @@ static void dm_vupdate_high_irq(void *interrupt_params)</pre><pre> * page-flip completion events that have been queued to us</pre><pre> * if a pageflip happened inside front-porch.</pre><pre> */</pre><pre>- if (amdgpu_dm_vrr_active(acrtc_state)) {</pre><pre>+ if (vrr_active) {</pre><pre> drm_crtc_handle_vblank(&acrtc->base);</pre><pre> </pre><pre> /* BTR processing for pre-DCE12 ASICs */</pre><pre>- if (acrtc_state->stream &&</pre><pre>+ if (acrtc->dm_irq_params.stream &&</pre><pre> adev->family < AMDGPU_FAMILY_AI) {</pre><pre> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);</pre><pre> mod_freesync_handle_v_update(</pre><pre> adev->dm.freesync_module,</pre><pre>- acrtc_state->stream,</pre><pre>- &acrtc_state->vrr_params);</pre><pre>+ acrtc->dm_irq_params.stream,</pre><pre>+ &acrtc->dm_irq_params.vrr_params);</pre><pre> </pre><pre> dc_stream_adjust_vmin_vmax(</pre><pre> adev->dm.dc,</pre><pre>- acrtc_state->stream,</pre><pre>- &acrtc_state->vrr_params.adjust);</pre><pre>+ acrtc->dm_irq_params.stream,</pre><pre>+ &acrtc->dm_irq_params.vrr_params.adjust);</pre><pre> spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);</pre><pre> }</pre><pre> }</pre><pre>@@ -457,18 +458,17 @@ static void dm_crtc_high_irq(void *interrupt_params)</pre><pre> struct common_irq_params *irq_params = interrupt_params;</pre><pre> struct amdgpu_device *adev = irq_params->adev;</pre><pre> struct amdgpu_crtc *acrtc;</pre><pre>- struct dm_crtc_state *acrtc_state;</pre><pre> unsigned long flags;</pre><pre>+ int vrr_active;</pre><pre> </pre><pre> acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);</pre><pre> if (!acrtc)</pre><pre> return;</pre><pre> </pre><pre>- acrtc_state = to_dm_crtc_state(acrtc->base.state);</pre><pre>+ vrr_active = amdgpu_dm_vrr_active_irq(acrtc);</pre><pre> </pre><pre> DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d, planes:%d\n", acrtc->crtc_id,</pre><pre>- amdgpu_dm_vrr_active(acrtc_state),</pre><pre>- acrtc_state->active_planes);</pre><pre>+ vrr_active, acrtc->dm_irq_params.active_planes);</pre><pre> </pre><pre> /**</pre><pre> * Core vblank handling at start of front-porch is only possible</pre><pre>@@ -476,7 +476,7 @@ static void dm_crtc_high_irq(void *interrupt_params)</pre><pre> * valid results while done in front-porch. Otherwise defer it</pre><pre> * to dm_vupdate_high_irq after end of front-porch.</pre><pre> */</pre><pre>- if (!amdgpu_dm_vrr_active(acrtc_state))</pre><pre>+ if (!vrr_active)</pre><pre> drm_crtc_handle_vblank(&acrtc->base);</pre><pre> </pre><pre> /**</pre><pre>@@ -491,14 +491,16 @@ static void dm_crtc_high_irq(void *interrupt_params)</pre><pre> </pre><pre> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);</pre><pre> </pre><pre>- if (acrtc_state->stream && acrtc_state->vrr_params.supported &&</pre><pre>- acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {</pre><pre>+ if (acrtc->dm_irq_params.stream &&</pre><pre>+ acrtc->dm_irq_params.vrr_params.supported &&</pre><pre>+ acrtc->dm_irq_params.freesync_config.state ==</pre><pre>+ VRR_STATE_ACTIVE_VARIABLE) {</pre><pre> mod_freesync_handle_v_update(adev->dm.freesync_module,</pre><pre>- acrtc_state->stream,</pre><pre>- &acrtc_state->vrr_params);</pre><pre>+ acrtc->dm_irq_params.stream,</pre><pre>+ &acrtc->dm_irq_params.vrr_params);</pre><pre> </pre><pre>- dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc_state->stream,</pre><pre>- &acrtc_state->vrr_params.adjust);</pre><pre>+ dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc->dm_irq_params.stream,</pre><pre>+ &acrtc->dm_irq_params.vrr_params.adjust);</pre><pre> }</pre><pre> </pre><pre> /*</pre><pre>@@ -513,7 +515,7 @@ static void dm_crtc_high_irq(void *interrupt_params)</pre><pre> */</pre><pre> if (adev->family >= AMDGPU_FAMILY_RV &&</pre><pre> acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&</pre><pre>- acrtc_state->active_planes == 0) {</pre><pre>+ acrtc->dm_irq_params.active_planes == 0) {</pre><pre> if (acrtc->event) {</pre><pre> drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);</pre><pre> acrtc->event = NULL;</pre><pre>@@ -4845,7 +4847,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)</pre><pre> }</pre><pre> </pre><pre> state->active_planes = cur->active_planes;</pre><pre>- state->vrr_params = cur->vrr_params;</pre><pre> state->vrr_infopacket = cur->vrr_infopacket;</pre><pre> state->abm_level = cur->abm_level;</pre><pre> state->vrr_supported = cur->vrr_supported;</pre><pre>@@ -6913,6 +6914,7 @@ static void update_freesync_state_on_stream(</pre><pre> struct mod_vrr_params vrr_params;</pre><pre> struct dc_info_packet vrr_infopacket = {0};</pre><pre> struct amdgpu_device *adev = dm->adev;</pre><pre>+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);</pre><pre> unsigned long flags;</pre><pre> </pre><pre> if (!new_stream)</pre><pre>@@ -6927,7 +6929,7 @@ static void update_freesync_state_on_stream(</pre><pre> return;</pre><pre> </pre><pre> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);</pre><pre>- vrr_params = new_crtc_state->vrr_params;</pre><pre>+ vrr_params = acrtc->dm_irq_params.vrr_params;</pre><pre> </pre><pre> if (surface) {</pre><pre> mod_freesync_handle_preflip(</pre><pre>@@ -6958,7 +6960,7 @@ static void update_freesync_state_on_stream(</pre><pre> &vrr_infopacket);</pre><pre> </pre><pre> new_crtc_state->freesync_timing_changed |=</pre><pre>- (memcmp(&new_crtc_state->vrr_params.adjust,</pre><pre>+ (memcmp(&acrtc->dm_irq_params.vrr_params.adjust,</pre><pre> &vrr_params.adjust,</pre><pre> sizeof(vrr_params.adjust)) != 0);</pre><pre> </pre><pre>@@ -6967,10 +6969,10 @@ static void update_freesync_state_on_stream(</pre><pre> &vrr_infopacket,</pre><pre> sizeof(vrr_infopacket)) != 0);</pre><pre> </pre><pre>- new_crtc_state->vrr_params = vrr_params;</pre><pre>+ acrtc->dm_irq_params.vrr_params = vrr_params;</pre><pre> new_crtc_state->vrr_infopacket = vrr_infopacket;</pre><pre> </pre><pre>- new_stream->adjust = new_crtc_state->vrr_params.adjust;</pre><pre>+ new_stream->adjust = acrtc->dm_irq_params.vrr_params.adjust;</pre><pre> new_stream->vrr_infopacket = vrr_infopacket;</pre><pre> </pre><pre> if (new_crtc_state->freesync_vrr_info_changed)</pre><pre>@@ -6982,7 +6984,7 @@ static void update_freesync_state_on_stream(</pre><pre> spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);</pre><pre> }</pre><pre> </pre><pre>-static void pre_update_freesync_state_on_stream(</pre><pre>+static void update_stream_irq_parameters(</pre><pre> struct amdgpu_display_manager *dm,</pre><pre> struct dm_crtc_state *new_crtc_state)</pre><pre> {</pre><pre>@@ -6990,6 +6992,7 @@ static void pre_update_freesync_state_on_stream(</pre><pre> struct mod_vrr_params vrr_params;</pre><pre> struct mod_freesync_config config = new_crtc_state->freesync_config;</pre><pre> struct amdgpu_device *adev = dm->adev;</pre><pre>+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);</pre><pre> unsigned long flags;</pre><pre> </pre><pre> if (!new_stream)</pre><pre>@@ -7003,7 +7006,7 @@ static void pre_update_freesync_state_on_stream(</pre><pre> return;</pre><pre> </pre><pre> spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);</pre><pre>- vrr_params = new_crtc_state->vrr_params;</pre><pre>+ vrr_params = acrtc->dm_irq_params.vrr_params;</pre><pre> </pre><pre> if (new_crtc_state->vrr_supported &&</pre><pre> config.min_refresh_in_uhz &&</pre><pre>@@ -7020,11 +7023,14 @@ static void pre_update_freesync_state_on_stream(</pre><pre> &config, &vrr_params);</pre><pre> </pre><pre> new_crtc_state->freesync_timing_changed |=</pre><pre>- (memcmp(&new_crtc_state->vrr_params.adjust,</pre><pre>- &vrr_params.adjust,</pre><pre>- sizeof(vrr_params.adjust)) != 0);</pre><pre>+ (memcmp(&acrtc->dm_irq_params.vrr_params.adjust,</pre><pre>+ &vrr_params.adjust, sizeof(vrr_params.adjust)) != 0);</pre><pre> </pre><pre>- new_crtc_state->vrr_params = vrr_params;</pre><pre>+ new_crtc_state->freesync_config = config;</pre><pre>+ /* Copy state for access from DM IRQ handler */</pre><pre>+ acrtc->dm_irq_params.freesync_config = config;</pre><pre>+ acrtc->dm_irq_params.active_planes = new_crtc_state->active_planes;</pre><pre>+ acrtc->dm_irq_params.vrr_params = vrr_params;</pre><pre> spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);</pre><pre> }</pre><pre> </pre><pre>@@ -7332,7 +7338,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,</pre><pre> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);</pre><pre> dc_stream_adjust_vmin_vmax(</pre><pre> dm->dc, acrtc_state->stream,</pre><pre>- &acrtc_state->vrr_params.adjust);</pre><pre>+ &acrtc_attach->dm_irq_params.vrr_params.adjust);</pre><pre> spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);</pre><pre> }</pre><pre> mutex_lock(&dm->dc_lock);</pre><pre>@@ -7545,6 +7551,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)</pre><pre> struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;</pre><pre> int crtc_disable_count = 0;</pre><pre> bool mode_set_reset_required = false;</pre><pre>+ struct amdgpu_crtc *acrtc;</pre><pre> </pre><pre> drm_atomic_helper_update_legacy_modeset_state(dev, state);</pre><pre> </pre><pre>@@ -7651,9 +7658,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)</pre><pre> const struct dc_stream_status *status =</pre><pre> dc_stream_get_status(dm_new_crtc_state->stream);</pre><pre> </pre><pre>- if (!status)</pre><pre>+ if (!status) {</pre><pre> status = dc_stream_get_status_from_state(dc_state,</pre><pre> dm_new_crtc_state->stream);</pre><pre>+ dc_stream_retain(dm_new_crtc_state->stream);</pre></blockquote><pre> </pre><pre>You're missing a release on this reference (dc_stream_release) so this </pre><pre>will cause a memory leak.</pre><pre> </pre><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>+ acrtc->dm_irq_params.stream = dm_new_crtc_state->stream;</pre><pre>+ }</pre><pre> </pre><pre> if (!status)</pre><pre> DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);</pre><pre>@@ -7780,8 +7790,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)</pre><pre> dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);</pre><pre> dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);</pre><pre> </pre><pre>- /* Update freesync active state. */</pre><pre>- pre_update_freesync_state_on_stream(dm, dm_new_crtc_state);</pre><pre>+ /* For freesync config update on crtc state and params for irq */</pre><pre>+ update_stream_irq_parameters(dm, dm_new_crtc_state);</pre><pre> </pre><pre> /* Handle vrr on->off / off->on transitions */</pre><pre> amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,</pre><pre>@@ -7797,10 +7807,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)</pre><pre> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {</pre><pre> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);</pre><pre> </pre><pre>+ dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);</pre><pre>+</pre><pre> if (new_crtc_state->active &&</pre><pre> (!old_crtc_state->active ||</pre><pre> drm_atomic_crtc_needs_modeset(new_crtc_state))) {</pre><pre>+ dc_stream_retain(dm_new_crtc_state->stream);</pre></blockquote><pre> </pre><pre>This retain is also missing a dc_stream_release.</pre><pre> </pre><pre>Regards,</pre><pre>Nicholas Kazlauskas</pre><pre> </pre><pre> </pre></blockquote><pre> </pre><pre>Hi Nick,</pre><pre> </pre><pre>Thanks for the review, I've posted V2.</pre></body></html>