Re: [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler

[Aurabindo Pillai <aurabindo.pillai@xxxxxxx>]

On Wed, 2020-09-09 at 11:00 -0400, Kazlauskas, Nicholas wrote:

On 2020-09-09 10:28 a.m., Aurabindo Pillai wrote:

[Why&How]

Currently commit_tail holds global locks and wait for dependencies which is

against the DRM API contracts. Inorder to fix this, IRQ handler should be able

to run without having to access crtc state. Required parameters are copied over

so that they can be directly accessed from the interrupt handler

Signed-off-by: Aurabindo Pillai <

aurabindo.pillai@xxxxxxx

>

---

  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 115 ++++++++++--------

  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 -

  .../display/amdgpu_dm/amdgpu_dm_irq_params.h  |   4 +

  3 files changed, 68 insertions(+), 52 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 40814cdd8c92..0603436a3313 100644

--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

@@ -192,17 +192,14 @@ static u32 dm_vblank_get_counter(struct amdgpu_device *adev, int crtc)

  		return 0;

  	else {

  		struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];

-		struct dm_crtc_state *acrtc_state = to_dm_crtc_state(

-				acrtc->base.state);

-

-		if (acrtc_state->stream == NULL) {

+		if (acrtc->dm_irq_params.stream == NULL) {

  			DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",

  				  crtc);

  			return 0;

  		}

-		return dc_stream_get_vblank_counter(acrtc_state->stream);

+		return dc_stream_get_vblank_counter(acrtc->dm_irq_params.stream);

  	}

  }

@@ -215,10 +212,8 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,

  		return -EINVAL;

  	else {

  		struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];

-		struct dm_crtc_state *acrtc_state = to_dm_crtc_state(

-						acrtc->base.state);

-		if (acrtc_state->stream ==  NULL) {

+		if (acrtc->dm_irq_params.stream ==  NULL) {

  			DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",

  				  crtc);

  			return 0;

@@ -228,7 +223,7 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,

  		 * TODO rework base driver to use values directly.

  		 * for now parse it back into reg-format

  		 */

-		dc_stream_get_scanoutpos(acrtc_state->stream,

+		dc_stream_get_scanoutpos(acrtc->dm_irq_params.stream,

  					 &v_blank_start,

  					 &v_blank_end,

  					 &h_position,

@@ -287,6 +282,14 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,

  	return NULL;

  }

+static inline bool amdgpu_dm_vrr_active_irq(struct amdgpu_crtc *acrtc)

+{

+	return acrtc->dm_irq_params.freesync_config.state ==

+		       VRR_STATE_ACTIVE_VARIABLE ||

+	       acrtc->dm_irq_params.freesync_config.state ==

+		       VRR_STATE_ACTIVE_FIXED;

+}

+

  static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)

  {

  	return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||

@@ -307,7 +310,6 @@ static void dm_pflip_high_irq(void *interrupt_params)

  	struct amdgpu_device *adev = irq_params->adev;

  	unsigned long flags;

  	struct drm_pending_vblank_event *e;

-	struct dm_crtc_state *acrtc_state;

  	uint32_t vpos, hpos, v_blank_start, v_blank_end;

  	bool vrr_active;

@@ -339,12 +341,11 @@ static void dm_pflip_high_irq(void *interrupt_params)

  	if (!e)

  		WARN_ON(1);

-	acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);

-	vrr_active = amdgpu_dm_vrr_active(acrtc_state);

+	vrr_active = amdgpu_dm_vrr_active_irq(amdgpu_crtc);

  	/* Fixed refresh rate, or VRR scanout position outside front-porch? */

  	if (!vrr_active ||

-	    !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,

+	    !dc_stream_get_scanoutpos(amdgpu_crtc->dm_irq_params.stream, &v_blank_start,

  				      &v_blank_end, &hpos, &vpos) ||

  	    (vpos < v_blank_start)) {

  		/* Update to correct count and vblank timestamp if racing with

@@ -405,17 +406,17 @@ static void dm_vupdate_high_irq(void *interrupt_params)

  	struct common_irq_params *irq_params = interrupt_params;

  	struct amdgpu_device *adev = irq_params->adev;

  	struct amdgpu_crtc *acrtc;

-	struct dm_crtc_state *acrtc_state;

  	unsigned long flags;

+	int vrr_active;

  	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);

  	if (acrtc) {

-		acrtc_state = to_dm_crtc_state(acrtc->base.state);

+		vrr_active = amdgpu_dm_vrr_active_irq(acrtc);

  		DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d\n",

  			      acrtc->crtc_id,

-			      amdgpu_dm_vrr_active(acrtc_state));

+			      vrr_active);

  		/* Core vblank handling is done here after end of front-porch in

  		 * vrr mode, as vblank timestamping will give valid results

@@ -423,22 +424,22 @@ static void dm_vupdate_high_irq(void *interrupt_params)

  		 * page-flip completion events that have been queued to us

  		 * if a pageflip happened inside front-porch.

  		 */

-		if (amdgpu_dm_vrr_active(acrtc_state)) {

+		if (vrr_active) {

  			drm_crtc_handle_vblank(&acrtc->base);

  			/* BTR processing for pre-DCE12 ASICs */

-			if (acrtc_state->stream &&

+			if (acrtc->dm_irq_params.stream &&

  			    adev->family < AMDGPU_FAMILY_AI) {

  				spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);

  				mod_freesync_handle_v_update(

  				    adev->dm.freesync_module,

-				    acrtc_state->stream,

-				    &acrtc_state->vrr_params);

+				    acrtc->dm_irq_params.stream,

+				    &acrtc->dm_irq_params.vrr_params);

  				dc_stream_adjust_vmin_vmax(

  				    adev->dm.dc,

-				    acrtc_state->stream,

-				    &acrtc_state->vrr_params.adjust);

+				    acrtc->dm_irq_params.stream,

+				    &acrtc->dm_irq_params.vrr_params.adjust);

  				spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);

  			}

  		}

