drm/amd/display: Refactor CRTC interrupt toggling logic

[Why]
The vblank and pageflip interrupts should only be enabled for a CRTC
that's enabled and has active planes.

The current logic takes care of this, but isn't setup to handle the case
where the active plane count goes to zero but the stream remains
enabled.

We currently block this case since we don't allow commits that enable a
CRTC with no active planes, but shouldn't be any reason we can't support
this from a hardware perspective and many userspace applications expect
to be able to do it (like IGT).

[How]
The count_crtc_active_planes function fills in the number of
"active_planes" on the dm_crtc_state. This should be the same as
DC's plane_count on the stream_status but easier to access since we
don't need to lock the private atomic state with the DC context.

Add the "interrupts_enabled" flag to the dm_crtc_state and set it based
on whether the stream exists and if there are active planes on the
stream.

Update the disable and enable logic to make use of this new flag.

There shouldn't be any functional change (yet) with this patch.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Reviewed-by: David Francis <David.Francis@amd.com>
Acked-by: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

2 files changed, 68 insertions, 21 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 a3dc7ab523de..c82048cb901d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3438,6 +3438,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 		dc_stream_retain(state->stream);
 	}
 
+	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;
@@ -3862,7 +3864,7 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc)
 {
 }
 
-static bool does_crtc_have_active_plane(struct drm_crtc_state *new_crtc_state)
+static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
 {
 	struct drm_atomic_state *state = new_crtc_state->state;
 	struct drm_plane *plane;
@@ -3891,7 +3893,32 @@ static bool does_crtc_have_active_plane(struct drm_crtc_state *new_crtc_state)
 		num_active += (new_plane_state->fb != NULL);
 	}
 
-	return num_active > 0;
+	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)
+{
+	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,
@@ -3902,6 +3929,14 @@ 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);
+
 	if (unlikely(!dm_crtc_state->stream &&
 		     modeset_required(state, NULL, dm_crtc_state->stream))) {
 		WARN_ON(1);
@@ -3914,7 +3949,7 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 
 	/* We want at least one hardware plane enabled to use the stream. */
 	if (state->enable && state->active &&
-	    !does_crtc_have_active_plane(state))
+	    dm_crtc_state->active_planes == 0)
 		return -EINVAL;
 
 	if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK)
@@ -5390,24 +5425,33 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
 	int i;
 
 	/*
-	 * We evade vblanks and pflips on crtc that
-	 * should be changed. We do it here to flush & disable
-	 * interrupts before drm_swap_state is called in drm_atomic_helper_commit
-	 * it will update crtc->dm_crtc_state->stream pointer which is used in
-	 * the ISRs.
+	 * We evade vblank and pflip interrupts on CRTCs that are undergoing
+	 * a modeset, being disabled, or have no active planes.
+	 *
+	 * It's done in atomic commit rather than commit tail for now since
+	 * some of these interrupt handlers access the current CRTC state and
+	 * potentially the stream pointer itself.
+	 *
+	 * Since the atomic state is swapped within atomic commit and not within
+	 * commit tail this would leave to new state (that hasn't been committed yet)
+	 * being accesssed from within the handlers.
+	 *
+	 * TODO: Fix this so we can do this in commit tail and not have to block
+	 * 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 (drm_atomic_crtc_needs_modeset(new_crtc_state)
-		    && dm_old_crtc_state->stream) {
+		if (dm_old_crtc_state->interrupts_enabled &&
+		    (!dm_new_crtc_state->interrupts_enabled ||
+		     drm_atomic_crtc_needs_modeset(new_crtc_state))) {
 			/*
 			 * If the stream is removed and CRC capture was
 			 * enabled on the CRTC the extra vblank reference
-			 * needs to be dropped since CRC capture will be
-			 * disabled.
+			 * needs to be dropped since CRC capture will not
+			 * be re-enabled.
 			 */
 			if (!dm_new_crtc_state->stream
 			    && dm_new_crtc_state->crc_enabled) {
@@ -5635,13 +5679,14 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		pre_update_freesync_state_on_stream(dm, dm_new_crtc_state);
 	}
 
+	/*
+	 * Enable interrupts on CRTCs that are newly active, undergone
+	 * a modeset, or have active planes again.
+	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 			new_crtc_state, i) {
-		/*
-		 * loop to enable interrupts on newly arrived crtc
-		 */
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-		bool modeset_needed;
+		bool enable;
 
 		if (old_crtc_state->active && !new_crtc_state->active)
 			crtc_disable_count++;
@@ -5653,12 +5698,11 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
 						dm_new_crtc_state);
 
-		modeset_needed = modeset_required(
-				new_crtc_state,
-				dm_new_crtc_state->stream,
-				dm_old_crtc_state->stream);
+		enable = dm_new_crtc_state->interrupts_enabled &&
+			 (!dm_old_crtc_state->interrupts_enabled ||
+			  drm_atomic_crtc_needs_modeset(new_crtc_state));
 
-		if (dm_new_crtc_state->stream == NULL || !modeset_needed)
+		if (!enable)
 			continue;
 
 		manage_dm_interrupts(adev, acrtc, true);
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 3a0b6164c755..978ff14a7d45 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -271,6 +271,9 @@ struct dm_crtc_state {
 	struct drm_crtc_state base;
 	struct dc_stream_state *stream;
 
+	int active_planes;
+	bool interrupts_enabled;
+
 	int crc_skip_count;
 	bool crc_enabled;