AMD Freesync patches v2

AMD Freesync patches v2

[Mario Kleiner]

The current patch series, with feedback from Paul, Nicholas and Harry
applied and r-b / acked-by tags added. Thanks for the feedback.
Rebased to current drm-5.2-wip branch.

Patch 1/4 is still the same though. Don't know if i or Nicholas could
fix it in a followup patch, or this one needs more work directly, or
who does what. Nicholas?

thanks,
-mario


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

[PATCH 1/4] drm/amd/display: Prevent vblank irq disable while VRR is active. (v2)

[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. Suggested by Nicholas.

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 42531ed6ae75..b73313d6450f 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;
@@ -4712,6 +4718,31 @@ static void update_freesync_state_on_stream(
 			      (int)vrr_params.state);
 }
 
+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,
@@ -5246,6 +5277,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.17.1

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

[PATCH 2/4] 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 b73313d6450f..aabd23fa0766 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);
 
@@ -4764,9 +4772,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 {
@@ -4910,7 +4917,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:
@@ -4926,11 +4933,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.17.1

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

[PATCH 3/4] 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>
---
 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 aabd23fa0766..fe207988d0b2 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;
@@ -4737,7 +4854,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);
@@ -4745,6 +4866,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.17.1

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

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

[Kazlauskas, Nicholas]

On 3/22/19 4:04 PM, Mario Kleiner wrote:
> 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>

This patch seems reasonable to me for the most part. I did testing on 
the previous version and seems to do what it intends to.

Harry made good points about the potential problems associated with 
VUPDATE - it's dependent on the timing for the mode and could 
potentially even be before line 0, but I think that's unlikely to happen 
for any monitor that supports adaptive sync.

I'd like to look into moving all of this over to VLINE instead at some 
point for a guarantee on when these events get sent to userspace but for 
now this is probably good enough for resolving these timestamp issues.

There still needs to be that other patch preceding your first one that 
addresses the freesync config being updated after it's checked for 
keeping the vblank reference.

I should be able to find some time this week to write up a patch that 
fixes that problem and send it to amd-gfx if you haven't already taken a 
look at the issue.

Nicholas Kazlauskas

> ---
>   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 aabd23fa0766..fe207988d0b2 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;
> @@ -4737,7 +4854,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);
> @@ -4745,6 +4866,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),
> 

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

[PATCH 4/4] 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 fe207988d0b2..c20e9f695f11 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.17.1

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

[PATCH 1/4] drm/amd/display: Prevent vblank irq disable while VRR is active. (v2)

[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. Suggested by Nicholas.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/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 a718ac2..1c83e80 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;
@@ -4716,6 +4722,31 @@ static void update_freesync_state_on_stream(
 			      (int)vrr_params.state);
 }
 
+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,
@@ -5250,6 +5281,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.7.4

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

Re: [PATCH 1/4] drm/amd/display: Prevent vblank irq disable while VRR is active. (v2)

[Kazlauskas, Nicholas]

On 3/20/19 4:12 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. Suggested by Nicholas.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>   drivers/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 a718ac2..1c83e80 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;
> @@ -4716,6 +4722,31 @@ static void update_freesync_state_on_stream(
>   			      (int)vrr_params.state);
>   }
>   
> +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,
> @@ -5250,6 +5281,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);
> +

I guess there's actually another problem with this here - we won't have 
the actual dm_state->freesync_config.state until the commit following 
this one since it gets calculated below this transition handler.

We need this handler to trigger before the new framebuffer address is 
written but after these parameters are calculated.

So this needs a v3 or another patch in the series that shifts the 
calculation of config.state before this.

While it's probably sufficient to just reuse the block:

	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;
	}

the problem is config.state could still technically be modified within 
mod_freesync_build_vrr_params I think.

So it's probably best to shift everything up to and including 
mod_freesync_build_vrr_params(...) from within 
update_freesync_state_on_stream(...) earlier, either in atomic check or 
in commit tail in another loop.

While I prefer the first approach, it's a little bit complicated. The 
get_freesync_config_for_crtc looks like the right place to put this, but 
that function would have to be moved. The problem is that

if (!(enable && aconnector && new_crtc_state->enable &&
	      new_crtc_state->active))

Won't be true if we're disabling a CRTC at the same time we're disabling 
VRR. The reset_freesync_config_for_crtc function would cover this but it 
doesn't actually touch the freesync_config - but I think it's safe to do 
so, since the check above will be true if the stream still exists.

Nicholas Kazlauskas

>   		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

Re: [PATCH 1/4] drm/amd/display: Prevent vblank irq disable while VRR is active. (v2)

[Mario Kleiner]

On Wed, Mar 20, 2019 at 2:11 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 3/20/19 4:12 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. Suggested by Nicholas.
> >
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > ---
> >   drivers/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 a718ac2..1c83e80 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;
> > @@ -4716,6 +4722,31 @@ static void update_freesync_state_on_stream(
> >                             (int)vrr_params.state);
> >   }
> >
> > +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,
> > @@ -5250,6 +5281,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);
> > +
>
> I guess there's actually another problem with this here - we won't have
> the actual dm_state->freesync_config.state until the commit following
> this one since it gets calculated below this transition handler.
>
> We need this handler to trigger before the new framebuffer address is
> written but after these parameters are calculated.
>
> So this needs a v3 or another patch in the series that shifts the
> calculation of config.state before this.
>

It's surprising how well it works with the thing i threw at it, for
something that's still wrong ;). But maybe that's just dumb luck in
the specific way X currently uses it.

Could you maybe try to create a patch on top of this one, implementing
your proposal below?

thanks,
-mario

> While it's probably sufficient to just reuse the block:
>
>         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;
>         }
>
> the problem is config.state could still technically be modified within
> mod_freesync_build_vrr_params I think.
>
> So it's probably best to shift everything up to and including
> mod_freesync_build_vrr_params(...) from within
> update_freesync_state_on_stream(...) earlier, either in atomic check or
> in commit tail in another loop.
>
> While I prefer the first approach, it's a little bit complicated. The
> get_freesync_config_for_crtc looks like the right place to put this, but
> that function would have to be moved. The problem is that
>
> if (!(enable && aconnector && new_crtc_state->enable &&
>               new_crtc_state->active))
>
> Won't be true if we're disabling a CRTC at the same time we're disabling
> VRR. The reset_freesync_config_for_crtc function would cover this but it
> doesn't actually touch the freesync_config - but I think it's safe to do
> so, since the check above will be true if the stream still exists.
>
> Nicholas Kazlauskas
>
> >               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