@@ -457,18 +458,17 @@ static void dm_crtc_high_irq(void *interrupt_params)

  	struct common_irq_params *irq_params = interrupt_params;

  	struct amdgpu_device *adev = irq_params->adev;

  	struct amdgpu_crtc *acrtc;

-	struct dm_crtc_state *acrtc_state;

  	unsigned long flags;

+	int vrr_active;

  	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);

  	if (!acrtc)

  		return;

-	acrtc_state = to_dm_crtc_state(acrtc->base.state);

+	vrr_active = amdgpu_dm_vrr_active_irq(acrtc);

  	DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d, planes:%d\n", acrtc->crtc_id,

-			 amdgpu_dm_vrr_active(acrtc_state),

-			 acrtc_state->active_planes);

+		      vrr_active, acrtc->dm_irq_params.active_planes);

  	/**

  	 * Core vblank handling at start of front-porch is only possible

@@ -476,7 +476,7 @@ static void dm_crtc_high_irq(void *interrupt_params)

  	 * valid results while done in front-porch. Otherwise defer it

  	 * to dm_vupdate_high_irq after end of front-porch.

  	 */

-	if (!amdgpu_dm_vrr_active(acrtc_state))

+	if (!vrr_active)

  		drm_crtc_handle_vblank(&acrtc->base);

  	/**

@@ -491,14 +491,16 @@ static void dm_crtc_high_irq(void *interrupt_params)

  	spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);

-	if (acrtc_state->stream && acrtc_state->vrr_params.supported &&

-	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {

+	if (acrtc->dm_irq_params.stream &&

+	    acrtc->dm_irq_params.vrr_params.supported &&

+	    acrtc->dm_irq_params.freesync_config.state ==

+		    VRR_STATE_ACTIVE_VARIABLE) {

  		mod_freesync_handle_v_update(adev->dm.freesync_module,

-					     acrtc_state->stream,

-					     &acrtc_state->vrr_params);

+					     acrtc->dm_irq_params.stream,

+					     &acrtc->dm_irq_params.vrr_params);

-		dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc_state->stream,

-					   &acrtc_state->vrr_params.adjust);

+		dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc->dm_irq_params.stream,

+					   &acrtc->dm_irq_params.vrr_params.adjust);

  	}

  	/*

@@ -513,7 +515,7 @@ static void dm_crtc_high_irq(void *interrupt_params)

  	 */

  	if (adev->family >= AMDGPU_FAMILY_RV &&

  	    acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&

-	    acrtc_state->active_planes == 0) {

+	    acrtc->dm_irq_params.active_planes == 0) {

  		if (acrtc->event) {

  			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);

  			acrtc->event = NULL;

@@ -4845,7 +4847,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)

  	}

  	state->active_planes = cur->active_planes;

-	state->vrr_params = cur->vrr_params;

  	state->vrr_infopacket = cur->vrr_infopacket;

  	state->abm_level = cur->abm_level;

  	state->vrr_supported = cur->vrr_supported;

@@ -6913,6 +6914,7 @@ static void update_freesync_state_on_stream(

  	struct mod_vrr_params vrr_params;

  	struct dc_info_packet vrr_infopacket = {0};

  	struct amdgpu_device *adev = dm->adev;

+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);

  	unsigned long flags;

  	if (!new_stream)

@@ -6927,7 +6929,7 @@ static void update_freesync_state_on_stream(

  		return;

  	spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);

-	vrr_params = new_crtc_state->vrr_params;

