All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 15:34 ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li-5C7GfCeVMHo @ 2019-11-05 15:34 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Leo Li, mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w,
	harry.wentland-5C7GfCeVMHo, nicholas.kazlauskas-5C7GfCeVMHo

From: Leo Li <sunpeng.li@amd.com>

[Why]

For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
interrupt. This is different from DCE, which has it assigned to vblank
start.

We'd like to send vblank and user events at vstartup because:

* It happens close enough to vupdate - the point of no return for HW.

* It is programmed as lines relative to vblank end - i.e. it is not in
  the variable portion when VRR is enabled. We should signal user
  events here.

* The pflip interrupt responsible for sending user events today only
  fires if the DCH HUBP component is not clock gated. In situations
  where planes are disabled - but the CRTC is enabled - user events won't
  be sent out, leading to flip done timeouts.

Consequently, this makes vupdate on DCN hardware redundant. It will be
removed in the next change.

[How]

Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
signal. Inside the DCN handler, we send off user events if the pflip
handler hasn't already done so.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

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 00017b91c91a..256a23a0ec28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
 	}
 }

+
+/**
+ * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
+ * @interrupt params - interrupt parameters
+ *
+ * Notify DRM's vblank event handler at VSTARTUP
+ *
+ * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
+ * * We are close enough to VUPDATE - the point of no return for hw
+ * * We are in the fixed portion of variable front porch when vrr is enabled
+ * * We are before VUPDATE, where double-buffered vrr registers are swapped
+ *
+ * It is therefore the correct place to signal vblank, send user flip events,
+ * and update VRR.
+ */
+static void dm_dcn_crtc_high_irq(void *interrupt_params)
+{
+	struct common_irq_params *irq_params = interrupt_params;
+	struct amdgpu_device *adev = irq_params->adev;
+	struct amdgpu_crtc *acrtc;
+	struct dm_crtc_state *acrtc_state;
+	unsigned long flags;
+
+	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
+
+	if (!acrtc)
+		return;
+
+	acrtc_state = to_dm_crtc_state(acrtc->base.state);
+
+	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				amdgpu_dm_vrr_active(acrtc_state));
+
+	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
+	drm_crtc_handle_vblank(&acrtc->base);
+
+	spin_lock_irqsave(&adev->ddev->event_lock, flags);
+
+	if (acrtc_state->vrr_params.supported &&
+	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
+		mod_freesync_handle_v_update(
+		adev->dm.freesync_module,
+		acrtc_state->stream,
+		&acrtc_state->vrr_params);
+
+		dc_stream_adjust_vmin_vmax(
+			adev->dm.dc,
+			acrtc_state->stream,
+			&acrtc_state->vrr_params.adjust);
+	}
+
+	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+		if (acrtc->event) {
+			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
+			acrtc->event = NULL;
+			drm_crtc_vblank_put(&acrtc->base);
+		}
+		acrtc->pflip_status = AMDGPU_FLIP_NONE;
+	}
+
+	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
+}
+
 static int dm_set_clockgating_state(void *handle,
 		  enum amd_clockgating_state state)
 {
@@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 		c_irq_params->irq_src = int_params.irq_source;

 		amdgpu_dm_irq_register_interrupt(adev, &int_params,
-				dm_crtc_high_irq, c_irq_params);
+				dm_dcn_crtc_high_irq, c_irq_params);
 	}

 	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 15:34 ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li @ 2019-11-05 15:34 UTC (permalink / raw)
  To: amd-gfx; +Cc: Leo Li, mario.kleiner.de, harry.wentland, nicholas.kazlauskas

From: Leo Li <sunpeng.li@amd.com>

[Why]

For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
interrupt. This is different from DCE, which has it assigned to vblank
start.

We'd like to send vblank and user events at vstartup because:

* It happens close enough to vupdate - the point of no return for HW.

* It is programmed as lines relative to vblank end - i.e. it is not in
  the variable portion when VRR is enabled. We should signal user
  events here.

* The pflip interrupt responsible for sending user events today only
  fires if the DCH HUBP component is not clock gated. In situations
  where planes are disabled - but the CRTC is enabled - user events won't
  be sent out, leading to flip done timeouts.

Consequently, this makes vupdate on DCN hardware redundant. It will be
removed in the next change.

[How]

Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
signal. Inside the DCN handler, we send off user events if the pflip
handler hasn't already done so.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

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 00017b91c91a..256a23a0ec28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
 	}
 }

+
+/**
+ * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
+ * @interrupt params - interrupt parameters
+ *
+ * Notify DRM's vblank event handler at VSTARTUP
+ *
+ * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
+ * * We are close enough to VUPDATE - the point of no return for hw
+ * * We are in the fixed portion of variable front porch when vrr is enabled
+ * * We are before VUPDATE, where double-buffered vrr registers are swapped
+ *
+ * It is therefore the correct place to signal vblank, send user flip events,
+ * and update VRR.
+ */
+static void dm_dcn_crtc_high_irq(void *interrupt_params)
+{
+	struct common_irq_params *irq_params = interrupt_params;
+	struct amdgpu_device *adev = irq_params->adev;
+	struct amdgpu_crtc *acrtc;
+	struct dm_crtc_state *acrtc_state;
+	unsigned long flags;
+
+	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
+
+	if (!acrtc)
+		return;
+
+	acrtc_state = to_dm_crtc_state(acrtc->base.state);
+
+	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				amdgpu_dm_vrr_active(acrtc_state));
+
+	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
+	drm_crtc_handle_vblank(&acrtc->base);
+
+	spin_lock_irqsave(&adev->ddev->event_lock, flags);
+
+	if (acrtc_state->vrr_params.supported &&
+	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
+		mod_freesync_handle_v_update(
+		adev->dm.freesync_module,
+		acrtc_state->stream,
+		&acrtc_state->vrr_params);
+
+		dc_stream_adjust_vmin_vmax(
+			adev->dm.dc,
+			acrtc_state->stream,
+			&acrtc_state->vrr_params.adjust);
+	}
+
+	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+		if (acrtc->event) {
+			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
+			acrtc->event = NULL;
+			drm_crtc_vblank_put(&acrtc->base);
+		}
+		acrtc->pflip_status = AMDGPU_FLIP_NONE;
+	}
+
+	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
+}
+
 static int dm_set_clockgating_state(void *handle,
 		  enum amd_clockgating_state state)
 {
@@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 		c_irq_params->irq_src = int_params.irq_source;

 		amdgpu_dm_irq_register_interrupt(adev, &int_params,
-				dm_crtc_high_irq, c_irq_params);
+				dm_dcn_crtc_high_irq, c_irq_params);
 	}

 	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 15:34     ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li-5C7GfCeVMHo @ 2019-11-05 15:34 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Leo Li, mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w,
	harry.wentland-5C7GfCeVMHo, nicholas.kazlauskas-5C7GfCeVMHo

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayerd user events due to vrr are sent off there.

[How]

Remove registration of VUpdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
 1 file changed, 4 insertions(+), 30 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 256a23a0ec28..568df046b2fe 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 	int rc = 0;

-	if (enable) {
+	if (enable && adev->family < AMDGPU_FAMILY_AI) {
 		/* vblank irq on -> Only need vupdate irq in vrr mode */
 		if (amdgpu_dm_vrr_active(acrtc_state))
 			rc = dm_set_vupdate_irq(crtc, true);
@@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
 static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 					    struct dm_crtc_state *new_state)
 {
+	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
 	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
 	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);

@@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * We also need vupdate irq for the actual core vblank handling
 		 * at end of vblank.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, true);
+		if (adev->family < AMDGPU_FAMILY_AI)
+			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);
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 15:34     ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li @ 2019-11-05 15:34 UTC (permalink / raw)
  To: amd-gfx; +Cc: Leo Li, mario.kleiner.de, harry.wentland, nicholas.kazlauskas

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayerd user events due to vrr are sent off there.

[How]

Remove registration of VUpdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
 1 file changed, 4 insertions(+), 30 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 256a23a0ec28..568df046b2fe 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 	int rc = 0;

-	if (enable) {
+	if (enable && adev->family < AMDGPU_FAMILY_AI) {
 		/* vblank irq on -> Only need vupdate irq in vrr mode */
 		if (amdgpu_dm_vrr_active(acrtc_state))
 			rc = dm_set_vupdate_irq(crtc, true);
@@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
 static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 					    struct dm_crtc_state *new_state)
 {
+	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
 	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
 	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);

@@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * We also need vupdate irq for the actual core vblank handling
 		 * at end of vblank.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, true);
+		if (adev->family < AMDGPU_FAMILY_AI)
+			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);
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 15:58         ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li-5C7GfCeVMHo @ 2019-11-05 15:58 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Leo Li, mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w,
	harry.wentland-5C7GfCeVMHo, nicholas.kazlauskas-5C7GfCeVMHo

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayerd user events due to vrr are sent off there.

[How]

Remove registration of VUpdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
 1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 	int rc = 0;

-	if (enable) {
+	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
 		/* vblank irq on -> Only need vupdate irq in vrr mode */
 		if (amdgpu_dm_vrr_active(acrtc_state))
 			rc = dm_set_vupdate_irq(crtc, true);
@@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
 static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 					    struct dm_crtc_state *new_state)
 {
+	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
 	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
 	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);

@@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * We also need vupdate irq for the actual core vblank handling
 		 * at end of vblank.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, true);
+		if (adev->family <= AMDGPU_FAMILY_AI)
+			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);
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 15:58         ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li @ 2019-11-05 15:58 UTC (permalink / raw)
  To: amd-gfx; +Cc: Leo Li, mario.kleiner.de, harry.wentland, nicholas.kazlauskas

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayerd user events due to vrr are sent off there.

[How]

Remove registration of VUpdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
 1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 	int rc = 0;

-	if (enable) {
+	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
 		/* vblank irq on -> Only need vupdate irq in vrr mode */
 		if (amdgpu_dm_vrr_active(acrtc_state))
 			rc = dm_set_vupdate_irq(crtc, true);
@@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
 static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 					    struct dm_crtc_state *new_state)
 {
+	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
 	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
 	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);

@@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * We also need vupdate irq for the actual core vblank handling
 		 * at end of vblank.
 		 */
-		dm_set_vupdate_irq(new_state->base.crtc, true);
+		if (adev->family <= AMDGPU_FAMILY_AI)
+			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);
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 16:15     ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 16:15 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry

On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
> interrupt. This is different from DCE, which has it assigned to vblank
> start.
> 
> We'd like to send vblank and user events at vstartup because:
> 
> * It happens close enough to vupdate - the point of no return for HW.
> 
> * It is programmed as lines relative to vblank end - i.e. it is not in
>    the variable portion when VRR is enabled. We should signal user
>    events here.
> 
> * The pflip interrupt responsible for sending user events today only
>    fires if the DCH HUBP component is not clock gated. In situations
>    where planes are disabled - but the CRTC is enabled - user events won't
>    be sent out, leading to flip done timeouts.
> 
> Consequently, this makes vupdate on DCN hardware redundant. It will be
> removed in the next change.
> 
> [How]
> 
> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
> signal. Inside the DCN handler, we send off user events if the pflip
> handler hasn't already done so.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>   1 file changed, 64 insertions(+), 1 deletion(-)
> 
> 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 00017b91c91a..256a23a0ec28 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   	}
>   }
> 
> +
> +/**
> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
> + * @interrupt params - interrupt parameters
> + *
> + * Notify DRM's vblank event handler at VSTARTUP
> + *
> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
> + * * We are close enough to VUPDATE - the point of no return for hw
> + * * We are in the fixed portion of variable front porch when vrr is enabled
> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
> + *
> + * It is therefore the correct place to signal vblank, send user flip events,
> + * and update VRR.
> + */
> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
> +{
> +	struct common_irq_params *irq_params = interrupt_params;
> +	struct amdgpu_device *adev = irq_params->adev;
> +	struct amdgpu_crtc *acrtc;
> +	struct dm_crtc_state *acrtc_state;
> +	unsigned long flags;
> +
> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> +
> +	if (!acrtc)
> +		return;
> +
> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
> +
> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> +				amdgpu_dm_vrr_active(acrtc_state));
> +
> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> +	drm_crtc_handle_vblank(&acrtc->base);

Shouldn't this be the other way around? Don't we want the CRC sent back 
to userspace to have the updated vblank counter?

This is how it worked before at least.

Other than that, this patch looks fine to me.

Nicholas Kazlauskas

> +
> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> +
> +	if (acrtc_state->vrr_params.supported &&
> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> +		mod_freesync_handle_v_update(
> +		adev->dm.freesync_module,
> +		acrtc_state->stream,
> +		&acrtc_state->vrr_params);
> +
> +		dc_stream_adjust_vmin_vmax(
> +			adev->dm.dc,
> +			acrtc_state->stream,
> +			&acrtc_state->vrr_params.adjust);
> +	}
> +
> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
> +		if (acrtc->event) {
> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
> +			acrtc->event = NULL;
> +			drm_crtc_vblank_put(&acrtc->base);
> +		}
> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
> +	}
> +
> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> +}
> +
>   static int dm_set_clockgating_state(void *handle,
>   		  enum amd_clockgating_state state)
>   {
> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   		c_irq_params->irq_src = int_params.irq_source;
> 
>   		amdgpu_dm_irq_register_interrupt(adev, &int_params,
> -				dm_crtc_high_irq, c_irq_params);
> +				dm_dcn_crtc_high_irq, c_irq_params);
>   	}
> 
>   	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
> --
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 16:15     ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 16:15 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry

On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
> interrupt. This is different from DCE, which has it assigned to vblank
> start.
> 
> We'd like to send vblank and user events at vstartup because:
> 
> * It happens close enough to vupdate - the point of no return for HW.
> 
> * It is programmed as lines relative to vblank end - i.e. it is not in
>    the variable portion when VRR is enabled. We should signal user
>    events here.
> 
> * The pflip interrupt responsible for sending user events today only
>    fires if the DCH HUBP component is not clock gated. In situations
>    where planes are disabled - but the CRTC is enabled - user events won't
>    be sent out, leading to flip done timeouts.
> 
> Consequently, this makes vupdate on DCN hardware redundant. It will be
> removed in the next change.
> 
> [How]
> 
> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
> signal. Inside the DCN handler, we send off user events if the pflip
> handler hasn't already done so.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>   1 file changed, 64 insertions(+), 1 deletion(-)
> 
> 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 00017b91c91a..256a23a0ec28 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   	}
>   }
> 
> +
> +/**
> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
> + * @interrupt params - interrupt parameters
> + *
> + * Notify DRM's vblank event handler at VSTARTUP
> + *
> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
> + * * We are close enough to VUPDATE - the point of no return for hw
> + * * We are in the fixed portion of variable front porch when vrr is enabled
> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
> + *
> + * It is therefore the correct place to signal vblank, send user flip events,
> + * and update VRR.
> + */
> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
> +{
> +	struct common_irq_params *irq_params = interrupt_params;
> +	struct amdgpu_device *adev = irq_params->adev;
> +	struct amdgpu_crtc *acrtc;
> +	struct dm_crtc_state *acrtc_state;
> +	unsigned long flags;
> +
> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> +
> +	if (!acrtc)
> +		return;
> +
> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
> +
> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> +				amdgpu_dm_vrr_active(acrtc_state));
> +
> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> +	drm_crtc_handle_vblank(&acrtc->base);

Shouldn't this be the other way around? Don't we want the CRC sent back 
to userspace to have the updated vblank counter?

This is how it worked before at least.

Other than that, this patch looks fine to me.

Nicholas Kazlauskas

> +
> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> +
> +	if (acrtc_state->vrr_params.supported &&
> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> +		mod_freesync_handle_v_update(
> +		adev->dm.freesync_module,
> +		acrtc_state->stream,
> +		&acrtc_state->vrr_params);
> +
> +		dc_stream_adjust_vmin_vmax(
> +			adev->dm.dc,
> +			acrtc_state->stream,
> +			&acrtc_state->vrr_params.adjust);
> +	}
> +
> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
> +		if (acrtc->event) {
> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
> +			acrtc->event = NULL;
> +			drm_crtc_vblank_put(&acrtc->base);
> +		}
> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
> +	}
> +
> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> +}
> +
>   static int dm_set_clockgating_state(void *handle,
>   		  enum amd_clockgating_state state)
>   {
> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   		c_irq_params->irq_src = int_params.irq_source;
> 
>   		amdgpu_dm_irq_register_interrupt(adev, &int_params,
> -				dm_crtc_high_irq, c_irq_params);
> +				dm_dcn_crtc_high_irq, c_irq_params);
>   	}
> 
>   	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
> --
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 16:16             ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 16:16 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry

On 2019-11-05 10:58 a.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayerd user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of VUpdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
>   1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_dcn_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;
> @@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>   	int rc = 0;
> 
> -	if (enable) {
> +	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
>   		/* vblank irq on -> Only need vupdate irq in vrr mode */
>   		if (amdgpu_dm_vrr_active(acrtc_state))
>   			rc = dm_set_vupdate_irq(crtc, true);
> @@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
>   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   					    struct dm_crtc_state *new_state)
>   {
> +	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
>   	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
>   	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
> 
> @@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		 * We also need vupdate irq for the actual core vblank handling
>   		 * at end of vblank.
>   		 */
> -		dm_set_vupdate_irq(new_state->base.crtc, true);
> +		if (adev->family <= AMDGPU_FAMILY_AI)
> +			dm_set_vupdate_irq(new_state->base.crtc, true);

Can we move the if into dm_set_vupdate_irq directly? We're setting it to 
false even when we don't have it with this patch.

Nicholas Kazlauskas

>   		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);
> --
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 16:16             ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 16:16 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry

On 2019-11-05 10:58 a.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayerd user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of VUpdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
>   1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_dcn_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;
> @@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>   	int rc = 0;
> 
> -	if (enable) {
> +	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
>   		/* vblank irq on -> Only need vupdate irq in vrr mode */
>   		if (amdgpu_dm_vrr_active(acrtc_state))
>   			rc = dm_set_vupdate_irq(crtc, true);
> @@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
>   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   					    struct dm_crtc_state *new_state)
>   {
> +	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
>   	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
>   	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
> 
> @@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		 * We also need vupdate irq for the actual core vblank handling
>   		 * at end of vblank.
>   		 */
> -		dm_set_vupdate_irq(new_state->base.crtc, true);
> +		if (adev->family <= AMDGPU_FAMILY_AI)
> +			dm_set_vupdate_irq(new_state->base.crtc, true);

Can we move the if into dm_set_vupdate_irq directly? We're setting it to 
false even when we don't have it with this patch.

Nicholas Kazlauskas

>   		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);
> --
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 18:32         ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 18:32 UTC (permalink / raw)
  To: Kazlauskas, Nicholas, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry



On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> [Why]
>>
>> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
>> interrupt. This is different from DCE, which has it assigned to vblank
>> start.
>>
>> We'd like to send vblank and user events at vstartup because:
>>
>> * It happens close enough to vupdate - the point of no return for HW.
>>
>> * It is programmed as lines relative to vblank end - i.e. it is not in
>>    the variable portion when VRR is enabled. We should signal user
>>    events here.
>>
>> * The pflip interrupt responsible for sending user events today only
>>    fires if the DCH HUBP component is not clock gated. In situations
>>    where planes are disabled - but the CRTC is enabled - user events won't
>>    be sent out, leading to flip done timeouts.
>>
>> Consequently, this makes vupdate on DCN hardware redundant. It will be
>> removed in the next change.
>>
>> [How]
>>
>> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
>> signal. Inside the DCN handler, we send off user events if the pflip
>> handler hasn't already done so.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> 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 00017b91c91a..256a23a0ec28 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>   	}
>>   }
>>
>> +
>> +/**
>> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
>> + * @interrupt params - interrupt parameters
>> + *
>> + * Notify DRM's vblank event handler at VSTARTUP
>> + *
>> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
>> + * * We are close enough to VUPDATE - the point of no return for hw
>> + * * We are in the fixed portion of variable front porch when vrr is enabled
>> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
>> + *
>> + * It is therefore the correct place to signal vblank, send user flip events,
>> + * and update VRR.
>> + */
>> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
>> +{
>> +	struct common_irq_params *irq_params = interrupt_params;
>> +	struct amdgpu_device *adev = irq_params->adev;
>> +	struct amdgpu_crtc *acrtc;
>> +	struct dm_crtc_state *acrtc_state;
>> +	unsigned long flags;
>> +
>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>> +
>> +	if (!acrtc)
>> +		return;
>> +
>> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
>> +
>> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>> +				amdgpu_dm_vrr_active(acrtc_state));
>> +
>> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> +	drm_crtc_handle_vblank(&acrtc->base);
> 
> Shouldn't this be the other way around? Don't we want the CRC sent back 
> to userspace to have the updated vblank counter?
> 
> This is how it worked before at least.
> 
> Other than that, this patch looks fine to me.
> 
> Nicholas Kazlauskas


Looks like we're doing a crtc_accurate_vblank_count() inside the crc handler,
so I don't think order matters here.

Leo

> 
>> +
>> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> +
>> +	if (acrtc_state->vrr_params.supported &&
>> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>> +		mod_freesync_handle_v_update(
>> +		adev->dm.freesync_module,
>> +		acrtc_state->stream,
>> +		&acrtc_state->vrr_params);
>> +
>> +		dc_stream_adjust_vmin_vmax(
>> +			adev->dm.dc,
>> +			acrtc_state->stream,
>> +			&acrtc_state->vrr_params.adjust);
>> +	}
>> +
>> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
>> +		if (acrtc->event) {
>> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>> +			acrtc->event = NULL;
>> +			drm_crtc_vblank_put(&acrtc->base);
>> +		}
>> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>> +}
>> +
>>   static int dm_set_clockgating_state(void *handle,
>>   		  enum amd_clockgating_state state)
>>   {
>> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>   		c_irq_params->irq_src = int_params.irq_source;
>>
>>   		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>> -				dm_crtc_high_irq, c_irq_params);
>> +				dm_dcn_crtc_high_irq, c_irq_params);
>>   	}
>>
>>   	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>> --
>> 2.23.0
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 18:32         ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 18:32 UTC (permalink / raw)
  To: Kazlauskas, Nicholas, amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry



On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> [Why]
>>
>> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
>> interrupt. This is different from DCE, which has it assigned to vblank
>> start.
>>
>> We'd like to send vblank and user events at vstartup because:
>>
>> * It happens close enough to vupdate - the point of no return for HW.
>>
>> * It is programmed as lines relative to vblank end - i.e. it is not in
>>    the variable portion when VRR is enabled. We should signal user
>>    events here.
>>
>> * The pflip interrupt responsible for sending user events today only
>>    fires if the DCH HUBP component is not clock gated. In situations
>>    where planes are disabled - but the CRTC is enabled - user events won't
>>    be sent out, leading to flip done timeouts.
>>
>> Consequently, this makes vupdate on DCN hardware redundant. It will be
>> removed in the next change.
>>
>> [How]
>>
>> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
>> signal. Inside the DCN handler, we send off user events if the pflip
>> handler hasn't already done so.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> 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 00017b91c91a..256a23a0ec28 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>   	}
>>   }
>>
>> +
>> +/**
>> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
>> + * @interrupt params - interrupt parameters
>> + *
>> + * Notify DRM's vblank event handler at VSTARTUP
>> + *
>> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
>> + * * We are close enough to VUPDATE - the point of no return for hw
>> + * * We are in the fixed portion of variable front porch when vrr is enabled
>> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
>> + *
>> + * It is therefore the correct place to signal vblank, send user flip events,
>> + * and update VRR.
>> + */
>> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
>> +{
>> +	struct common_irq_params *irq_params = interrupt_params;
>> +	struct amdgpu_device *adev = irq_params->adev;
>> +	struct amdgpu_crtc *acrtc;
>> +	struct dm_crtc_state *acrtc_state;
>> +	unsigned long flags;
>> +
>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>> +
>> +	if (!acrtc)
>> +		return;
>> +
>> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
>> +
>> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>> +				amdgpu_dm_vrr_active(acrtc_state));
>> +
>> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> +	drm_crtc_handle_vblank(&acrtc->base);
> 
> Shouldn't this be the other way around? Don't we want the CRC sent back 
> to userspace to have the updated vblank counter?
> 
> This is how it worked before at least.
> 
> Other than that, this patch looks fine to me.
> 
> Nicholas Kazlauskas


Looks like we're doing a crtc_accurate_vblank_count() inside the crc handler,
so I don't think order matters here.

Leo

> 
>> +
>> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> +
>> +	if (acrtc_state->vrr_params.supported &&
>> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>> +		mod_freesync_handle_v_update(
>> +		adev->dm.freesync_module,
>> +		acrtc_state->stream,
>> +		&acrtc_state->vrr_params);
>> +
>> +		dc_stream_adjust_vmin_vmax(
>> +			adev->dm.dc,
>> +			acrtc_state->stream,
>> +			&acrtc_state->vrr_params.adjust);
>> +	}
>> +
>> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
>> +		if (acrtc->event) {
>> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>> +			acrtc->event = NULL;
>> +			drm_crtc_vblank_put(&acrtc->base);
>> +		}
>> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>> +}
>> +
>>   static int dm_set_clockgating_state(void *handle,
>>   		  enum amd_clockgating_state state)
>>   {
>> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>   		c_irq_params->irq_src = int_params.irq_source;
>>
>>   		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>> -				dm_crtc_high_irq, c_irq_params);
>> +				dm_dcn_crtc_high_irq, c_irq_params);
>>   	}
>>
>>   	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>> --
>> 2.23.0
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 18:54                 ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 18:54 UTC (permalink / raw)
  To: Kazlauskas, Nicholas, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry



On 2019-11-05 11:16 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:58 a.m., sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> [Why]
>>
>> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
>> handler redundant.
>>
>> All the vupdate handler does is handle vblank events, and update vrr
>> for DCE hw (excluding VEGA, more on that later). As far as usermode is
>> concerned. vstartup happens close enough to vupdate on DCN that it can
>> be considered the "same". Handling vblank and updating vrr at vstartup
>> effectively replaces vupdate on DCN.
>>
>> Vega is a bit special. Like DCN, the VRR registers on Vega are
>> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
>> vstartup interrupt. This means we can't quite remove the vupdate handler
>> for it, since delayerd user events due to vrr are sent off there.
>>
>> [How]
>>
>> Remove registration of VUpdate interrupt handler for DCN. Disable
>> vupdate interrupt if asic family DCN, enable otherwise.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>
>> v2: Don't exclude vega when enabling vupdate interrupts
>>
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
>>   1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>   				dm_dcn_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;
>> @@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>>   	int rc = 0;
>>
>> -	if (enable) {
>> +	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
>>   		/* vblank irq on -> Only need vupdate irq in vrr mode */
>>   		if (amdgpu_dm_vrr_active(acrtc_state))
>>   			rc = dm_set_vupdate_irq(crtc, true);
>> @@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
>>   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   					    struct dm_crtc_state *new_state)
>>   {
>> +	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
>>   	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
>>   	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
>>
>> @@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   		 * We also need vupdate irq for the actual core vblank handling
>>   		 * at end of vblank.
>>   		 */
>> -		dm_set_vupdate_irq(new_state->base.crtc, true);
>> +		if (adev->family <= AMDGPU_FAMILY_AI)
>> +			dm_set_vupdate_irq(new_state->base.crtc, true);
> 
> Can we move the if into dm_set_vupdate_irq directly? We're setting it to 
> false even when we don't have it with this patch.
> 
> Nicholas Kazlauskas

Good point, don't know why I didn't do that in the first place :)

Thanks,
Leo

> 
>>   		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);
>> --
>> 2.23.0
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 18:54                 ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 18:54 UTC (permalink / raw)
  To: Kazlauskas, Nicholas, amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry



On 2019-11-05 11:16 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:58 a.m., sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> [Why]
>>
>> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
>> handler redundant.
>>
>> All the vupdate handler does is handle vblank events, and update vrr
>> for DCE hw (excluding VEGA, more on that later). As far as usermode is
>> concerned. vstartup happens close enough to vupdate on DCN that it can
>> be considered the "same". Handling vblank and updating vrr at vstartup
>> effectively replaces vupdate on DCN.
>>
>> Vega is a bit special. Like DCN, the VRR registers on Vega are
>> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
>> vstartup interrupt. This means we can't quite remove the vupdate handler
>> for it, since delayerd user events due to vrr are sent off there.
>>
>> [How]
>>
>> Remove registration of VUpdate interrupt handler for DCN. Disable
>> vupdate interrupt if asic family DCN, enable otherwise.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>
>> v2: Don't exclude vega when enabling vupdate interrupts
>>
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
>>   1 file changed, 4 insertions(+), 30 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 256a23a0ec28..3664af3b41a1 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>   				dm_dcn_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;
>> @@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>>   	int rc = 0;
>>
>> -	if (enable) {
>> +	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
>>   		/* vblank irq on -> Only need vupdate irq in vrr mode */
>>   		if (amdgpu_dm_vrr_active(acrtc_state))
>>   			rc = dm_set_vupdate_irq(crtc, true);
>> @@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
>>   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   					    struct dm_crtc_state *new_state)
>>   {
>> +	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
>>   	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
>>   	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
>>
>> @@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   		 * We also need vupdate irq for the actual core vblank handling
>>   		 * at end of vblank.
>>   		 */
>> -		dm_set_vupdate_irq(new_state->base.crtc, true);
>> +		if (adev->family <= AMDGPU_FAMILY_AI)
>> +			dm_set_vupdate_irq(new_state->base.crtc, true);
> 
> Can we move the if into dm_set_vupdate_irq directly? We're setting it to 
> false even when we don't have it with this patch.
> 
> Nicholas Kazlauskas

Good point, don't know why I didn't do that in the first place :)

Thanks,
Leo

> 
>>   		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);
>> --
>> 2.23.0
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:01             ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li-5C7GfCeVMHo @ 2019-11-05 19:01 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Leo Li, mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w,
	harry.wentland-5C7GfCeVMHo, nicholas.kazlauskas-5C7GfCeVMHo

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayed user events due to vrr are sent off there.

[How]

Remove registration of vupdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

v3: Move FAMILY_AI check inside dm_set_vupdate_irq()

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
 1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 	struct amdgpu_device *adev = crtc->dev->dev_private;
 	int rc;
 
+	/* Do not set vupdate for DCN hardware */
+	if (adev->family <= AMDGPU_FAMILY_AI)
+		return 0;
+
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
 
 	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v3] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:01             ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li @ 2019-11-05 19:01 UTC (permalink / raw)
  To: amd-gfx; +Cc: Leo Li, mario.kleiner.de, harry.wentland, nicholas.kazlauskas

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayed user events due to vrr are sent off there.

[How]

Remove registration of vupdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

v3: Move FAMILY_AI check inside dm_set_vupdate_irq()

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
 1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 	struct amdgpu_device *adev = crtc->dev->dev_private;
 	int rc;
 
+	/* Do not set vupdate for DCN hardware */
+	if (adev->family <= AMDGPU_FAMILY_AI)
+		return 0;
+
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
 
 	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:04                 ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 19:04 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry,
	Kazlauskas, Nicholas



On 2019-11-05 2:01 p.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayed user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of vupdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
> v3: Move FAMILY_AI check inside dm_set_vupdate_irq()
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
>  1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>  				dm_dcn_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;
> @@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>  	struct amdgpu_device *adev = crtc->dev->dev_private;
>  	int rc;
>  
> +	/* Do not set vupdate for DCN hardware */
> +	if (adev->family <= AMDGPU_FAMILY_AI)

Err, no. s/<=/>/...

Leo

> +		return 0;
> +
>  	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>  
>  	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:04                 ` Li, Sun peng (Leo)
  0 siblings, 0 replies; 25+ messages in thread
From: Li, Sun peng (Leo) @ 2019-11-05 19:04 UTC (permalink / raw)
  To: amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry, Kazlauskas, Nicholas



On 2019-11-05 2:01 p.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayed user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of vupdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
> v3: Move FAMILY_AI check inside dm_set_vupdate_irq()
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
>  1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>  				dm_dcn_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;
> @@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>  	struct amdgpu_device *adev = crtc->dev->dev_private;
>  	int rc;
>  
> +	/* Do not set vupdate for DCN hardware */
> +	if (adev->family <= AMDGPU_FAMILY_AI)

Err, no. s/<=/>/...

Leo

> +		return 0;
> +
>  	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>  
>  	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v4] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:07             ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li-5C7GfCeVMHo @ 2019-11-05 19:07 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Leo Li, mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w,
	harry.wentland-5C7GfCeVMHo, nicholas.kazlauskas-5C7GfCeVMHo

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayed user events due to vrr are sent off there.

[How]

Remove registration of vupdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

v3: Move FAMILY_AI check inside dm_set_vupdate_irq()

v4: Correct a brain fart

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
 1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 	struct amdgpu_device *adev = crtc->dev->dev_private;
 	int rc;

+	/* Do not set vupdate for DCN hardware */
+	if (adev->family > AMDGPU_FAMILY_AI)
+		return 0;
+
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;

 	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 19:07             ` sunpeng.li
  0 siblings, 0 replies; 25+ messages in thread
From: sunpeng.li @ 2019-11-05 19:07 UTC (permalink / raw)
  To: amd-gfx; +Cc: Leo Li, mario.kleiner.de, harry.wentland, nicholas.kazlauskas

From: Leo Li <sunpeng.li@amd.com>

[Why]

On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
handler redundant.

All the vupdate handler does is handle vblank events, and update vrr
for DCE hw (excluding VEGA, more on that later). As far as usermode is
concerned. vstartup happens close enough to vupdate on DCN that it can
be considered the "same". Handling vblank and updating vrr at vstartup
effectively replaces vupdate on DCN.

Vega is a bit special. Like DCN, the VRR registers on Vega are
double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
vstartup interrupt. This means we can't quite remove the vupdate handler
for it, since delayed user events due to vrr are sent off there.

[How]

Remove registration of vupdate interrupt handler for DCN. Disable
vupdate interrupt if asic family DCN, enable otherwise.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---

v2: Don't exclude vega when enabling vupdate interrupts

v3: Move FAMILY_AI check inside dm_set_vupdate_irq()

v4: Correct a brain fart

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
 1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_dcn_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;
@@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 	struct amdgpu_device *adev = crtc->dev->dev_private;
 	int rc;

+	/* Do not set vupdate for DCN hardware */
+	if (adev->family > AMDGPU_FAMILY_AI)
+		return 0;
+
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;

 	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
--
2.23.0

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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 20:21                 ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 20:21 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry

On 2019-11-05 2:07 p.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayed user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of vupdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

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

> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
> v3: Move FAMILY_AI check inside dm_set_vupdate_irq()
> 
> v4: Correct a brain fart
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
>   1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_dcn_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;
> @@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
>   	int rc;
> 
> +	/* Do not set vupdate for DCN hardware */
> +	if (adev->family > AMDGPU_FAMILY_AI)
> +		return 0;
> +
>   	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> 
>   	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> --
> 2.23.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] drm/amd/display: Disable VUpdate interrupt for DCN hardware
@ 2019-11-05 20:21                 ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 20:21 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry

On 2019-11-05 2:07 p.m., sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
> handler redundant.
> 
> All the vupdate handler does is handle vblank events, and update vrr
> for DCE hw (excluding VEGA, more on that later). As far as usermode is
> concerned. vstartup happens close enough to vupdate on DCN that it can
> be considered the "same". Handling vblank and updating vrr at vstartup
> effectively replaces vupdate on DCN.
> 
> Vega is a bit special. Like DCN, the VRR registers on Vega are
> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
> vstartup interrupt. This means we can't quite remove the vupdate handler
> for it, since delayed user events due to vrr are sent off there.
> 
> [How]
> 
> Remove registration of vupdate interrupt handler for DCN. Disable
> vupdate interrupt if asic family DCN, enable otherwise.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

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

> ---
> 
> v2: Don't exclude vega when enabling vupdate interrupts
> 
> v3: Move FAMILY_AI check inside dm_set_vupdate_irq()
> 
> v4: Correct a brain fart
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++----------------
>   1 file changed, 4 insertions(+), 28 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 256a23a0ec28..d40185dfd0c0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_dcn_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;
> @@ -4249,6 +4221,10 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
>   	int rc;
> 
> +	/* Do not set vupdate for DCN hardware */
> +	if (adev->family > AMDGPU_FAMILY_AI)
> +		return 0;
> +
>   	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> 
>   	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> --
> 2.23.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 20:21             ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 20:21 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w, Wentland, Harry

On 2019-11-05 1:32 p.m., Li, Sun peng (Leo) wrote:
> 
> 
> On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
>> On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
>>> From: Leo Li <sunpeng.li@amd.com>
>>>
>>> [Why]
>>>
>>> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
>>> interrupt. This is different from DCE, which has it assigned to vblank
>>> start.
>>>
>>> We'd like to send vblank and user events at vstartup because:
>>>
>>> * It happens close enough to vupdate - the point of no return for HW.
>>>
>>> * It is programmed as lines relative to vblank end - i.e. it is not in
>>>     the variable portion when VRR is enabled. We should signal user
>>>     events here.
>>>
>>> * The pflip interrupt responsible for sending user events today only
>>>     fires if the DCH HUBP component is not clock gated. In situations
>>>     where planes are disabled - but the CRTC is enabled - user events won't
>>>     be sent out, leading to flip done timeouts.
>>>
>>> Consequently, this makes vupdate on DCN hardware redundant. It will be
>>> removed in the next change.
>>>
>>> [How]
>>>
>>> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
>>> signal. Inside the DCN handler, we send off user events if the pflip
>>> handler hasn't already done so.
>>>
>>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>>> ---
>>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>>>    1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> 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 00017b91c91a..256a23a0ec28 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>>    	}
>>>    }
>>>
>>> +
>>> +/**
>>> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
>>> + * @interrupt params - interrupt parameters
>>> + *
>>> + * Notify DRM's vblank event handler at VSTARTUP
>>> + *
>>> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
>>> + * * We are close enough to VUPDATE - the point of no return for hw
>>> + * * We are in the fixed portion of variable front porch when vrr is enabled
>>> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
>>> + *
>>> + * It is therefore the correct place to signal vblank, send user flip events,
>>> + * and update VRR.
>>> + */
>>> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
>>> +{
>>> +	struct common_irq_params *irq_params = interrupt_params;
>>> +	struct amdgpu_device *adev = irq_params->adev;
>>> +	struct amdgpu_crtc *acrtc;
>>> +	struct dm_crtc_state *acrtc_state;
>>> +	unsigned long flags;
>>> +
>>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>> +
>>> +	if (!acrtc)
>>> +		return;
>>> +
>>> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>> +
>>> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>>> +				amdgpu_dm_vrr_active(acrtc_state));
>>> +
>>> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>> +	drm_crtc_handle_vblank(&acrtc->base);
>>
>> Shouldn't this be the other way around? Don't we want the CRC sent back
>> to userspace to have the updated vblank counter?
>>
>> This is how it worked before at least.
>>
>> Other than that, this patch looks fine to me.
>>
>> Nicholas Kazlauskas
> 
> 
> Looks like we're doing a crtc_accurate_vblank_count() inside the crc handler,
> so I don't think order matters here.
> 
> Leo

Sounds fine to me then.

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

Nicholas Kazlauskas

> 
>>
>>> +
>>> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> +
>>> +	if (acrtc_state->vrr_params.supported &&
>>> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>>> +		mod_freesync_handle_v_update(
>>> +		adev->dm.freesync_module,
>>> +		acrtc_state->stream,
>>> +		&acrtc_state->vrr_params);
>>> +
>>> +		dc_stream_adjust_vmin_vmax(
>>> +			adev->dm.dc,
>>> +			acrtc_state->stream,
>>> +			&acrtc_state->vrr_params.adjust);
>>> +	}
>>> +
>>> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
>>> +		if (acrtc->event) {
>>> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>>> +			acrtc->event = NULL;
>>> +			drm_crtc_vblank_put(&acrtc->base);
>>> +		}
>>> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
>>> +	}
>>> +
>>> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>>> +}
>>> +
>>>    static int dm_set_clockgating_state(void *handle,
>>>    		  enum amd_clockgating_state state)
>>>    {
>>> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>>    		c_irq_params->irq_src = int_params.irq_source;
>>>
>>>    		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>>> -				dm_crtc_high_irq, c_irq_params);
>>> +				dm_dcn_crtc_high_irq, c_irq_params);
>>>    	}
>>>
>>>    	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>>> --
>>> 2.23.0
>>>
>>

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
@ 2019-11-05 20:21             ` Kazlauskas, Nicholas
  0 siblings, 0 replies; 25+ messages in thread
From: Kazlauskas, Nicholas @ 2019-11-05 20:21 UTC (permalink / raw)
  To: Li, Sun peng (Leo), amd-gfx; +Cc: mario.kleiner.de, Wentland, Harry

On 2019-11-05 1:32 p.m., Li, Sun peng (Leo) wrote:
> 
> 
> On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
>> On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
>>> From: Leo Li <sunpeng.li@amd.com>
>>>
>>> [Why]
>>>
>>> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
>>> interrupt. This is different from DCE, which has it assigned to vblank
>>> start.
>>>
>>> We'd like to send vblank and user events at vstartup because:
>>>
>>> * It happens close enough to vupdate - the point of no return for HW.
>>>
>>> * It is programmed as lines relative to vblank end - i.e. it is not in
>>>     the variable portion when VRR is enabled. We should signal user
>>>     events here.
>>>
>>> * The pflip interrupt responsible for sending user events today only
>>>     fires if the DCH HUBP component is not clock gated. In situations
>>>     where planes are disabled - but the CRTC is enabled - user events won't
>>>     be sent out, leading to flip done timeouts.
>>>
>>> Consequently, this makes vupdate on DCN hardware redundant. It will be
>>> removed in the next change.
>>>
>>> [How]
>>>
>>> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
>>> signal. Inside the DCN handler, we send off user events if the pflip
>>> handler hasn't already done so.
>>>
>>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>>> ---
>>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>>>    1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> 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 00017b91c91a..256a23a0ec28 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>>    	}
>>>    }
>>>
>>> +
>>> +/**
>>> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
>>> + * @interrupt params - interrupt parameters
>>> + *
>>> + * Notify DRM's vblank event handler at VSTARTUP
>>> + *
>>> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
>>> + * * We are close enough to VUPDATE - the point of no return for hw
>>> + * * We are in the fixed portion of variable front porch when vrr is enabled
>>> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
>>> + *
>>> + * It is therefore the correct place to signal vblank, send user flip events,
>>> + * and update VRR.
>>> + */
>>> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
>>> +{
>>> +	struct common_irq_params *irq_params = interrupt_params;
>>> +	struct amdgpu_device *adev = irq_params->adev;
>>> +	struct amdgpu_crtc *acrtc;
>>> +	struct dm_crtc_state *acrtc_state;
>>> +	unsigned long flags;
>>> +
>>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>> +
>>> +	if (!acrtc)
>>> +		return;
>>> +
>>> +	acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>> +
>>> +	DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>>> +				amdgpu_dm_vrr_active(acrtc_state));
>>> +
>>> +	amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>> +	drm_crtc_handle_vblank(&acrtc->base);
>>
>> Shouldn't this be the other way around? Don't we want the CRC sent back
>> to userspace to have the updated vblank counter?
>>
>> This is how it worked before at least.
>>
>> Other than that, this patch looks fine to me.
>>
>> Nicholas Kazlauskas
> 
> 
> Looks like we're doing a crtc_accurate_vblank_count() inside the crc handler,
> so I don't think order matters here.
> 
> Leo

Sounds fine to me then.

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

Nicholas Kazlauskas

> 
>>
>>> +
>>> +	spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> +
>>> +	if (acrtc_state->vrr_params.supported &&
>>> +	    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>>> +		mod_freesync_handle_v_update(
>>> +		adev->dm.freesync_module,
>>> +		acrtc_state->stream,
>>> +		&acrtc_state->vrr_params);
>>> +
>>> +		dc_stream_adjust_vmin_vmax(
>>> +			adev->dm.dc,
>>> +			acrtc_state->stream,
>>> +			&acrtc_state->vrr_params.adjust);
>>> +	}
>>> +
>>> +	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
>>> +		if (acrtc->event) {
>>> +			drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>>> +			acrtc->event = NULL;
>>> +			drm_crtc_vblank_put(&acrtc->base);
>>> +		}
>>> +		acrtc->pflip_status = AMDGPU_FLIP_NONE;
>>> +	}
>>> +
>>> +	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>>> +}
>>> +
>>>    static int dm_set_clockgating_state(void *handle,
>>>    		  enum amd_clockgating_state state)
>>>    {
>>> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>>    		c_irq_params->irq_src = int_params.irq_source;
>>>
>>>    		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>>> -				dm_crtc_high_irq, c_irq_params);
>>> +				dm_dcn_crtc_high_irq, c_irq_params);
>>>    	}
>>>
>>>    	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>>> --
>>> 2.23.0
>>>
>>

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN
  2019-11-05 18:32         ` Li, Sun peng (Leo)
  (?)
  (?)
@ 2019-11-29 19:20         ` Mario Kleiner
  -1 siblings, 0 replies; 25+ messages in thread
From: Mario Kleiner @ 2019-11-29 19:20 UTC (permalink / raw)
  To: Li, Sun peng (Leo)
  Cc: Alex Deucher, Mario Kleiner, Wentland, Harry, Kazlauskas,
	Nicholas, amd-gfx


[-- Attachment #1.1: Type: text/plain, Size: 10704 bytes --]

Hi Leo and others,

sorry for the late reply. I just spent some time looking at your patches
and testing them on a Raven DCN-1.

I looked at how the vstartup line is computed in the dc_bandwidth_calcs
etc., and added some DRM_DEBUG statements to the dm_dcn_crtc_high_irq and
dm_pflip_high_irq handlers to print the scanline at which the handlers get
invoked.

From my reading and the results my understanding is that VSTARTUP always
fires after end of front-porch in VRR mode, so the dm_dcn_crtc_high_irq
handler will only get invoked in the vsync/back-porch area? This is good
and very important, as otherwise all the vblank and timestamp calculations
would often be wrong (if it ever happened inside front-porch).

Could you give me some overview of which interrupts / hw events happens
when on DCN vs DCE? I intend to spend quite a bit of quality time in
December playing with the freesync code in DC and see if i can hack up some
proof-of-concept for precisely timed pageflips - the approach Harry
suggested in his XDC2019 talk which i finally found time to watch. I think
with the highly precise vblank and pageflip timestamps we should be able to
implement this precisely without the need for (jittery) software timers,
just some extensions to DRR hw programming and some trickery similar to
what below-the-range BTR support does. That would be so cool, especially
for neuro-science/vision-science/medical research applications.

My rough undestanding so far for DCN seems to be:

1. Pageflips can execute in front-porch, ie. the register double-buffering
can switch there. Can they also still execute after front-porch? How far
into vsync/back-porch? I assume at some point close to the end of
back-porch they can't anymore, because after a flip the line buffer needs
time to prefetch the new pixeldata from the new scanout buffer [a]?

2. The VSTARTUP interrupt/event in VRR mode happens somewhere programmable
after end of front-porch (suggested by the bandwidth calc code), but before
VUPDATE? Is VSTARTUP the last point at which double-buffering for a
pageflip can happen, ie. after that the line-buffer refill for the next
frame starts, ie. [a]?

3. The VUPDATE interrupt/event marks the end of vblank? And that's where
double-buffering / switch of new values for the DRR registers happens? So
DRR values programmed before VUPDATE will take effect after VUPDATE and
thereby affect the vblank after the current one ie. after the following
video frame?

Is this correct? And how does it differ from Vega/DCE-12 and older <=
Polaris / <= DCE-11 ? I remember from earlier this year that BTR works much
better on DCN (tested) and DCE-12 (presumably, don't have hw to test) than
it does on DCE-11 and earlier. This was due to different behaviour of when
the DRR programing takes effect, allowing for much quicker switching on
DCN. I'd like to understand in detail how the DRR
switching/latching/double-buffering differs, if one of you can enlighten me.

There's one thing about this patch though that i think is not right. The
sending of pageflip completion events from within dm_dcn_crtc_high_irq()
seems to be both not needed and possibly causing potentially wrong results
in pageflip events/timestamps / visual glitches due to races?

Two cases:

a) If a pageflip completes in front porch and the pageflip handler
dm_pflip_high_irq() executes while in front-porch, it will queue the proper
pageflip event for later delivery to user space by drm_crtc_handle_vblank()
which is called by dm_dcn_crtc_high_irq() already.

b) If dm_pflip_high_irq() executes after front-porch (pageflip completes in
back-porch if this is possible), it will deliver the pageflip event itself
after updating the vblank count and timestamps correctly via
drm_crtc_accurate_vblank_count().

There isn't a need for the extra code in your patch (if
(acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {...}) and indeed i can just
comment it out and everything works fine.

I think the code could be even harmful if a pageflip gets queued into the
hardware before invocation of dm_dcn_crtc_high_irq() (ie. a bit before
VSTARTUP + irq handling delay), but after missing the deadline for
double-buffering of the hardwares primary surface base address registers.
You could end up with setting acrtc->pflip_status = AMDGPU_FLIP_SUBMITTED,
missing the hw double-buffering deadline, and then dm_dcn_crtc_high_irq()
would decide to send out a pageflip completion event to userspace for a
flip that hasn't actually taken place in the hw in this vblank. Userspace
would then misschedule its presentation due to the wrong pageflip event /
timestamp and you'd end up with the previous rendered image presented one
scanout cycle too long, and the current image silently dropped and never
displayed!

Indeed debug output i added shows that the dm_pflip_high_irq() handler
essentially turns into doing nothing with your patch applied, so pageflip
completion events sent to user space no longer correspond to true hw flips.

I have some hw measuring equipment to verify flip timing independent of the
driver and during a few short test runs i think i observed this glitch at
least once, suggesting the problem is real.

thanks,
-mario

On Tue, Nov 5, 2019 at 7:32 PM Li, Sun peng (Leo) <Sunpeng.Li@amd.com>
wrote:

>
>
> On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
> > On 2019-11-05 10:34 a.m., sunpeng.li@amd.com wrote:
> >> From: Leo Li <sunpeng.li@amd.com>
> >>
> >> [Why]
> >>
> >> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
> >> interrupt. This is different from DCE, which has it assigned to vblank
> >> start.
> >>
> >> We'd like to send vblank and user events at vstartup because:
> >>
> >> * It happens close enough to vupdate - the point of no return for HW.
> >>
> >> * It is programmed as lines relative to vblank end - i.e. it is not in
> >>    the variable portion when VRR is enabled. We should signal user
> >>    events here.
> >>
> >> * The pflip interrupt responsible for sending user events today only
> >>    fires if the DCH HUBP component is not clock gated. In situations
> >>    where planes are disabled - but the CRTC is enabled - user events
> won't
> >>    be sent out, leading to flip done timeouts.
> >>
> >> Consequently, this makes vupdate on DCN hardware redundant. It will be
> >> removed in the next change.
> >>
> >> [How]
> >>
> >> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
> >> signal. Inside the DCN handler, we send off user events if the pflip
> >> handler hasn't already done so.
> >>
> >> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> >> ---
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
> >>   1 file changed, 64 insertions(+), 1 deletion(-)
> >>
> >> 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 00017b91c91a..256a23a0ec28 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void
> *interrupt_params)
> >>      }
> >>   }
> >>
> >> +
> >> +/**
> >> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN
> generation ASICs
> >> + * @interrupt params - interrupt parameters
> >> + *
> >> + * Notify DRM's vblank event handler at VSTARTUP
> >> + *
> >> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
> >> + * * We are close enough to VUPDATE - the point of no return for hw
> >> + * * We are in the fixed portion of variable front porch when vrr is
> enabled
> >> + * * We are before VUPDATE, where double-buffered vrr registers are
> swapped
> >> + *
> >> + * It is therefore the correct place to signal vblank, send user flip
> events,
> >> + * and update VRR.
> >> + */
> >> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
> >> +{
> >> +    struct common_irq_params *irq_params = interrupt_params;
> >> +    struct amdgpu_device *adev = irq_params->adev;
> >> +    struct amdgpu_crtc *acrtc;
> >> +    struct dm_crtc_state *acrtc_state;
> >> +    unsigned long flags;
> >> +
> >> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src -
> IRQ_TYPE_VBLANK);
> >> +
> >> +    if (!acrtc)
> >> +            return;
> >> +
> >> +    acrtc_state = to_dm_crtc_state(acrtc->base.state);
> >> +
> >> +    DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> >> +                            amdgpu_dm_vrr_active(acrtc_state));
> >> +
> >> +    amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >> +    drm_crtc_handle_vblank(&acrtc->base);
> >
> > Shouldn't this be the other way around? Don't we want the CRC sent back
> > to userspace to have the updated vblank counter?
> >
> > This is how it worked before at least.
> >
> > Other than that, this patch looks fine to me.
> >
> > Nicholas Kazlauskas
>
>
> Looks like we're doing a crtc_accurate_vblank_count() inside the crc
> handler,
> so I don't think order matters here.
>
> Leo
>
> >
> >> +
> >> +    spin_lock_irqsave(&adev->ddev->event_lock, flags)
> >> +
> >> +    if (acrtc_state->vrr_params.supported &&
> >> +        acrtc_state->freesync_config.state ==
> VRR_STATE_ACTIVE_VARIABLE) {
> >> +            mod_freesync_handle_v_update(
> >> +            adev->dm.freesync_module,
> >> +            acrtc_state->stream,
> >> +            &acrtc_state->vrr_params);
> >> +
> >> +            dc_stream_adjust_vmin_vmax(
> >> +                    adev->dm.dc,
> >> +                    acrtc_state->stream,
> >> +                    &acrtc_state->vrr_params.adjust);
> >> +    }
> >> +
> >> +    if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
> >> +            if (acrtc->event) {
> >> +                    drm_crtc_send_vblank_event(&acrtc->base,
> acrtc->event);
> >> +                    acrtc->event = NULL;
> >> +                    drm_crtc_vblank_put(&acrtc->base);
> >> +            }
> >> +            acrtc->pflip_status = AMDGPU_FLIP_NONE;
> >> +    }
> >> +
> >> +    spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> >> +}
> >> +
> >>   static int dm_set_clockgating_state(void *handle,
> >>                enum amd_clockgating_state state)
> >>   {
> >> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct
> amdgpu_device *adev)
> >>              c_irq_params->irq_src = int_params.irq_source;
> >>
> >>              amdgpu_dm_irq_register_interrupt(adev, &int_params,
> >> -                            dm_crtc_high_irq, c_irq_params);
> >> +                            dm_dcn_crtc_high_irq, c_irq_params);
> >>      }
> >>
> >>      /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond
> to
> >> --
> >> 2.23.0
> >>
> >
>

[-- Attachment #1.2: Type: text/html, Size: 13295 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-11-29 19:20 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 15:34 [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN sunpeng.li-5C7GfCeVMHo
2019-11-05 15:34 ` sunpeng.li
     [not found] ` <20191105153416.32049-1-sunpeng.li-5C7GfCeVMHo@public.gmane.org>
2019-11-05 15:34   ` [PATCH 2/2] drm/amd/display: Disable VUpdate interrupt for DCN hardware sunpeng.li-5C7GfCeVMHo
2019-11-05 15:34     ` sunpeng.li
     [not found]     ` <20191105153416.32049-2-sunpeng.li-5C7GfCeVMHo@public.gmane.org>
