[PATCH 1/2] drm/amd/display: Add missing DCN30 registers and fields for OTG_CRC_CNTL2

[PATCH 1/2] drm/amd/display: Add missing DCN30 registers and fields for OTG_CRC_CNTL2

[Nicholas Kazlauskas]

[Why]
When enabling the debugfs for CRC capture we hit assertions caused by
register address and field masks and shifts missing.

[How]
We want these registers programmed, so add in the SRI/SF entries for
this register.

Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
index d4106dd5a9b0..33f13c1e7520 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
@@ -98,6 +98,7 @@
 	SRI(OTG_GSL_WINDOW_Y, OTG, inst),\
 	SRI(OTG_VUPDATE_KEEPOUT, OTG, inst),\
 	SRI(OTG_DSC_START_POSITION, OTG, inst),\
+	SRI(OTG_CRC_CNTL2, OTG, inst),\
 	SRI(OTG_DRR_TRIGGER_WINDOW, OTG, inst),\
 	SRI(OTG_DRR_V_TOTAL_CHANGE, OTG, inst),\
 	SRI(OPTC_DATA_FORMAT_CONTROL, ODM, inst),\
@@ -248,6 +249,10 @@
 	SF(OTG0_OTG_GSL_CONTROL, OTG_MASTER_UPDATE_LOCK_GSL_EN, mask_sh), \
 	SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_X, mask_sh), \
 	SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_LINE_NUM, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DSC_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_COMBINE_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_SPLIT_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_FORMAT, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG0_SRC_SEL, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG1_SRC_SEL, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_NUM_OF_INPUT_SEGMENT, mask_sh),\
@@ -280,6 +285,10 @@
 	SF(OTG0_OTG_GSL_CONTROL, OTG_MASTER_UPDATE_LOCK_GSL_EN, mask_sh), \
 	SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_X, mask_sh), \
 	SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_LINE_NUM, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DSC_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_COMBINE_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_SPLIT_MODE, mask_sh),\
+	SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_FORMAT, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG0_SRC_SEL, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG1_SRC_SEL, mask_sh),\
 	SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG2_SRC_SEL, mask_sh),\
-- 
2.25.1

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

[PATCH 2/2] drm/amd/display: Allow for vblank enabled with no active planes

[Nicholas Kazlauskas]

[Why]
CRC capture doesn't work when the active plane count is 0 since we
currently tie both vblank and pageflip interrupts to active_plane_count
greater than 0.

[How]
The frontend is what generates the vblank interrupts while the backend
is what generates pageflip interrupts. Both have a requirement for
the CRTC to be active, so control the overall interrupt state based
on that instead.

Pageflip interrupts need to be enabled based on active plane count, but
we actually rely on power gating to take care of disabling the interrupt
for us on pipes that can be power gated.

For pipes that can't be power gated it's still fine to leave it enabled
since the interrupt only triggers after the address has been written
to that particular pipe - which we won't be doing without an active
plane.

The issue we had before with this setup was that we couldn't force
the state back on. We were essentially manipulating the refcount
to enable or disable as needed in a two pass approach.

However, there is a function that solves this problem more elegantly:
amdgpu_irq_update() will unconditionally call the set based on what it
thinks the current enablement state is.

This leaves two future TODO items for our IRQ handling:
- Disabling IRQs in commit tail instead of atomic commit
- Mapping the pageflip interrupt to VUPDATE or something that's tied to
  the frontend instead of the backend since the mapping to CRTC is not
  correct

Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 168 ++++++++----------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 -
 2 files changed, 78 insertions(+), 91 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 d9f8e9c26390..05160c6bbc03 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4719,7 +4719,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 	}
 
 	state->active_planes = cur->active_planes;
-	state->interrupts_enabled = cur->interrupts_enabled;
 	state->vrr_params = cur->vrr_params;
 	state->vrr_infopacket = cur->vrr_infopacket;
 	state->abm_level = cur->abm_level;
@@ -5393,29 +5392,19 @@ static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
 	return num_active;
 }
 