+        vrr_params = acrtc->dm_irq_params.vrr_params;

  	if (surface) {

  		mod_freesync_handle_preflip(

@@ -6958,7 +6960,7 @@ static void update_freesync_state_on_stream(

  		&vrr_infopacket);

  	new_crtc_state->freesync_timing_changed |=

-		(memcmp(&new_crtc_state->vrr_params.adjust,

+		(memcmp(&acrtc->dm_irq_params.vrr_params.adjust,

  			&vrr_params.adjust,

  			sizeof(vrr_params.adjust)) != 0);

@@ -6967,10 +6969,10 @@ static void update_freesync_state_on_stream(

  			&vrr_infopacket,

  			sizeof(vrr_infopacket)) != 0);

-	new_crtc_state->vrr_params = vrr_params;

+	acrtc->dm_irq_params.vrr_params = vrr_params;

  	new_crtc_state->vrr_infopacket = vrr_infopacket;

-	new_stream->adjust = new_crtc_state->vrr_params.adjust;

+	new_stream->adjust = acrtc->dm_irq_params.vrr_params.adjust;

  	new_stream->vrr_infopacket = vrr_infopacket;

  	if (new_crtc_state->freesync_vrr_info_changed)

@@ -6982,7 +6984,7 @@ static void update_freesync_state_on_stream(

  	spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);

  }

-static void pre_update_freesync_state_on_stream(

+static void update_stream_irq_parameters(

  	struct amdgpu_display_manager *dm,

  	struct dm_crtc_state *new_crtc_state)

  {

@@ -6990,6 +6992,7 @@ static void pre_update_freesync_state_on_stream(

  	struct mod_vrr_params vrr_params;

  	struct mod_freesync_config config = new_crtc_state->freesync_config;

  	struct amdgpu_device *adev = dm->adev;

+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);

  	unsigned long flags;

  	if (!new_stream)

@@ -7003,7 +7006,7 @@ static void pre_update_freesync_state_on_stream(

  		return;

  	spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);

-	vrr_params = new_crtc_state->vrr_params;

+        vrr_params = acrtc->dm_irq_params.vrr_params;

  	if (new_crtc_state->vrr_supported &&

  	    config.min_refresh_in_uhz &&

@@ -7020,11 +7023,14 @@ static void pre_update_freesync_state_on_stream(

  				      &config, &vrr_params);

  	new_crtc_state->freesync_timing_changed |=

-		(memcmp(&new_crtc_state->vrr_params.adjust,

-			&vrr_params.adjust,

-			sizeof(vrr_params.adjust)) != 0);

+		(memcmp(&acrtc->dm_irq_params.vrr_params.adjust,

+			&vrr_params.adjust, sizeof(vrr_params.adjust)) != 0);

-	new_crtc_state->vrr_params = vrr_params;

+	new_crtc_state->freesync_config = config;

+	/* Copy state for access from DM IRQ handler */

+	acrtc->dm_irq_params.freesync_config = config;

+	acrtc->dm_irq_params.active_planes = new_crtc_state->active_planes;

+	acrtc->dm_irq_params.vrr_params = vrr_params;

  	spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);

  }

@@ -7332,7 +7338,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,

  			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);

  			dc_stream_adjust_vmin_vmax(

  				dm->dc, acrtc_state->stream,

-				&acrtc_state->vrr_params.adjust);

+				&acrtc_attach->dm_irq_params.vrr_params.adjust);

  			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);

  		}

  		mutex_lock(&dm->dc_lock);

@@ -7545,6 +7551,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)

  	struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;

  	int crtc_disable_count = 0;

  	bool mode_set_reset_required = false;

+        struct amdgpu_crtc *acrtc;

  	drm_atomic_helper_update_legacy_modeset_state(dev, state);

@@ -7651,9 +7658,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)

  			const struct dc_stream_status *status =

  					dc_stream_get_status(dm_new_crtc_state->stream);

-			if (!status)

+			if (!status) {

  				status = dc_stream_get_status_from_state(dc_state,

  									 dm_new_crtc_state->stream);

+				dc_stream_retain(dm_new_crtc_state->stream);

You're missing a release on this reference (dc_stream_release) so this 

will cause a memory leak.

+				acrtc->dm_irq_params.stream = dm_new_crtc_state->stream;

+			}

  			if (!status)

  				DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);

@@ -7780,8 +7790,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)

  		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);

  		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);

-		/* Update freesync active state. */

-		pre_update_freesync_state_on_stream(dm, dm_new_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,

@@ -7797,10 +7807,15 @@ 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);

+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);

+

  		if (new_crtc_state->active &&

  		    (!old_crtc_state->active ||

  		     drm_atomic_crtc_needs_modeset(new_crtc_state))) {

+			dc_stream_retain(dm_new_crtc_state->stream);

This retain is also missing a dc_stream_release.

Regards,

Nicholas Kazlauskas

Hi Nick,

Thanks for the review, I've posted V2.

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx