AMD Freesync patches v3

AMD Freesync patches v3

[Mario Kleiner]

The hopefully final patch series, with feedback applied and
r-b / acked-by tags added. Rebased to current agd5f/drm-5.2-wip
branch.

Numbering has slightly changed. Patches 3-5 are identical to last
series. Patch 2/5 v3 (the former 1/4 v2) trivially rebased on top of
the new 1/5.

Patch 1/5 is new and splits the VRR state updating to a part
early in commit_tail, before the vrr transition handling from
Patch 2/5, and a late part in the plane commit code, which must
happen after transition handling/during plane commit.

This should address all of Nicholas remaining comments. Tested
and nicely working on DCN-1.0 / Raven as well.

Thanks,
-mario


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

[PATCH 1/5] drm/amd/display: Update VRR state earlier in atomic_commit_tail.

[Mario Kleiner]

We need the VRR active/inactive state info earlier in
the commit sequence, so VRR related setup functions like
amdgpu_dm_handle_vrr_transition() know the final VRR state
when they need to do their hw setup work.

Split update_freesync_state_on_stream() into an early part,
that can run at the beginning of commit tail before the
vrr transition handling, and a late part that must run after
vrr transition handling inside the commit planes code for
enabled crtc's.

Suggested by Nicholas Kazlauskas.
Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 61 ++++++++++++++-----
 1 file changed, 46 insertions(+), 15 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 7d1c782072ee..6528258f8975 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4645,7 +4645,6 @@ static void update_freesync_state_on_stream(
 {
 	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
 	struct dc_info_packet vrr_infopacket = {0};
-	struct mod_freesync_config config = new_crtc_state->freesync_config;
 
 	if (!new_stream)
 		return;
@@ -4658,20 +4657,6 @@ static void update_freesync_state_on_stream(
 	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
 		return;
 
-	if (new_crtc_state->vrr_supported &&
-	    config.min_refresh_in_uhz &&
-	    config.max_refresh_in_uhz) {
-		config.state = new_crtc_state->base.vrr_enabled ?
-			VRR_STATE_ACTIVE_VARIABLE :
-			VRR_STATE_INACTIVE;
-	} else {
-		config.state = VRR_STATE_UNSUPPORTED;
-	}
-
-	mod_freesync_build_vrr_params(dm->freesync_module,
-				      new_stream,
-				      &config, &vrr_params);
-
 	if (surface) {
 		mod_freesync_handle_preflip(
 			dm->freesync_module,
@@ -4712,6 +4697,46 @@ static void update_freesync_state_on_stream(
 			      (int)vrr_params.state);
 }
 
+static void pre_update_freesync_state_on_stream(
+	struct amdgpu_display_manager *dm,
+	struct dm_crtc_state *new_crtc_state)
+{
+	struct dc_stream_state *new_stream = new_crtc_state->stream;
+	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
+	struct mod_freesync_config config = new_crtc_state->freesync_config;
+
+	if (!new_stream)
+		return;
+
+	/*
+	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
+	 * For now it's sufficient to just guard against these conditions.
+	 */
+	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
+		return;
+
+	if (new_crtc_state->vrr_supported &&
+	    config.min_refresh_in_uhz &&
+	    config.max_refresh_in_uhz) {
+		config.state = new_crtc_state->base.vrr_enabled ?
+			VRR_STATE_ACTIVE_VARIABLE :
+			VRR_STATE_INACTIVE;
+	} else {
+		config.state = VRR_STATE_UNSUPPORTED;
+	}
+
+	mod_freesync_build_vrr_params(dm->freesync_module,
+				      new_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);
+
+	new_crtc_state->vrr_params = vrr_params;
+}
+
 static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 				    struct dc_state *dc_state,
 				    struct drm_device *dev,
@@ -5233,6 +5258,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	/* Update freesync state before amdgpu_dm_handle_vrr_transition(). */
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+		pre_update_freesync_state_on_stream(dm, dm_new_crtc_state);
+	}
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 			new_crtc_state, i) {
 		/*
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/amd/display: Prevent vblank irq disable while VRR is active. (v3)

[Mario Kleiner]

During VRR mode we can not allow vblank irq dis-/enable
transitions, as an enable after a disable can happen at
an arbitrary time during the video refresh cycle, e.g.,
with a high likelyhood inside vblank front-porch. An
enable during front-porch would cause vblank timestamp
updates/calculations which are completely bogus, given
the code can't know when the vblank will end as long
as we are in front-porch with no page flip completed.

Hold a permanent vblank reference on the crtc while
in active VRR mode to prevent a vblank disable, and
drop the reference again when switching back to fixed
refresh rate non-VRR mode.

v2: Make sure transition is also handled if vrr is
    disabled and stream gets disabled in the same
    atomic commit by moving the call to the transition
    function outside of plane commit.
    Suggested by Nicholas.

v3: Trivial rebase onto previous patch.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

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 6528258f8975..6c413bc012af 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -251,6 +251,12 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,
 	return NULL;
 }
 
+static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)
+{
+	return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||
+	       dm_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
+}
+
 static void dm_pflip_high_irq(void *interrupt_params)
 {
 	struct amdgpu_crtc *amdgpu_crtc;
@@ -4737,6 +4743,31 @@ static void pre_update_freesync_state_on_stream(
 	new_crtc_state->vrr_params = vrr_params;
 }
 
+static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
+					    struct dm_crtc_state *new_state)
+{
+	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
+	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
+
+	if (!old_vrr_active && new_vrr_active) {
+		/* Transition VRR inactive -> active:
+		 * While VRR is active, we must not disable vblank irq, as a
+		 * reenable after disable would compute bogus vblank/pflip
+		 * timestamps if it likely happened inside display front-porch.
+		 */
+		drm_crtc_vblank_get(new_state->base.crtc);
+		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.
+		 */
+		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);
+	}
+}
+
 static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 				    struct dc_state *dc_state,
 				    struct drm_device *dev,