-/*
- * Sets whether interrupts should be enabled on a specific CRTC.
- * We require that the stream be enabled and that there exist active
- * DC planes on the stream.
- */
-static void
-dm_update_crtc_interrupt_state(struct drm_crtc *crtc,
-			       struct drm_crtc_state *new_crtc_state)
+static void dm_update_crtc_active_planes(struct drm_crtc *crtc,
+					 struct drm_crtc_state *new_crtc_state)
 {
 	struct dm_crtc_state *dm_new_crtc_state =
 		to_dm_crtc_state(new_crtc_state);
 
 	dm_new_crtc_state->active_planes = 0;
-	dm_new_crtc_state->interrupts_enabled = false;
 
 	if (!dm_new_crtc_state->stream)
 		return;
 
 	dm_new_crtc_state->active_planes =
 		count_crtc_active_planes(new_crtc_state);
-
-	dm_new_crtc_state->interrupts_enabled =
-		dm_new_crtc_state->active_planes > 0;
 }
 
 static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
@@ -5426,13 +5415,7 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state);
 	int ret = -EINVAL;
 
-	/*
-	 * Update interrupt state for the CRTC. This needs to happen whenever
-	 * the CRTC has changed or whenever any of its planes have changed.
-	 * Atomic check satisfies both of these requirements since the CRTC
-	 * is added to the state by DRM during drm_atomic_helper_check_planes.
-	 */
-	dm_update_crtc_interrupt_state(crtc, state);
+	dm_update_crtc_active_planes(crtc, state);
 
 	if (unlikely(!dm_crtc_state->stream &&
 		     modeset_required(state, NULL, dm_crtc_state->stream))) {
@@ -6547,8 +6530,10 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
 				 bool enable)
 {
 	/*
-	 * this is not correct translation but will work as soon as VBLANK
-	 * constant is the same as PFLIP
+	 * We have no guarantee that the frontend index maps to the same
+	 * backend index - some even map to more than one.
+	 *
+	 * TODO: Use a different interrupt or check DC itself for the mapping.
 	 */
 	int irq_type =
 		amdgpu_display_crtc_idx_to_irq_type(
@@ -6571,6 +6556,19 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
 	}
 }
 
+static void dm_update_pflip_irq_state(struct amdgpu_device *adev,
+				      struct amdgpu_crtc *acrtc)
+{
+	int irq_type =
+		amdgpu_display_crtc_idx_to_irq_type(adev, acrtc->crtc_id);
+
+	/**
+	 * This reads the current state for the IRQ and force reapplies
+	 * the setting to hardware.
+	 */
+	amdgpu_irq_update(adev, &adev->pageflip_irq, irq_type);
+}
+
 static bool
 is_scaling_state_different(const struct dm_connector_state *dm_state,
 			   const struct dm_connector_state *old_dm_state)
@@ -7154,7 +7152,16 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			usleep_range(1000, 1100);
 		}
 
-		if (acrtc_attach->base.state->event) {
+		/**
+		 * Prepare the flip event for the pageflip interrupt to handle.
+		 *
+		 * This only works in the case where we've already turned on the
+		 * appropriate hardware blocks (eg. HUBP) so in the transition case
+		 * from 0 -> n planes we have to skip a hardware generated event
+		 * and rely on sending it from software.
+		 */
+		if (acrtc_attach->base.state->event &&
+		    acrtc_state->active_planes > 0) {
 			drm_crtc_vblank_get(pcrtc);
 
 			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
@@ -7223,6 +7230,24 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 						     &bundle->stream_update,
 						     dc_state);
 
+		/**
+		 * Enable or disable the interrupts on the backend.
+		 *
+		 * Most pipes are put into power gating when unused.
+		 *
+		 * When power gating is enabled on a pipe we lose the
+		 * interrupt enablement state when power gating is disabled.
+		 *
+		 * So we need to update the IRQ control state in hardware
+		 * whenever the pipe turns on (since it could be previously
+		 * power gated) or off (since some pipes can't be power gated
+		 * on some ASICs).
+		 */
+		if (dm_old_crtc_state->active_planes != acrtc_state->active_planes)
+			dm_update_pflip_irq_state(
+				(struct amdgpu_device *)dev->dev_private,
+				acrtc_attach);
+
 		if ((acrtc_state->update_type > UPDATE_TYPE_FAST) &&
 				acrtc_state->stream->link->psr_settings.psr_version != DC_PSR_VERSION_UNSUPPORTED &&
 				!acrtc_state->stream->link->psr_settings.psr_feature_enabled)
@@ -7323,64 +7348,6 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
 	}
 }
 
-/*
- * Enable interrupts on CRTCs that are newly active, undergone
- * a modeset, or have active planes again.
- *
- * Done in two passes, based on the for_modeset flag:
- * Pass 1: For CRTCs going through modeset
- * Pass 2: For CRTCs going from 0 to n active planes
- *
- * Interrupts can only be enabled after the planes are programmed,
- * so this requires a two-pass approach since we don't want to
- * just defer the interrupts until after commit planes every time.
- */
-static void amdgpu_dm_enable_crtc_interrupts(struct drm_device *dev,
-					     struct drm_atomic_state *state,
-					     bool for_modeset)
-{
-	struct amdgpu_device *adev = dev->dev_private;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-	int i;
-#ifdef CONFIG_DEBUG_FS
-	enum amdgpu_dm_pipe_crc_source source;
-#endif
-
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
-				      new_crtc_state, i) {
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-		struct dm_crtc_state *dm_new_crtc_state =
-			to_dm_crtc_state(new_crtc_state);
-		struct dm_crtc_state *dm_old_crtc_state =
-			to_dm_crtc_state(old_crtc_state);
-		bool modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
-		bool run_pass;
-
-		run_pass = (for_modeset && modeset) ||
-			   (!for_modeset && !modeset &&
-			    !dm_old_crtc_state->interrupts_enabled);
-
-		if (!run_pass)
-			continue;
-
-		if (!dm_new_crtc_state->interrupts_enabled)
-			continue;
-
-		manage_dm_interrupts(adev, acrtc, true);
-
-#ifdef CONFIG_DEBUG_FS
-		/* The stream has changed so CRC capture needs to re-enabled. */
-		source = dm_new_crtc_state->crc_src;
-		if (amdgpu_dm_is_valid_crc_source(source)) {
-			amdgpu_dm_crtc_configure_crc_source(
-				crtc, dm_new_crtc_state,
-				dm_new_crtc_state->crc_src);
-		}
-#endif
-	}
-}
-
 /*
  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
  * @crtc_state: the DRM CRTC state
@@ -7420,12 +7387,10 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
 	 * in atomic check.
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		struct dm_crtc_state *dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
-		struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		if (dm_old_crtc_state->interrupts_enabled &&
-		    (!dm_new_crtc_state->interrupts_enabled ||
+		if (old_crtc_state->active &&
+		    (!new_crtc_state->active ||
 		     drm_atomic_crtc_needs_modeset(new_crtc_state)))
 			manage_dm_interrupts(adev, acrtc, false);
 	}
@@ -7704,8 +7669,34 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 						dm_new_crtc_state);
 	}
 
-	/* Enable interrupts for CRTCs going through a modeset. */
-	amdgpu_dm_enable_crtc_interrupts(dev, state, true);
+	/**
+	 * Enable interrupts for CRTCs that are newly enabled or went through
+	 * a modeset. It was intentionally deferred until after the front end
+	 * state was modified to wait until the OTG was on and so the IRQ
+	 * handlers didn't access stale or invalid state.
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+
+		if (new_crtc_state->active &&
+		    (!old_crtc_state->active ||
+		     drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+			manage_dm_interrupts(adev, acrtc, true);
+#ifdef CONFIG_DEBUG_FS
+			/**
+			 * Frontend may have changed so reapply the CRC capture
+			 * settings for the stream.
+			 */
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+
+			if (amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src)) {
+				amdgpu_dm_crtc_configure_crc_source(
+					crtc, dm_new_crtc_state,
+					dm_new_crtc_state->crc_src);
+			}
+#endif
+		}
+	}
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
 		if (new_crtc_state->async_flip)
@@ -7720,9 +7711,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 						dm, crtc, wait_for_vblank);
 	}
 
-	/* Enable interrupts for CRTCs going from 0 to n active planes. */
-	amdgpu_dm_enable_crtc_interrupts(dev, state, false);
-
 	/* Update audio instances for each connector. */
 	amdgpu_dm_commit_audio(dev, state);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 94d3ff727c25..82cdf07b4bd0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -414,7 +414,6 @@ struct dm_crtc_state {
 
 	int update_type;
 	int active_planes;
-	bool interrupts_enabled;
 
 	int crc_skip_count;
 	enum amdgpu_dm_pipe_crc_source crc_src;
-- 
2.25.1

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

Re: [PATCH 1/2] drm/amd/display: Add missing DCN30 registers and fields for OTG_CRC_CNTL2

[Lakha, Bhawanpreet]

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

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
________________________________
From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Sent: July 13, 2020 11:39 AM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>
Subject: [PATCH 1/2] drm/amd/display: Add missing DCN30 registers and fields for OTG_CRC_CNTL2

[Why]
When enabling the debugfs for CRC capture we hit assertions caused by
register address and field masks and shifts missing.

[How]
We want these registers programmed, so add in the SRI/SF entries for
this register.

Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
index d4106dd5a9b0..33f13c1e7520 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
@@ -98,6 +98,7 @@
         SRI(OTG_GSL_WINDOW_Y, OTG, inst),\
         SRI(OTG_VUPDATE_KEEPOUT, OTG, inst),\
         SRI(OTG_DSC_START_POSITION, OTG, inst),\
+       SRI(OTG_CRC_CNTL2, OTG, inst),\
         SRI(OTG_DRR_TRIGGER_WINDOW, OTG, inst),\
         SRI(OTG_DRR_V_TOTAL_CHANGE, OTG, inst),\
         SRI(OPTC_DATA_FORMAT_CONTROL, ODM, inst),\
@@ -248,6 +249,10 @@
         SF(OTG0_OTG_GSL_CONTROL, OTG_MASTER_UPDATE_LOCK_GSL_EN, mask_sh), \
         SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_X, mask_sh), \
         SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_LINE_NUM, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DSC_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_COMBINE_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_SPLIT_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_FORMAT, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG0_SRC_SEL, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG1_SRC_SEL, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_NUM_OF_INPUT_SEGMENT, mask_sh),\
@@ -280,6 +285,10 @@
         SF(OTG0_OTG_GSL_CONTROL, OTG_MASTER_UPDATE_LOCK_GSL_EN, mask_sh), \
         SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_X, mask_sh), \
         SF(OTG0_OTG_DSC_START_POSITION, OTG_DSC_START_POSITION_LINE_NUM, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DSC_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_COMBINE_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_STREAM_SPLIT_MODE, mask_sh),\
+       SF(OTG0_OTG_CRC_CNTL2, OTG_CRC_DATA_FORMAT, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG0_SRC_SEL, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG1_SRC_SEL, mask_sh),\
         SF(ODM0_OPTC_DATA_SOURCE_SELECT, OPTC_SEG2_SRC_SEL, mask_sh),\
--
2.25.1


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

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

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

Re: [PATCH 2/2] drm/amd/display: Allow for vblank enabled with no active planes

[Lakha, Bhawanpreet]

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

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
________________________________
From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Sent: July 13, 2020 11:39 AM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>
Subject: [PATCH 2/2] drm/amd/display: Allow for vblank enabled with no active planes

[Why]
CRC capture doesn't work when the active plane count is 0 since we
currently tie both vblank and pageflip interrupts to active_plane_count
greater than 0.

[How]
The frontend is what generates the vblank interrupts while the backend
is what generates pageflip interrupts. Both have a requirement for
the CRTC to be active, so control the overall interrupt state based
on that instead.

Pageflip interrupts need to be enabled based on active plane count, but
we actually rely on power gating to take care of disabling the interrupt
for us on pipes that can be power gated.

For pipes that can't be power gated it's still fine to leave it enabled
since the interrupt only triggers after the address has been written
to that particular pipe - which we won't be doing without an active
plane.

The issue we had before with this setup was that we couldn't force
the state back on. We were essentially manipulating the refcount
to enable or disable as needed in a two pass approach.

However, there is a function that solves this problem more elegantly:
amdgpu_irq_update() will unconditionally call the set based on what it
thinks the current enablement state is.

This leaves two future TODO items for our IRQ handling:
- Disabling IRQs in commit tail instead of atomic commit
- Mapping the pageflip interrupt to VUPDATE or something that's tied to
  the frontend instead of the backend since the mapping to CRTC is not
  correct

Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 168 ++++++++----------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 -
 2 files changed, 78 insertions(+), 91 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 d9f8e9c26390..05160c6bbc03 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4719,7 +4719,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
         }

         state->active_planes = cur->active_planes;
-       state->interrupts_enabled = cur->interrupts_enabled;
         state->vrr_params = cur->vrr_params;
         state->vrr_infopacket = cur->vrr_infopacket;
         state->abm_level = cur->abm_level;
@@ -5393,29 +5392,19 @@ static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
         return num_active;
 }