2019-11-05 15:58       ` [PATCH 2/2 v2] " sunpeng.li-5C7GfCeVMHo
2019-11-05 15:58         ` sunpeng.li
     [not found]         ` <20191105155802.1302-1-sunpeng.li-5C7GfCeVMHo@public.gmane.org>
2019-11-05 16:16           ` Kazlauskas, Nicholas
2019-11-05 16:16             ` Kazlauskas, Nicholas
     [not found]             ` <47f9dd1b-66c1-a521-d6c8-b9422616cf2e-5C7GfCeVMHo@public.gmane.org>
2019-11-05 18:54               ` Li, Sun peng (Leo)
2019-11-05 18:54                 ` Li, Sun peng (Leo)
2019-11-05 19:01           ` [PATCH v3] " sunpeng.li-5C7GfCeVMHo
2019-11-05 19:01             ` sunpeng.li
     [not found]             ` <20191105190147.7283-1-sunpeng.li-5C7GfCeVMHo@public.gmane.org>
2019-11-05 19:04               ` Li, Sun peng (Leo)
2019-11-05 19:04                 ` Li, Sun peng (Leo)
2019-11-05 19:07           ` [PATCH v4] " sunpeng.li-5C7GfCeVMHo
2019-11-05 19:07             ` sunpeng.li
     [not found]             ` <20191105190709.7816-1-sunpeng.li-5C7GfCeVMHo@public.gmane.org>
2019-11-05 20:21               ` Kazlauskas, Nicholas
2019-11-05 20:21                 ` Kazlauskas, Nicholas
2019-11-05 16:15   ` [PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN Kazlauskas, Nicholas
2019-11-05 16:15     ` Kazlauskas, Nicholas
     [not found]     ` <c93c503d-48dc-1ea5-19f7-42ff9392e162-5C7GfCeVMHo@public.gmane.org>
2019-11-05 18:32       ` Li, Sun peng (Leo)
2019-11-05 18:32         ` Li, Sun peng (Leo)
     [not found]         ` <ed7b7f5e-4a53-f3e1-912f-e0ae5181288c-5C7GfCeVMHo@public.gmane.org>
2019-11-05 20:21           ` Kazlauskas, Nicholas
2019-11-05 20:21             ` Kazlauskas, Nicholas
2019-11-29 19:20         ` Mario Kleiner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.