@@ -5277,6 +5308,11 @@ 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);
+
+		/* Handle vrr on->off / off->on transitions */
+		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
+						dm_new_crtc_state);
+
 		modeset_needed = modeset_required(
 				new_crtc_state,
 				dm_new_crtc_state->stream,
-- 
2.20.1

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

[PATCH 3/5] drm/amd/display: Rework vrr flip throttling for late vblank irq.

[Mario Kleiner]

For throttling to work correctly, we always need a baseline vblank
count last_flip_vblank that increments at start of front-porch.

This is the case for drm_crtc_vblank_count() in non-VRR mode, where
the vblank irq fires at start of front-porch and triggers DRM core
vblank handling, but it is no longer the case in VRR mode, where
core vblank handling is done later, after end of front-porch.

Therefore drm_crtc_vblank_count() is no longer useful for this.
We also can't use drm_crtc_accurate_vblank_count(), as that would
screw up vblank timestamps in VRR mode when called in front-porch.

To solve this, use the cooked hardware vblank counter returned by
amdgpu_get_vblank_counter_kms() instead, as that one is cooked to
always increment at start of front-porch, independent of when
vblank related irq's fire.

This patch allows vblank irq handling to happen anywhere within
vblank of even after it, without a negative impact on flip
throttling, so followup patches can shift the vblank core
handling trigger point wherever they need it.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  2 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++--------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 889e443eeee7..add238fe4b57 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -406,7 +406,7 @@ struct amdgpu_crtc {
 	struct amdgpu_flip_work *pflip_works;
 	enum amdgpu_flip_status pflip_status;
 	int deferred_flip_completion;
-	u64 last_flip_vblank;
+	u32 last_flip_vblank;
 	/* pll sharing */
 	struct amdgpu_atom_ss ss;
 	bool ss_enabled;
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 6c413bc012af..7366528e5cc2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -286,7 +286,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	}
 
 	/* Update to correct count(s) if racing with vblank irq */
-	amdgpu_crtc->last_flip_vblank = drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
+	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
 
 	/* wake up userspace */
 	if (amdgpu_crtc->event) {
@@ -298,6 +298,14 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	} else
 		WARN_ON(1);
 
+	/* Keep track of vblank of this flip for flip throttling. We use the
+	 * cooked hw counter, as that one incremented at start of this vblank
+	 * of pageflip completion, so last_flip_vblank is the forbidden count
+	 * for queueing new pageflips if vsync + VRR is enabled.
+	 */
+	amdgpu_crtc->last_flip_vblank = amdgpu_get_vblank_counter_kms(adev->ddev,
+							amdgpu_crtc->crtc_id);
+
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
@@ -4789,9 +4797,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 	struct amdgpu_bo *abo;
 	uint64_t tiling_flags;
-	uint32_t target, target_vblank;
-	uint64_t last_flip_vblank;
-	bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE;
+	uint32_t target_vblank, last_flip_vblank;
+	bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
 	bool pflip_present = false;
 
 	struct {
@@ -4935,7 +4942,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			 * clients using the GLX_OML_sync_control extension or
 			 * DRI3/Present extension with defined target_msc.
 			 */
-			last_flip_vblank = drm_crtc_vblank_count(pcrtc);
+			last_flip_vblank = amdgpu_get_vblank_counter_kms(dm->ddev, acrtc_attach->crtc_id);
 		}
 		else {
 			/* For variable refresh rate mode only:
@@ -4951,11 +4958,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
 
-		target = (uint32_t)last_flip_vblank + wait_for_vblank;
-
-		/* Prepare wait for target vblank early - before the fence-waits */
-		target_vblank = target - (uint32_t)drm_crtc_vblank_count(pcrtc) +
-				amdgpu_get_vblank_counter_kms(pcrtc->dev, acrtc_attach->crtc_id);
+		target_vblank = last_flip_vblank + wait_for_vblank;
 
 		/*
 		 * Wait until we're out of the vertical blank period before the one
-- 
2.20.1

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

[PATCH 4/5] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank. (v2)

[Mario Kleiner]

In VRR mode, proper vblank/pageflip timestamps can only be computed
after the display scanout position has left front-porch. Therefore
delay calls to drm_crtc_handle_vblank(), and thereby calls to
drm_update_vblank_count() and pageflip event delivery, to after the
end of front-porch when in VRR mode.

We add a new vupdate irq, which triggers at the end of the vupdate
interval, ie. at the end of vblank, and calls the core vblank handler
function. The new irq handler is not executed in standard non-VRR
mode, so vblank handling for fixed refresh rate mode is identical
to the past implementation.

v2: Implement feedback by Nicholas and Paul Menzel.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Acked-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   1 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 128 +++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   9 ++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c |  22 +++
 .../dc/irq/dce110/irq_service_dce110.c        |   7 +-
 .../dc/irq/dce120/irq_service_dce120.c        |   7 +-
 .../display/dc/irq/dce80/irq_service_dce80.c  |   6 +-
 .../display/dc/irq/dcn10/irq_service_dcn10.c  |  40 ++++--
 8 files changed, 204 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 6e71749cb3bb..6294316f24c7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -827,6 +827,7 @@ struct amdgpu_device {
 	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
 	struct work_struct		hotplug_work;
 	struct amdgpu_irq_src		crtc_irq;
+	struct amdgpu_irq_src		vupdate_irq;
 	struct amdgpu_irq_src		pageflip_irq;
 	struct amdgpu_irq_src		hpd_irq;
 
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 7366528e5cc2..1a5be73e4172 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	drm_crtc_vblank_put(&amdgpu_crtc->base);
 }
 
+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;
+
+	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);
+
+		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				 amdgpu_dm_vrr_active(acrtc_state));
+
+		/* Core vblank handling is done here after end of front-porch in
+		 * vrr mode, as vblank timestamping will give valid results
+		 * while now done after front-porch. This will also deliver
+		 * page-flip completion events that have been queued to us
+		 * if a pageflip happened inside front-porch.
+		 */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			drm_crtc_handle_vblank(&acrtc->base);
+	}
+}
+
 static void dm_crtc_high_irq(void *interrupt_params)
 {
 	struct common_irq_params *irq_params = interrupt_params;
@@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
 	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
 
 	if (acrtc) {
-		drm_crtc_handle_vblank(&acrtc->base);
-		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
-
 		acrtc_state = to_dm_crtc_state(acrtc->base.state);
 
+		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				 amdgpu_dm_vrr_active(acrtc_state));
+
+		/* Core vblank handling at start of front-porch is only possible
+		 * in non-vrr mode, as only there vblank timestamping will give
+		 * 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))
+			drm_crtc_handle_vblank(&acrtc->base);
+
+		/* Following stuff must happen at start of vblank, for crc
+		 * computation and below-the-range btr support in vrr mode.
+		 */
+		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
+
 		if (acrtc_state->stream &&
 		    acrtc_state->vrr_params.supported &&
 		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
@@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
 				dm_crtc_high_irq, c_irq_params);
 	}
 
+	/* Use VUPDATE interrupt */
+	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i += 2) {
+		r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
+		if (r) {
+			DRM_ERROR("Failed to add vupdate irq id!\n");
+			return r;
+		}
+
+		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
+		int_params.irq_source =
+			dc_interrupt_to_irq_source(dc, i, 0);
+
+		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
+
+		c_irq_params->adev = adev;
+		c_irq_params->irq_src = int_params.irq_source;
+
+		amdgpu_dm_irq_register_interrupt(adev, &int_params,
+				dm_vupdate_high_irq, c_irq_params);
+	}
+
 	/* Use GRPH_PFLIP interrupt */
 	for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
 			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
@@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_crtc_high_irq, c_irq_params);
 	}
 
+	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
+	 * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
+	 * to trigger at end of each vblank, regardless of state of the lock,
+	 * matching DCE behaviour.
+	 */
+	for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
+	     i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
+	     i++) {
+		r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
+
+		if (r) {
+			DRM_ERROR("Failed to add vupdate irq id!\n");
+			return r;
+		}
+
+		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
+		int_params.irq_source =
+			dc_interrupt_to_irq_source(dc, i, 0);
+
+		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
+
+		c_irq_params->adev = adev;
+		c_irq_params->irq_src = int_params.irq_source;
+
+		amdgpu_dm_irq_register_interrupt(adev, &int_params,
+				dm_vupdate_high_irq, c_irq_params);
+	}
+
 	/* Use GRPH_PFLIP interrupt */
 	for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
 			i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
@@ -3223,12 +3311,41 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &state->base;
 }
 
+static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
+{
+	enum dc_irq_source irq_source;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct amdgpu_device *adev = crtc->dev->dev_private;
+	int rc;
+
+	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
+
+	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+
+	DRM_DEBUG_DRIVER("crtc %d - vupdate irq %sabling: r=%d\n",
+			 acrtc->crtc_id, enable ? "en" : "dis", rc);
+	return rc;
+}
 
 static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 {
 	enum dc_irq_source irq_source;
 	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 	struct amdgpu_device *adev = crtc->dev->dev_private;
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	int rc = 0;
+
+	if (enable) {
+		/* vblank irq on -> Only need vupdate irq in vrr mode */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			rc = dm_set_vupdate_irq(crtc, true);
+	} else {
+		/* vblank irq off -> vupdate irq off */
+		rc = dm_set_vupdate_irq(crtc, false);
+	}
+
+	if (rc)
+		return rc;
 
 	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
 	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
@@ -4762,7 +4879,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * While VRR is active, we must not disable vblank irq, as a
 		 * reenable after disable would compute bogus vblank/pflip
 		 * timestamps if it likely happened inside display front-porch.
+		 *
+		 * 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);
 		DRM_DEBUG_DRIVER("%s: crtc=%u VRR off->on: Get vblank ref\n",
 				 __func__, new_state->base.crtc->base.id);
@@ -4770,6 +4891,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		/* Transition VRR active -> inactive:
 		 * Allow vblank irq disable again for fixed refresh rate.
 		 */
+		dm_set_vupdate_irq(new_state->base.crtc, false);
 		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);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 773ef5ca8441..3a0b6164c755 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -182,6 +182,15 @@ struct amdgpu_display_manager {
 	struct common_irq_params
 	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
 
+	/**
+	 * @vupdate_params:
+	 *
+	 * Vertical update IRQ parameters, passed to registered handlers when
+	 * triggered.
+	 */
+	struct common_irq_params
+	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
+
 	spinlock_t irq_handler_list_table_lock;
 
 	struct backlight_device *backlight_dev;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index cd10f77cdeb0..fd22b4474dbf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
 		__func__);
 }
 
+static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
+					   struct amdgpu_irq_src *source,
+					   unsigned int crtc_id,
+					   enum amdgpu_interrupt_state state)
+{
+	return dm_irq_state(
+		adev,
+		source,
+		crtc_id,
+		state,
+		IRQ_TYPE_VUPDATE,
+		__func__);
+}
+
 static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
 	.set = amdgpu_dm_set_crtc_irq_state,
 	.process = amdgpu_dm_irq_handler,
 };
 
+static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
+	.set = amdgpu_dm_set_vupdate_irq_state,
+	.process = amdgpu_dm_irq_handler,
+};
+
 static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
 	.set = amdgpu_dm_set_pflip_irq_state,
 	.process = amdgpu_dm_irq_handler,
@@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
 	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
 	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
 
+	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
+	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
+
 	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
 	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
 
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
index afe0876fe6f8..86987f5e8bd5 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
@@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
 		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
@@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
index 1ea7256ec89b..750ba0ab4106 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
@@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		IRQ_REG_ENTRY(CRTC, reg_num,\
 			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
 			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
index 8a2066c313fe..de218fe84a43 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
@@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
 
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
@@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
index e04ae49243f6..10ac6deff5ff 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
@@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
 		return DC_IRQ_SOURCE_VBLANK5;
 	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
 		return DC_IRQ_SOURCE_VBLANK6;
+	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE1;
+	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE2;
+	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE3;
+	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE4;
+	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE5;
+	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE6;
 	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
 		return DC_IRQ_SOURCE_PFLIP1;
 	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
@@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		.funcs = &pflip_irq_info_funcs\
 	}
 
-#define vupdate_int_entry(reg_num)\
+/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
+ * of DCE's DC_IRQ_SOURCE_VUPDATEx.
+ */
+#define vupdate_no_lock_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
 		IRQ_REG_ENTRY(OTG, reg_num,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
+		.funcs = &vupdate_no_lock_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
@@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
 	dc_underflow_int_entry(6),
 	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
 	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
-	vupdate_int_entry(0),
-	vupdate_int_entry(1),
-	vupdate_int_entry(2),
-	vupdate_int_entry(3),
-	vupdate_int_entry(4),
-	vupdate_int_entry(5),
+	vupdate_no_lock_int_entry(0),
+	vupdate_no_lock_int_entry(1),
+	vupdate_no_lock_int_entry(2),
+	vupdate_no_lock_int_entry(3),
+	vupdate_no_lock_int_entry(4),
+	vupdate_no_lock_int_entry(5),
 	vblank_int_entry(0),
 	vblank_int_entry(1),
 	vblank_int_entry(2),
-- 
2.20.1

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

[PATCH 5/5] drm/amd/display: Make pageflip event delivery compatible with VRR.

[Mario Kleiner]

We want vblank counts and timestamps of flip completion as sent
in pageflip completion events to be consistent with the vblank
count and timestamp of the vblank of flip completion, like in non
VRR mode.

In VRR mode, drm_update_vblank_count() - and thereby vblank
count and timestamp updates - must be delayed until after the
end of front-porch of each vblank, as it is only safe to
calculate vblank timestamps outside of the front-porch, when
we actually know when the vblank will end or has ended.

The function drm_update_vblank_count() which updates timestamps
and counts gets called by drm_crtc_accurate_vblank_count() or by
drm_crtc_handle_vblank().

Therefore we must make sure that pageflip events for a completed
flip are only sent out after drm_crtc_accurate_vblank_count() or
drm_crtc_handle_vblank() is executed, after end of front-porch
for the vblank of flip completion.

Two cases:

a) Pageflip irq handler executes inside front-porch:
   In this case we must defer sending pageflip events until
   drm_crtc_handle_vblank() executes after end of front-porch,
   and thereby calculates proper vblank count and timestamp.
   Iow. the pflip irq handler must just arm a pageflip event
   to be sent out by drm_crtc_handle_vblank() later on.

b) Pageflip irq handler executes after end of front-porch, e.g.,
   after flip completion in back-porch or due to a massively
   delayed handler invocation into the active scanout of the new
   frame. In this case we can call drm_crtc_accurate_vblank_count()
   to safely force calculation of a proper vblank count and
   timestamp, and must send the pageflip completion event
   ourselves from the pageflip irq handler.

   This is the same behaviour as needed for standard fixed refresh
   rate mode.

To decide from within pageflip handler if we are in case a) or b),
we check the current scanout position against the boundary of
front-porch. In non-VRR mode we just do what we did in the past.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68 +++++++++++++++----
 1 file changed, 55 insertions(+), 13 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 1a5be73e4172..e9ef42e86a73 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -263,6 +263,10 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	struct common_irq_params *irq_params = 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;
 
 	amdgpu_crtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_PFLIP);
 
@@ -285,18 +289,57 @@ static void dm_pflip_high_irq(void *interrupt_params)
 		return;
 	}
 
-	/* Update to correct count(s) if racing with vblank irq */
-	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
+	/* page flip completed. */
+	e = amdgpu_crtc->event;
+	amdgpu_crtc->event = NULL;
 
-	/* wake up userspace */
-	if (amdgpu_crtc->event) {
-		drm_crtc_send_vblank_event(&amdgpu_crtc->base, amdgpu_crtc->event);
+	if (!e)
+		WARN_ON(1);
 
-		/* page flip completed. clean up */
-		amdgpu_crtc->event = NULL;
+	acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);
+	vrr_active = amdgpu_dm_vrr_active(acrtc_state);
+
+	/* Fixed refresh rate, or VRR scanout position outside front-porch? */
+	if (!vrr_active ||
+	    !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,
+				      &v_blank_end, &hpos, &vpos) ||
+	    (vpos < v_blank_start)) {
+		/* Update to correct count and vblank timestamp if racing with
+		 * vblank irq. This also updates to the correct vblank timestamp
+		 * even in VRR mode, as scanout is past the front-porch atm.
+		 */
+		drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
 
-	} else
-		WARN_ON(1);
+		/* Wake up userspace by sending the pageflip event with proper
+		 * count and timestamp of vblank of flip completion.
+		 */
+		if (e) {
+			drm_crtc_send_vblank_event(&amdgpu_crtc->base, e);
+
+			/* Event sent, so done with vblank for this flip */
+			drm_crtc_vblank_put(&amdgpu_crtc->base);
+		}
+	} else if (e) {
+		/* VRR active and inside front-porch: vblank count and
+		 * timestamp for pageflip event will only be up to date after
+		 * drm_crtc_handle_vblank() has been executed from late vblank
+		 * irq handler after start of back-porch (vline 0). We queue the
+		 * pageflip event for send-out by drm_crtc_handle_vblank() with
+		 * updated timestamp and count, once it runs after us.
+		 *
+		 * We need to open-code this instead of using the helper
+		 * drm_crtc_arm_vblank_event(), as that helper would
+		 * call drm_crtc_accurate_vblank_count(), which we must
+		 * not call in VRR mode while we are in front-porch!
+		 */
+
+		/* sequence will be replaced by real count during send-out. */
+		e->sequence = drm_crtc_vblank_count(&amdgpu_crtc->base);
+		e->pipe = amdgpu_crtc->crtc_id;
+
+		list_add_tail(&e->base.link, &adev->ddev->vblank_event_list);
+		e = NULL;
+	}
 
 	/* Keep track of vblank of this flip for flip throttling. We use the
 	 * cooked hw counter, as that one incremented at start of this vblank
@@ -309,10 +352,9 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
-	DRM_DEBUG_DRIVER("%s - crtc :%d[%p], pflip_stat:AMDGPU_FLIP_NONE\n",
-					__func__, amdgpu_crtc->crtc_id, amdgpu_crtc);
-
-	drm_crtc_vblank_put(&amdgpu_crtc->base);
+	DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_NONE, vrr[%d]-fp %d\n",
+			 amdgpu_crtc->crtc_id, amdgpu_crtc,
+			 vrr_active, (int) !e);
 }
 
 static void dm_vupdate_high_irq(void *interrupt_params)
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/amd/display: Update VRR state earlier in atomic_commit_tail.

[Kazlauskas, Nicholas]

On 3/29/19 8:00 AM, Mario Kleiner wrote:
> We need the VRR active/inactive state info earlier in
> the commit sequence, so VRR related setup functions like
> amdgpu_dm_handle_vrr_transition() know the final VRR state
> when they need to do their hw setup work.
> 
> Split update_freesync_state_on_stream() into an early part,
> that can run at the beginning of commit tail before the
> vrr transition handling, and a late part that must run after
> vrr transition handling inside the commit planes code for
> enabled crtc's.
> 
> Suggested by Nicholas Kazlauskas.
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

This works fine as far as I can tell even though my preference was still 
on doing it on atomic check. I can still refactor or take a look at that 
later if I find some time at least.

Nicholas Kazlauskas

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 61 ++++++++++++++-----
>   1 file changed, 46 insertions(+), 15 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 7d1c782072ee..6528258f8975 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4645,7 +4645,6 @@ static void update_freesync_state_on_stream(
>   {
>   	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
>   	struct dc_info_packet vrr_infopacket = {0};
> -	struct mod_freesync_config config = new_crtc_state->freesync_config;
>   
>   	if (!new_stream)
>   		return;
> @@ -4658,20 +4657,6 @@ static void update_freesync_state_on_stream(
>   	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
>   		return;
>   
> -	if (new_crtc_state->vrr_supported &&
> -	    config.min_refresh_in_uhz &&
> -	    config.max_refresh_in_uhz) {
> -		config.state = new_crtc_state->base.vrr_enabled ?
> -			VRR_STATE_ACTIVE_VARIABLE :
> -			VRR_STATE_INACTIVE;
> -	} else {
> -		config.state = VRR_STATE_UNSUPPORTED;
> -	}
> -
> -	mod_freesync_build_vrr_params(dm->freesync_module,
> -				      new_stream,
> -				      &config, &vrr_params);
> -
>   	if (surface) {
>   		mod_freesync_handle_preflip(
>   			dm->freesync_module,
> @@ -4712,6 +4697,46 @@ static void update_freesync_state_on_stream(
>   			      (int)vrr_params.state);
>   }
>   
> +static void pre_update_freesync_state_on_stream(
> +	struct amdgpu_display_manager *dm,
> +	struct dm_crtc_state *new_crtc_state)
> +{
> +	struct dc_stream_state *new_stream = new_crtc_state->stream;
> +	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> +	struct mod_freesync_config config = new_crtc_state->freesync_config;
> +
> +	if (!new_stream)
> +		return;
> +
> +	/*
> +	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
> +	 * For now it's sufficient to just guard against these conditions.
> +	 */
> +	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> +		return;
> +
> +	if (new_crtc_state->vrr_supported &&
> +	    config.min_refresh_in_uhz &&
> +	    config.max_refresh_in_uhz) {
> +		config.state = new_crtc_state->base.vrr_enabled ?
> +			VRR_STATE_ACTIVE_VARIABLE :
> +			VRR_STATE_INACTIVE;
> +	} else {
> +		config.state = VRR_STATE_UNSUPPORTED;
> +	}
> +
> +	mod_freesync_build_vrr_params(dm->freesync_module,
> +				      new_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);
> +
> +	new_crtc_state->vrr_params = vrr_params;
> +}
> +
>   static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   				    struct dc_state *dc_state,
>   				    struct drm_device *dev,
> @@ -5233,6 +5258,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   		mutex_unlock(&dm->dc_lock);
>   	}
>   
> +	/* Update freesync state before amdgpu_dm_handle_vrr_transition(). */
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> +		pre_update_freesync_state_on_stream(dm, dm_new_crtc_state);
> +	}
> +
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>   			new_crtc_state, i) {
>   		/*
> 

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

Re: [PATCH 2/5] drm/amd/display: Prevent vblank irq disable while VRR is active. (v3)

[Kazlauskas, Nicholas]

On 3/29/19 8:00 AM, Mario Kleiner wrote:
> During VRR mode we can not allow vblank irq dis-/enable
> transitions, as an enable after a disable can happen at
> an arbitrary time during the video refresh cycle, e.g.,
> with a high likelyhood inside vblank front-porch. An
> enable during front-porch would cause vblank timestamp
> updates/calculations which are completely bogus, given
> the code can't know when the vblank will end as long
> as we are in front-porch with no page flip completed.
> 
> Hold a permanent vblank reference on the crtc while
> in active VRR mode to prevent a vblank disable, and
> drop the reference again when switching back to fixed
> refresh rate non-VRR mode.
> 
> v2: Make sure transition is also handled if vrr is
>      disabled and stream gets disabled in the same
>      atomic commit by moving the call to the transition
>      function outside of plane commit.
>      Suggested by Nicholas.
> 
> v3: Trivial rebase onto previous patch.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

I'll go ahead and push this series to amd-staging-drm-next now that 
these are all reviewed (and I've tested them on both dce and dcn hardware).

Thanks for helping to improve this!

Nicholas Kazlauskas

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 +++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> 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 6528258f8975..6c413bc012af 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -251,6 +251,12 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,
>   	return NULL;
>   }
>   
> +static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)
> +{
> +	return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||
> +	       dm_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
> +}
> +
>   static void dm_pflip_high_irq(void *interrupt_params)
>   {
>   	struct amdgpu_crtc *amdgpu_crtc;
> @@ -4737,6 +4743,31 @@ static void pre_update_freesync_state_on_stream(
>   	new_crtc_state->vrr_params = vrr_params;
>   }
>   
> +static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> +					    struct dm_crtc_state *new_state)
> +{
> +	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
> +	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
> +
> +	if (!old_vrr_active && new_vrr_active) {
> +		/* Transition VRR inactive -> active:
> +		 * While VRR is active, we must not disable vblank irq, as a
> +		 * reenable after disable would compute bogus vblank/pflip
> +		 * timestamps if it likely happened inside display front-porch.
> +		 */
> +		drm_crtc_vblank_get(new_state->base.crtc);
> +		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.
> +		 */
> +		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);
> +	}
> +}
> +
>   static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   				    struct dc_state *dc_state,
>   				    struct drm_device *dev,
> @@ -5277,6 +5308,11 @@ 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);
> +
> +		/* Handle vrr on->off / off->on transitions */
> +		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
> +						dm_new_crtc_state);
> +
>   		modeset_needed = modeset_required(
>   				new_crtc_state,
>   				dm_new_crtc_state->stream,
> 

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