-/*
- * Sets whether interrupts should be enabled on a specific CRTC.
- * We require that the stream be enabled and that there exist active
- * DC planes on the stream.
- */
-static void
-dm_update_crtc_interrupt_state(struct drm_crtc *crtc,
-                              struct drm_crtc_state *new_crtc_state)
+static void dm_update_crtc_active_planes(struct drm_crtc *crtc,
+                                        struct drm_crtc_state *new_crtc_state)
 {
         struct dm_crtc_state *dm_new_crtc_state =
                 to_dm_crtc_state(new_crtc_state);

         dm_new_crtc_state->active_planes = 0;
-       dm_new_crtc_state->interrupts_enabled = false;

         if (!dm_new_crtc_state->stream)
                 return;

         dm_new_crtc_state->active_planes =
                 count_crtc_active_planes(new_crtc_state);
-
-       dm_new_crtc_state->interrupts_enabled =
-               dm_new_crtc_state->active_planes > 0;
 }

 static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
@@ -5426,13 +5415,7 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
         struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state);
         int ret = -EINVAL;

-       /*
-        * Update interrupt state for the CRTC. This needs to happen whenever
-        * the CRTC has changed or whenever any of its planes have changed.
-        * Atomic check satisfies both of these requirements since the CRTC
-        * is added to the state by DRM during drm_atomic_helper_check_planes.
-        */
-       dm_update_crtc_interrupt_state(crtc, state);
+       dm_update_crtc_active_planes(crtc, state);

         if (unlikely(!dm_crtc_state->stream &&
                      modeset_required(state, NULL, dm_crtc_state->stream))) {
@@ -6547,8 +6530,10 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
                                  bool enable)
 {
         /*
-        * this is not correct translation but will work as soon as VBLANK
-        * constant is the same as PFLIP
+        * We have no guarantee that the frontend index maps to the same
+        * backend index - some even map to more than one.
+        *
+        * TODO: Use a different interrupt or check DC itself for the mapping.
          */
         int irq_type =
                 amdgpu_display_crtc_idx_to_irq_type(
@@ -6571,6 +6556,19 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
         }
 }

+static void dm_update_pflip_irq_state(struct amdgpu_device *adev,
+                                     struct amdgpu_crtc *acrtc)
+{
+       int irq_type =
+               amdgpu_display_crtc_idx_to_irq_type(adev, acrtc->crtc_id);
+
+       /**
+        * This reads the current state for the IRQ and force reapplies
+        * the setting to hardware.
+        */
+       amdgpu_irq_update(adev, &adev->pageflip_irq, irq_type);
+}
+
 static bool
 is_scaling_state_different(const struct dm_connector_state *dm_state,
                            const struct dm_connector_state *old_dm_state)
@@ -7154,7 +7152,16 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
                         usleep_range(1000, 1100);
                 }

-               if (acrtc_attach->base.state->event) {
+               /**
+                * Prepare the flip event for the pageflip interrupt to handle.
+                *
+                * This only works in the case where we've already turned on the
+                * appropriate hardware blocks (eg. HUBP) so in the transition case
+                * from 0 -> n planes we have to skip a hardware generated event
+                * and rely on sending it from software.
+                */
+               if (acrtc_attach->base.state->event &&
+                   acrtc_state->active_planes > 0) {
                         drm_crtc_vblank_get(pcrtc);

                         spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
@@ -7223,6 +7230,24 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
                                                      &bundle->stream_update,
                                                      dc_state);

+               /**
+                * Enable or disable the interrupts on the backend.
+                *
+                * Most pipes are put into power gating when unused.
+                *
+                * When power gating is enabled on a pipe we lose the
+                * interrupt enablement state when power gating is disabled.
+                *
+                * So we need to update the IRQ control state in hardware
+                * whenever the pipe turns on (since it could be previously
+                * power gated) or off (since some pipes can't be power gated
+                * on some ASICs).
+                */
+               if (dm_old_crtc_state->active_planes != acrtc_state->active_planes)
+                       dm_update_pflip_irq_state(
+                               (struct amdgpu_device *)dev->dev_private,
+                               acrtc_attach);
+
                 if ((acrtc_state->update_type > UPDATE_TYPE_FAST) &&
                                 acrtc_state->stream->link->psr_settings.psr_version != DC_PSR_VERSION_UNSUPPORTED &&
                                 !acrtc_state->stream->link->psr_settings.psr_feature_enabled)
@@ -7323,64 +7348,6 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
         }
 }

-/*
- * Enable interrupts on CRTCs that are newly active, undergone
- * a modeset, or have active planes again.
- *
- * Done in two passes, based on the for_modeset flag:
- * Pass 1: For CRTCs going through modeset
- * Pass 2: For CRTCs going from 0 to n active planes
- *
- * Interrupts can only be enabled after the planes are programmed,
- * so this requires a two-pass approach since we don't want to
- * just defer the interrupts until after commit planes every time.
- */
-static void amdgpu_dm_enable_crtc_interrupts(struct drm_device *dev,
-                                            struct drm_atomic_state *state,
-                                            bool for_modeset)
-{
-       struct amdgpu_device *adev = dev->dev_private;
-       struct drm_crtc *crtc;
-       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-       int i;
-#ifdef CONFIG_DEBUG_FS
-       enum amdgpu_dm_pipe_crc_source source;
-#endif
-
-       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
-                                     new_crtc_state, i) {
-               struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-               struct dm_crtc_state *dm_new_crtc_state =
-                       to_dm_crtc_state(new_crtc_state);
-               struct dm_crtc_state *dm_old_crtc_state =
-                       to_dm_crtc_state(old_crtc_state);
-               bool modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
-               bool run_pass;
-
-               run_pass = (for_modeset && modeset) ||
-                          (!for_modeset && !modeset &&
-                           !dm_old_crtc_state->interrupts_enabled);
-
-               if (!run_pass)
-                       continue;
-
-               if (!dm_new_crtc_state->interrupts_enabled)
-                       continue;
-
-               manage_dm_interrupts(adev, acrtc, true);
-
-#ifdef CONFIG_DEBUG_FS
-               /* The stream has changed so CRC capture needs to re-enabled. */
-               source = dm_new_crtc_state->crc_src;
-               if (amdgpu_dm_is_valid_crc_source(source)) {
-                       amdgpu_dm_crtc_configure_crc_source(
-                               crtc, dm_new_crtc_state,
-                               dm_new_crtc_state->crc_src);
-               }
-#endif
-       }
-}
-
 /*
  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
  * @crtc_state: the DRM CRTC state
@@ -7420,12 +7387,10 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
          * in atomic check.
          */
         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-               struct dm_crtc_state *dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
-               struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
                 struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);

-               if (dm_old_crtc_state->interrupts_enabled &&
-                   (!dm_new_crtc_state->interrupts_enabled ||
+               if (old_crtc_state->active &&
+                   (!new_crtc_state->active ||
                      drm_atomic_crtc_needs_modeset(new_crtc_state)))
                         manage_dm_interrupts(adev, acrtc, false);
         }
@@ -7704,8 +7669,34 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
                                                 dm_new_crtc_state);
         }

-       /* Enable interrupts for CRTCs going through a modeset. */
-       amdgpu_dm_enable_crtc_interrupts(dev, state, true);
+       /**
+        * Enable interrupts for CRTCs that are newly enabled or went through
+        * a modeset. It was intentionally deferred until after the front end
+        * state was modified to wait until the OTG was on and so the IRQ
+        * handlers didn't access stale or invalid state.
+        */
+       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+               struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+
+               if (new_crtc_state->active &&
+                   (!old_crtc_state->active ||
+                    drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+                       manage_dm_interrupts(adev, acrtc, true);
+#ifdef CONFIG_DEBUG_FS
+                       /**
+                        * Frontend may have changed so reapply the CRC capture
+                        * settings for the stream.
+                        */
+                       dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+
+                       if (amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src)) {
+                               amdgpu_dm_crtc_configure_crc_source(
+                                       crtc, dm_new_crtc_state,
+                                       dm_new_crtc_state->crc_src);
+                       }
+#endif
+               }
+       }

         for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
                 if (new_crtc_state->async_flip)
@@ -7720,9 +7711,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
                                                 dm, crtc, wait_for_vblank);
         }

-       /* Enable interrupts for CRTCs going from 0 to n active planes. */
-       amdgpu_dm_enable_crtc_interrupts(dev, state, false);
-
         /* Update audio instances for each connector. */
         amdgpu_dm_commit_audio(dev, state);

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 94d3ff727c25..82cdf07b4bd0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -414,7 +414,6 @@ struct dm_crtc_state {

         int update_type;
         int active_planes;
-       bool interrupts_enabled;

         int crc_skip_count;
         enum amdgpu_dm_pipe_crc_source crc_src;
--
2.25.1


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

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

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