[PATCH v4 00/13] PSR DSB support

[PATCH v4 00/13] PSR DSB support

[Jouni Högander]

This patch set is doing necessary modifications to support PSR update
using DSB on LunarLake onwards

It is not necessary to wait for PSR1 to idle or PSR2 to exit DEEP
sleep at the begin of commit This is left out from DSB commit. There
might be room for optimization for non-DSB as well because such wait
is not supposed to be necessary at the begin of update.

PSR mutex is not locked when performing DSB commit. It is not
necessary as we are currently using DSB only when sending updates
towards panel. I.e. not using it when changing PSR mode. Also
necessary changes are made to use PSR2_MAN_TRK_CTL only in
DSB. Frontbuffer updates and legacy cursor updates are using SFF_CTL
register to perform full frame updates.

DSB_SKIP_WAITS_EN is removed to ensure all waits are performed when
PSR is active. PSR "Frame Change" event is manually triggered at the
begin of each DSB commit by adding CURSURFLIVE register write.

Possibe problem with DSB commit when PSR is already waking up is
avoided by evadding scanline 0.

v4:
  - remove DSB_SKIP_WAITS_EN
  - Add CURSURFLIVE register write at the begin of DSB commit
  - evade scanline 0
v3:
  - do not use DSB when PSR mode is changing
v2:
  - use _MMIO_TRANS instead of _MMIO_TRANS2
  - drop evasion from intel_psr_configure_full_frame_update

Jouni Högander (13):
  drm/i915/psr: Use PSR2_MAN_TRK_CTL CFF bit only to send full update
  drm/i915/psr: Rename psr_force_hw_tracking_exit as
    intel_psr_force_update
  drm/i915/psr: Split setting sff and cff bits away from
    intel_psr_force_update
  drm/i915/psr: Add register definitions for SFF_CTL and CFF_CTL
    registers
  drm/i915/psr: Use SFF_CTL on invalidate/flush for LunarLake onwards
  drm/i915/psr: Allow writing PSR2_MAN_TRK_CTL using DSB
  drm/i915/psr: Changes for PSR2_MAN_TRK_CTL handling when DSB is in use
  drm/i915/psr: Add intel_psr_is_psr_mode_changing
  drm/i915/display: Don't use DSB if psr mode changing
  drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit
  drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled
  drm/i915/display: Ensure we have "Frame Change" event in DSB commit
  drm/i915/psr: Allow DSB usage when PSR is enabled

 drivers/gpu/drm/i915/display/intel_display.c  |  20 ++-
 drivers/gpu/drm/i915/display/intel_dsb.c      |  17 ++-
 drivers/gpu/drm/i915/display/intel_psr.c      | 139 +++++++++---------
 drivers/gpu/drm/i915/display/intel_psr.h      |   6 +-
 drivers/gpu/drm/i915/display/intel_psr_regs.h |  10 ++
 5 files changed, 118 insertions(+), 74 deletions(-)

-- 
2.43.0

[PATCH v4 01/13] drm/i915/psr: Use PSR2_MAN_TRK_CTL CFF bit only to send full update

[Jouni Högander]

We are preparing for a change where only frontbuffer flush will use
single full frame bit of a new register (SFF_CTL) available on LunarLake
onwards.

It shouldn't be necessary to have SFF bit set if CFF bit is set in
PSR2_MAN_TRK_CTL -> removing setting it on all platforms as there is not
reason to have it different on older platforms.

v2: commit message improved

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 0b021acb330f4..476305010e113 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2381,7 +2381,6 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
 	val |= man_trk_ctl_partial_frame_bit_get(display);
 
 	if (full_update) {
-		val |= man_trk_ctl_single_full_frame_bit_get(display);
 		val |= man_trk_ctl_continuos_full_frame(display);
 		goto exit;
 	}
-- 
2.43.0

[PATCH v4 02/13] drm/i915/psr: Rename psr_force_hw_tracking_exit as intel_psr_force_update

[Jouni Högander]

psr_force_hw_tracking_exit is misleading name as it is used for PSR1, PSR2
HW tracking and PSR2 selective fetch. Due to this rename it as
intel_psr_force_update.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 476305010e113..5411d0d6f362a 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2310,7 +2310,7 @@ static u32 man_trk_ctl_continuos_full_frame(struct intel_display *display)
 	       PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME;
 }
 
-static void psr_force_hw_tracking_exit(struct intel_dp *intel_dp)
+static void intel_psr_force_update(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
@@ -2857,7 +2857,7 @@ void intel_psr_post_plane_update(struct intel_atomic_state *state,
 
 		/* Force a PSR exit when enabling CRC to avoid CRC timeouts */
 		if (crtc_state->crc_enabled && psr->enabled)
-			psr_force_hw_tracking_exit(intel_dp);
+			intel_psr_force_update(intel_dp);
 
 		/*
 		 * Clear possible busy bits in case we have
@@ -3254,10 +3254,10 @@ static void _psr_flush_handle(struct intel_dp *intel_dp)
 			 * continuous full frame is disabled, only a single full
 			 * frame is required
 			 */
-			psr_force_hw_tracking_exit(intel_dp);
+			intel_psr_force_update(intel_dp);
 		}
 	} else {
-		psr_force_hw_tracking_exit(intel_dp);
+		intel_psr_force_update(intel_dp);
 
 		if (!intel_dp->psr.active && !intel_dp->psr.busy_frontbuffer_bits)
 			queue_work(dev_priv->unordered_wq, &intel_dp->psr.work);
-- 
2.43.0

[PATCH v4 03/13] drm/i915/psr: Split setting sff and cff bits away from intel_psr_force_update

[Jouni Högander]

This is a clean-up and a preparation for adding own SFF and CFF registers
for LunarLake onwards.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 88 +++++++++---------------
 1 file changed, 31 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 5411d0d6f362a..e6f96a8b4fb0d 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2313,15 +2313,6 @@ static u32 man_trk_ctl_continuos_full_frame(struct intel_display *display)
 static void intel_psr_force_update(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
-	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
-
-	if (intel_dp->psr.psr2_sel_fetch_enabled)
-		intel_de_write(display,
-			       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
-			       man_trk_ctl_enable_bit_get(display) |
-			       man_trk_ctl_partial_frame_bit_get(display) |
-			       man_trk_ctl_single_full_frame_bit_get(display) |
-			       man_trk_ctl_continuos_full_frame(display));
 
 	/*
 	 * Display WA #0884: skl+
@@ -3119,31 +3110,31 @@ static void intel_psr_work(struct work_struct *work)
 	mutex_unlock(&intel_dp->psr.lock);
 }
 
-static void _psr_invalidate_handle(struct intel_dp *intel_dp)
+static void intel_psr_configure_full_frame_update(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
 
-	if (intel_dp->psr.psr2_sel_fetch_enabled) {
-		u32 val;
+	if (!intel_dp->psr.psr2_sel_fetch_enabled)
+		return;
 
-		if (intel_dp->psr.psr2_sel_fetch_cff_enabled) {
-			/* Send one update otherwise lag is observed in screen */
-			intel_de_write(display,
-				       CURSURFLIVE(display, intel_dp->psr.pipe),
-				       0);
-			return;
+	intel_de_write(display,
+		       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
+		       man_trk_ctl_enable_bit_get(display) |
+		       man_trk_ctl_partial_frame_bit_get(display) |
+		       man_trk_ctl_single_full_frame_bit_get(display) |
+		       man_trk_ctl_continuos_full_frame(display));
+}
+
+static void _psr_invalidate_handle(struct intel_dp *intel_dp)
+{
+	if (intel_dp->psr.psr2_sel_fetch_enabled) {
+		if (!intel_dp->psr.psr2_sel_fetch_cff_enabled) {
+			intel_dp->psr.psr2_sel_fetch_cff_enabled = true;
+			intel_psr_configure_full_frame_update(intel_dp);
 		}
 
-		val = man_trk_ctl_enable_bit_get(display) |
-		      man_trk_ctl_partial_frame_bit_get(display) |
-		      man_trk_ctl_continuos_full_frame(display);
-		intel_de_write(display,
-			       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
-			       val);
-		intel_de_write(display,
-			       CURSURFLIVE(display, intel_dp->psr.pipe), 0);
-		intel_dp->psr.psr2_sel_fetch_cff_enabled = true;
+		intel_psr_force_update(intel_dp);
 	} else {
 		intel_psr_exit(intel_dp);
 	}
@@ -3224,44 +3215,27 @@ static void _psr_flush_handle(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(display->drm);
-	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
 
 	if (intel_dp->psr.psr2_sel_fetch_enabled) {
 		if (intel_dp->psr.psr2_sel_fetch_cff_enabled) {
 			/* can we turn CFF off? */
-			if (intel_dp->psr.busy_frontbuffer_bits == 0) {
-				u32 val = man_trk_ctl_enable_bit_get(display) |
-					man_trk_ctl_partial_frame_bit_get(display) |
-					man_trk_ctl_single_full_frame_bit_get(display) |
-					man_trk_ctl_continuos_full_frame(display);
-
-				/*
-				 * Set psr2_sel_fetch_cff_enabled as false to allow selective
-				 * updates. Still keep cff bit enabled as we don't have proper
-				 * SU configuration in case update is sent for any reason after
-				 * sff bit gets cleared by the HW on next vblank.
-				 */
-				intel_de_write(display,
-					       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
-					       val);
-				intel_de_write(display,
-					       CURSURFLIVE(display, intel_dp->psr.pipe),
-					       0);
+			if (intel_dp->psr.busy_frontbuffer_bits == 0)
 				intel_dp->psr.psr2_sel_fetch_cff_enabled = false;
-			}
-		} else {
-			/*
-			 * continuous full frame is disabled, only a single full
-			 * frame is required
-			 */
-			intel_psr_force_update(intel_dp);
 		}
-	} else {
-		intel_psr_force_update(intel_dp);
 
-		if (!intel_dp->psr.active && !intel_dp->psr.busy_frontbuffer_bits)
-			queue_work(dev_priv->unordered_wq, &intel_dp->psr.work);
+		/*
+		 * Still keep cff bit enabled as we don't have proper SU
+		 * configuration in case update is sent for any reason after
+		 * sff bit gets cleared by the HW on next vblank.
+		 */
+		intel_psr_configure_full_frame_update(intel_dp);
 	}
+
+	intel_psr_force_update(intel_dp);
+
+	if (!intel_dp->psr.psr2_sel_fetch_enabled && !intel_dp->psr.active &&
+	    !intel_dp->psr.busy_frontbuffer_bits)
+		queue_work(dev_priv->unordered_wq, &intel_dp->psr.work);
 }
 
 /**
-- 
2.43.0

[PATCH v4 04/13] drm/i915/psr: Add register definitions for SFF_CTL and CFF_CTL registers

[Jouni Högander]

Add register definitions for SFF_CTL and CFF_CTL registers. Name them as
LNL_SFF_CTL and LNL_CFF_CTL.

v2: use _MMIO_TRANS instead of _MMIO_TRANS2

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr_regs.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h b/drivers/gpu/drm/i915/display/intel_psr_regs.h
index 9ad7611506e88..795e6b9cc575c 100644
--- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
@@ -251,6 +251,16 @@
 #define  ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME		REG_BIT(14)
 #define  ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME		REG_BIT(13)
 
+#define _LNL_SFF_CTL_A				0x60918
+#define _LNL_SFF_CTL_B				0x61918
+#define LNL_SFF_CTL(tran)			_MMIO_TRANS(tran, _LNL_SFF_CTL_A, _LNL_SFF_CTL_B)
+#define  LNL_SFF_CTL_SF_SINGLE_FULL_FRAME	REG_BIT(1)
+
+#define _LNL_CFF_CTL_A				0x6091c
+#define _LNL_CFF_CTL_B				0x6191c
+#define LNL_CFF_CTL(tran)			_MMIO_TRANS(tran, _LNL_CFF_CTL_A, _LNL_CFF_CTL_B)
+#define  LNL_CFF_CTL_SF_CONTINUOUS_FULL_FRAME	REG_BIT(1)
+
 /* PSR2 Early transport */
 #define _PIPE_SRCSZ_ERLY_TPT_A	0x70074
 #define _PIPE_SRCSZ_ERLY_TPT_B	0x71074
-- 
2.43.0

[PATCH v4 05/13] drm/i915/psr: Use SFF_CTL on invalidate/flush for LunarLake onwards

[Jouni Högander]

In LunarLake we have SFF_CTL register which contains SFF bit ored with
respective SFF bit in PSR2_MAN_TRK_CTL register. Use this register instead
of the bit in PSR2_MAN_TRK_CTL on frontbuffer tracking callbacks. This
helps us avoiding taking psr mutex when performing atomic commit.

We don't need to set the CFF bit as selective update configuration in
PSR2_MAN_TRL_CTL is not overwritten anymore. I.e. we have valid
configuration in PSR2_MAN_TRK_CTL and in plane SEL_FETCH_* registers when
SFF bit gets cleared by the HW in case something triggers "frame change"
event after SFF bit is cleared.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index e6f96a8b4fb0d..85ecedd3162d8 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2345,7 +2345,7 @@ void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_st
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
 		lockdep_assert_held(&intel_dp->psr.lock);
-		if (intel_dp->psr.psr2_sel_fetch_cff_enabled)
+		if (DISPLAY_VER(display) < 20 && intel_dp->psr.psr2_sel_fetch_cff_enabled)
 			return;
 		break;
 	}
@@ -3118,12 +3118,16 @@ static void intel_psr_configure_full_frame_update(struct intel_dp *intel_dp)
 	if (!intel_dp->psr.psr2_sel_fetch_enabled)
 		return;
 
-	intel_de_write(display,
-		       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
-		       man_trk_ctl_enable_bit_get(display) |
-		       man_trk_ctl_partial_frame_bit_get(display) |
-		       man_trk_ctl_single_full_frame_bit_get(display) |
-		       man_trk_ctl_continuos_full_frame(display));
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_write(display, LNL_SFF_CTL(cpu_transcoder),
+			       LNL_SFF_CTL_SF_SINGLE_FULL_FRAME);
+	else
+		intel_de_write(display,
+			       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
+			       man_trk_ctl_enable_bit_get(display) |
+			       man_trk_ctl_partial_frame_bit_get(display) |
+			       man_trk_ctl_single_full_frame_bit_get(display) |
+			       man_trk_ctl_continuos_full_frame(display));
 }
 
 static void _psr_invalidate_handle(struct intel_dp *intel_dp)
@@ -3227,6 +3231,10 @@ static void _psr_flush_handle(struct intel_dp *intel_dp)
 		 * Still keep cff bit enabled as we don't have proper SU
 		 * configuration in case update is sent for any reason after
 		 * sff bit gets cleared by the HW on next vblank.
+		 *
+		 * NOTE: Setting cff bit is not needed for LunarLake onwards as
+		 * we have own register for SFF bit and we are not overwriting
+		 * existing SU configuration
 		 */
 		intel_psr_configure_full_frame_update(intel_dp);
 	}
-- 
2.43.0

[PATCH v4 06/13] drm/i915/psr: Allow writing PSR2_MAN_TRK_CTL using DSB

[Jouni Högander]

Allow writing PSR2_MAN_TRK_CTL using DSB by using intel_de_write_dsb. Do
not check intel_dp->psr.lock being held when using DSB. This assertion
doesn't make sense as in case of using DSB the actual write happens later
and we are not taking intel_dp->psr.lock mutex over dsb commit.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  2 +-
 drivers/gpu/drm/i915/display/intel_psr.c     | 16 ++++++++++------
 drivers/gpu/drm/i915/display/intel_psr.h     |  4 +++-
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f5d2eacce119b..d5e52bb884797 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7124,7 +7124,7 @@ static void commit_pipe_pre_planes(struct intel_atomic_state *state,
 			intel_pipe_fastset(old_crtc_state, new_crtc_state);
 	}
 
-	intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
+	intel_psr2_program_trans_man_trk_ctl(NULL, new_crtc_state);
 
 	intel_atomic_update_watermarks(state, crtc);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 85ecedd3162d8..1e99329b70a1e 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2330,7 +2330,8 @@ static void intel_psr_force_update(struct intel_dp *intel_dp)
 	intel_de_write(display, CURSURFLIVE(display, intel_dp->psr.pipe), 0);
 }
 
-void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state)
+void intel_psr2_program_trans_man_trk_ctl(struct intel_dsb *dsb,
+					  const struct intel_crtc_state *crtc_state)
 {
 	struct intel_display *display = to_intel_display(crtc_state);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -2344,20 +2345,23 @@ void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_st
 					     crtc_state->uapi.encoder_mask) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
-		lockdep_assert_held(&intel_dp->psr.lock);
+		if (!dsb)
+			lockdep_assert_held(&intel_dp->psr.lock);
+
 		if (DISPLAY_VER(display) < 20 && intel_dp->psr.psr2_sel_fetch_cff_enabled)
 			return;
 		break;
 	}
 
-	intel_de_write(display, PSR2_MAN_TRK_CTL(display, cpu_transcoder),
-		       crtc_state->psr2_man_track_ctl);
+	intel_de_write_dsb(display, dsb,
+			   PSR2_MAN_TRK_CTL(display, cpu_transcoder),
+			   crtc_state->psr2_man_track_ctl);
 
 	if (!crtc_state->enable_psr2_su_region_et)
 		return;
 
-	intel_de_write(display, PIPE_SRCSZ_ERLY_TPT(crtc->pipe),
-		       crtc_state->pipe_srcsz_early_tpt);
+	intel_de_write_dsb(display, dsb, PIPE_SRCSZ_ERLY_TPT(crtc->pipe),
+			   crtc_state->pipe_srcsz_early_tpt);
 }
 
 static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index 956be263c09e3..fc807817863e4 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -17,6 +17,7 @@ struct intel_crtc;
 struct intel_crtc_state;
 struct intel_display;
 struct intel_dp;
+struct intel_dsb;
 struct intel_encoder;
 struct intel_plane;
 struct intel_plane_state;
@@ -55,7 +56,8 @@ void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_stat
 bool intel_psr_enabled(struct intel_dp *intel_dp);
 int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 				struct intel_crtc *crtc);
-void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state);
+void intel_psr2_program_trans_man_trk_ctl(struct intel_dsb *dsb,
+					  const struct intel_crtc_state *crtc_state);
 void intel_psr_pause(struct intel_dp *intel_dp);
 void intel_psr_resume(struct intel_dp *intel_dp);
 bool intel_psr_needs_block_dc_vblank(const struct intel_crtc_state *crtc_state);
-- 
2.43.0

[PATCH v4 07/13] drm/i915/psr: Changes for PSR2_MAN_TRK_CTL handling when DSB is in use

[Jouni Högander]

Do needed changes to handle PSR2_MAN_TRK_CTL correctly when DSB is in use:

1. Write PSR2_MAN_TRK_CTL in commit_pipe_pre_planes only when not using
   DSB.
2. Add PSR2_MAN_TRK_CTL writing into DSB commit in
   intel_atomic_dsb_finish.

Taking PSR lock over DSB commit is not needed because PSR2_MAN_TRK_CTL is
now written only by DSB.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index d5e52bb884797..3ac1cc9ac08a8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7124,7 +7124,8 @@ static void commit_pipe_pre_planes(struct intel_atomic_state *state,
 			intel_pipe_fastset(old_crtc_state, new_crtc_state);
 	}
 
-	intel_psr2_program_trans_man_trk_ctl(NULL, new_crtc_state);
+	if (!new_crtc_state->use_dsb)
+		intel_psr2_program_trans_man_trk_ctl(NULL, new_crtc_state);
 
 	intel_atomic_update_watermarks(state, crtc);
 }
@@ -7713,6 +7714,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 					       new_crtc_state);
 		bdw_set_pipe_misc(new_crtc_state->dsb_commit,
 				  new_crtc_state);
+		intel_psr2_program_trans_man_trk_ctl(new_crtc_state->dsb_commit,
+						     new_crtc_state);
 		intel_crtc_planes_update_arm(new_crtc_state->dsb_commit,
 					     state, crtc);
 
-- 
2.43.0

[PATCH v4 08/13] drm/i915/psr: Add intel_psr_is_psr_mode_changing

[Jouni Högander]

Add new interface for checking possible PSR/PR mode change. We need this
information to decide if DSB can be used.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_psr.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 1e99329b70a1e..90e36e34e0c7b 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -225,6 +225,26 @@ bool intel_psr_needs_aux_io_power(struct intel_encoder *encoder,
 	       intel_encoder_can_psr(encoder);
 }
 
+/**
+ * intel_psr_is_psr_mode_changing - Check if PSR/PR mode is changing
+ * @old_crtc_state: pointer to old intel_crtc_state
+ * @new_crtc_state: pointer to new intel_crtc_state
+ *
+ * This can be used to figure out if PSR/PR mode is changing between old and new
+ * crtc state.
+ *
+ * Returns true if mode is changing, false if mode is not changing.
+ */
+bool intel_psr_is_psr_mode_changing(const struct intel_crtc_state *old_crtc_state,
+				    const struct intel_crtc_state *new_crtc_state)
+{
+	return old_crtc_state->has_psr != new_crtc_state->has_psr ||
+		old_crtc_state->has_sel_update != new_crtc_state->has_sel_update ||
+		old_crtc_state->has_panel_replay != new_crtc_state->has_panel_replay ||
+		old_crtc_state->enable_psr2_su_region_et !=
+		new_crtc_state->enable_psr2_su_region_et;
+}
+
 static bool psr_global_enabled(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index fc807817863e4..cc6267e87933d 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -26,6 +26,8 @@ struct intel_plane_state;
 				    (intel_dp)->psr.source_panel_replay_support)
 
 bool intel_encoder_can_psr(struct intel_encoder *encoder);
+bool intel_psr_is_psr_mode_changing(const struct intel_crtc_state *old_crtc_state,
+				    const struct intel_crtc_state *new_crtc_state);
 bool intel_psr_needs_aux_io_power(struct intel_encoder *encoder,
 				  const struct intel_crtc_state *crtc_state);
 void intel_psr_init_dpcd(struct intel_dp *intel_dp);
-- 
2.43.0

[PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Jouni Högander]

Changing PSR mode using DSB is not implemented. Do not use DSB when PSR
mode is changing.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3ac1cc9ac08a8..a189aa437d972 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		!new_crtc_state->scaler_state.scaler_users &&
 		!old_crtc_state->scaler_state.scaler_users &&
 		!intel_crtc_needs_modeset(new_crtc_state) &&
-		!intel_crtc_needs_fastset(new_crtc_state);
+		!intel_crtc_needs_fastset(new_crtc_state) &&
+		!intel_psr_is_psr_mode_changing(old_crtc_state, new_crtc_state);
 
 	if (!new_crtc_state->use_dsb && !new_crtc_state->dsb_color_vblank)
 		return;
-- 
2.43.0

Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> Changing PSR mode using DSB is not implemented. Do not use DSB when PSR
> mode is changing.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3ac1cc9ac08a8..a189aa437d972 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  		!new_crtc_state->scaler_state.scaler_users &&
>  		!old_crtc_state->scaler_state.scaler_users &&
>  		!intel_crtc_needs_modeset(new_crtc_state) &&
> -		!intel_crtc_needs_fastset(new_crtc_state);
> +		!intel_crtc_needs_fastset(new_crtc_state) &&
> +		!intel_psr_is_psr_mode_changing(old_crtc_state, new_crtc_state);

Hmm. Doesn't all that imply a fastset anyway, and/or maybe all the
PSR stuff happens in the {pre,post}_plane_update() stuff? In which
case it shouldn't really matter for the stuff that the DSB does.

>  
>  	if (!new_crtc_state->use_dsb && !new_crtc_state->dsb_color_vblank)
>  		return;
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Hogander, Jouni]

On Fri, 2025-01-24 at 13:53 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> > Changing PSR mode using DSB is not implemented. Do not use DSB when
> > PSR
> > mode is changing.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 3ac1cc9ac08a8..a189aa437d972 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct
> > intel_atomic_state *state,
> >  		!new_crtc_state->scaler_state.scaler_users &&
> >  		!old_crtc_state->scaler_state.scaler_users &&
> >  		!intel_crtc_needs_modeset(new_crtc_state) &&
> > -		!intel_crtc_needs_fastset(new_crtc_state);
> > +		!intel_crtc_needs_fastset(new_crtc_state) &&
> > +		!intel_psr_is_psr_mode_changing(old_crtc_state,
> > new_crtc_state);
> 
> Hmm. Doesn't all that imply a fastset anyway

PSR/PR doesn't imply fastset.

At the time of enabling PSR/PR crtc_state->has_psr is true at this
point, but PSR is not yet enabled. It gets enabled only in 
pre_plane_update. Also in hsw_activate_psr2 and
dg2_panel_replay_activate we are writing PSR2_MAN_TRK_CTL.

BR,

Jouni Högander

> , and/or maybe all the
> PSR stuff happens in the {pre,post}_plane_update() stuff? In which
> case it shouldn't really matter for the stuff that the DSB does.
> 
> >  
> >  	if (!new_crtc_state->use_dsb && !new_crtc_state-
> > >dsb_color_vblank)
> >  		return;
> > -- 
> > 2.43.0
> 

Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Hogander, Jouni]

On Fri, 2025-01-24 at 14:09 +0200, Hogander, Jouni wrote:
> On Fri, 2025-01-24 at 13:53 +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> > > Changing PSR mode using DSB is not implemented. Do not use DSB
> > > when
> > > PSR
> > > mode is changing.
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 3ac1cc9ac08a8..a189aa437d972 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct
> > > intel_atomic_state *state,
> > >  		!new_crtc_state->scaler_state.scaler_users &&
> > >  		!old_crtc_state->scaler_state.scaler_users &&
> > >  		!intel_crtc_needs_modeset(new_crtc_state) &&
> > > -		!intel_crtc_needs_fastset(new_crtc_state);
> > > +		!intel_crtc_needs_fastset(new_crtc_state) &&
> > > +		!intel_psr_is_psr_mode_changing(old_crtc_state,
> > > new_crtc_state);
> > 
> > Hmm. Doesn't all that imply a fastset anyway
> 
> PSR/PR doesn't imply fastset.
> 
> At the time of enabling PSR/PR crtc_state->has_psr is true at this
> point, but PSR is not yet enabled. It gets enabled only in 
> pre_plane_update. Also in hsw_activate_psr2 and
> dg2_panel_replay_activate we are writing PSR2_MAN_TRK_CTL.

I said it wrong here: post_plane_update I mean of course.

BR,

Jouni Högander

> 
> BR,
> 
> Jouni Högander
> 
> > , and/or maybe all the
> > PSR stuff happens in the {pre,post}_plane_update() stuff? In which
> > case it shouldn't really matter for the stuff that the DSB does.
> > 
> > >  
> > >  	if (!new_crtc_state->use_dsb && !new_crtc_state-
> > > > dsb_color_vblank)
> > >  		return;
> > > -- 
> > > 2.43.0
> > 
> 

Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:16:56PM +0000, Hogander, Jouni wrote:
> On Fri, 2025-01-24 at 14:09 +0200, Hogander, Jouni wrote:
> > On Fri, 2025-01-24 at 13:53 +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> > > > Changing PSR mode using DSB is not implemented. Do not use DSB
> > > > when
> > > > PSR
> > > > mode is changing.
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 3ac1cc9ac08a8..a189aa437d972 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct
> > > > intel_atomic_state *state,
> > > >  		!new_crtc_state->scaler_state.scaler_users &&
> > > >  		!old_crtc_state->scaler_state.scaler_users &&
> > > >  		!intel_crtc_needs_modeset(new_crtc_state) &&
> > > > -		!intel_crtc_needs_fastset(new_crtc_state);
> > > > +		!intel_crtc_needs_fastset(new_crtc_state) &&
> > > > +		!intel_psr_is_psr_mode_changing(old_crtc_state,
> > > > new_crtc_state);
> > > 
> > > Hmm. Doesn't all that imply a fastset anyway
> > 
> > PSR/PR doesn't imply fastset.

You seem to be checking has_psr, has_sel_update, has_panel_replay,
and enable_psr2_su_region_et, and all of those seem to come from 
from intel_psr_compute_config() which is only called from the
modeset path. Which means it's either going to end up as a full
modeset or fastset.

> > 
> > At the time of enabling PSR/PR crtc_state->has_psr is true at this
> > point, but PSR is not yet enabled. It gets enabled only in 
> > pre_plane_update. Also in hsw_activate_psr2 and
> > dg2_panel_replay_activate we are writing PSR2_MAN_TRK_CTL.
> 
> I said it wrong here: post_plane_update I mean of course.

We don't really care whether PSR is actually active or not.
All we care about is whether it might be active. Or I suppose
technically we wouldn't even have to care about that if we just
always blasted in the extra DSB commands, but since it's easy to
avoid the extra overhead when PSR is not even possible then I
suppose it might be worthwile to check for that.

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

[Hogander, Jouni]

On Fri, 2025-01-24 at 14:32 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:16:56PM +0000, Hogander, Jouni wrote:
> > On Fri, 2025-01-24 at 14:09 +0200, Hogander, Jouni wrote:
> > > On Fri, 2025-01-24 at 13:53 +0200, Ville Syrjälä wrote:
> > > > On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> > > > > Changing PSR mode using DSB is not implemented. Do not use
> > > > > DSB
> > > > > when
> > > > > PSR
> > > > > mode is changing.
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 3ac1cc9ac08a8..a189aa437d972 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -7682,7 +7682,8 @@ static void
> > > > > intel_atomic_dsb_finish(struct
> > > > > intel_atomic_state *state,
> > > > >  		!new_crtc_state->scaler_state.scaler_users
> > > > > &&
> > > > >  		!old_crtc_state->scaler_state.scaler_users
> > > > > &&
> > > > >  		!intel_crtc_needs_modeset(new_crtc_state) &&
> > > > > -		!intel_crtc_needs_fastset(new_crtc_state);
> > > > > +		!intel_crtc_needs_fastset(new_crtc_state) &&
> > > > > +		!intel_psr_is_psr_mode_changing(old_crtc_sta
> > > > > te,
> > > > > new_crtc_state);
> > > > 
> > > > Hmm. Doesn't all that imply a fastset anyway
> > > 
> > > PSR/PR doesn't imply fastset.
> 
> You seem to be checking has_psr, has_sel_update, has_panel_replay,
> and enable_psr2_su_region_et, and all of those seem to come from 
> from intel_psr_compute_config() which is only called from the
> modeset path. Which means it's either going to end up as a full
> modeset or fastset.

Ok, based on this I will drop patches 8 and 9.

BR,

Jouni Högander

> 
> > > 
> > > At the time of enabling PSR/PR crtc_state->has_psr is true at
> > > this
> > > point, but PSR is not yet enabled. It gets enabled only in 
> > > pre_plane_update. Also in hsw_activate_psr2 and
> > > dg2_panel_replay_activate we are writing PSR2_MAN_TRK_CTL.
> > 
> > I said it wrong here: post_plane_update I mean of course.
> 
> We don't really care whether PSR is actually active or not.
> All we care about is whether it might be active. Or I suppose
> technically we wouldn't even have to care about that if we just
> always blasted in the extra DSB commands, but since it's easy to
> avoid the extra overhead when PSR is not even possible then I
> suppose it might be worthwile to check for that.
> 

[PATCH v4 10/13] drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit

[Jouni Högander]

We have different approach on how flip is considered being complete. We are
waiting for vblank on DSB and generate interrupt when it happens and this
interrupt is considered as indication of completion -> we definitely do not
want to skip vblank wait.

Also not skipping scanline wait shouldn't cause any problems if we are in
DEEP_SLEEP PIPEDSL register is returning 0 -> evasion does nothing and if
we are not in DEEP_SLEEP evasion works same way as without PSR.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 89d3496bcbdbd..bb77ded8bf726 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -168,13 +168,12 @@ static u32 dsb_chicken(struct intel_atomic_state *state,
 		       struct intel_crtc *crtc)
 {
 	if (pre_commit_is_vrr_active(state, crtc))
-		return DSB_SKIP_WAITS_EN |
-			DSB_CTRL_WAIT_SAFE_WINDOW |
+		return DSB_CTRL_WAIT_SAFE_WINDOW |
 			DSB_CTRL_NO_WAIT_VBLANK |
 			DSB_INST_WAIT_SAFE_WINDOW |
 			DSB_INST_NO_WAIT_VBLANK;
 	else
-		return DSB_SKIP_WAITS_EN;
+		return 0;
 }
 
 static bool assert_dsb_has_room(struct intel_dsb *dsb)
-- 
2.43.0

Re: [PATCH v4 10/13] drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:56:21PM +0200, Jouni Högander wrote:
> We have different approach on how flip is considered being complete. We are
> waiting for vblank on DSB and generate interrupt when it happens and this
> interrupt is considered as indication of completion -> we definitely do not
> want to skip vblank wait.
> 
> Also not skipping scanline wait shouldn't cause any problems if we are in
> DEEP_SLEEP PIPEDSL register is returning 0 -> evasion does nothing and if
> we are not in DEEP_SLEEP evasion works same way as without PSR.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 89d3496bcbdbd..bb77ded8bf726 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -168,13 +168,12 @@ static u32 dsb_chicken(struct intel_atomic_state *state,
>  		       struct intel_crtc *crtc)
>  {

IIRC bspec suggests that we should always set DSB_SKIP_WAITS_EN,
so we should probably have a comment here explaining our reason
for omitting it. Otherwise I fear that someone is going to be
blindly reading the spec and then try to add the bit back.

>  	if (pre_commit_is_vrr_active(state, crtc))
> -		return DSB_SKIP_WAITS_EN |
> -			DSB_CTRL_WAIT_SAFE_WINDOW |
> +		return DSB_CTRL_WAIT_SAFE_WINDOW |
>  			DSB_CTRL_NO_WAIT_VBLANK |
>  			DSB_INST_WAIT_SAFE_WINDOW |
>  			DSB_INST_NO_WAIT_VBLANK;
>  	else
> -		return DSB_SKIP_WAITS_EN;
> +		return 0;
>  }
>  
>  static bool assert_dsb_has_room(struct intel_dsb *dsb)
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 10/13] drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit

[Hogander, Jouni]

On Fri, 2025-01-24 at 13:46 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:21PM +0200, Jouni Högander wrote:
> > We have different approach on how flip is considered being
> > complete. We are
> > waiting for vblank on DSB and generate interrupt when it happens
> > and this
> > interrupt is considered as indication of completion -> we
> > definitely do not
> > want to skip vblank wait.
> > 
> > Also not skipping scanline wait shouldn't cause any problems if we
> > are in
> > DEEP_SLEEP PIPEDSL register is returning 0 -> evasion does nothing
> > and if
> > we are not in DEEP_SLEEP evasion works same way as without PSR.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dsb.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index 89d3496bcbdbd..bb77ded8bf726 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -168,13 +168,12 @@ static u32 dsb_chicken(struct
> > intel_atomic_state *state,
> >  		       struct intel_crtc *crtc)
> >  {
> 
> IIRC bspec suggests that we should always set DSB_SKIP_WAITS_EN,
> so we should probably have a comment here explaining our reason
> for omitting it. Otherwise I fear that someone is going to be
> blindly reading the spec and then try to add the bit back.

Ok, I will add that.

BR,

Joui Högander
> 
> >  	if (pre_commit_is_vrr_active(state, crtc))
> > -		return DSB_SKIP_WAITS_EN |
> > -			DSB_CTRL_WAIT_SAFE_WINDOW |
> > +		return DSB_CTRL_WAIT_SAFE_WINDOW |
> >  			DSB_CTRL_NO_WAIT_VBLANK |
> >  			DSB_INST_WAIT_SAFE_WINDOW |
> >  			DSB_INST_NO_WAIT_VBLANK;
> >  	else
> > -		return DSB_SKIP_WAITS_EN;
> > +		return 0;
> >  }
> >  
> >  static bool assert_dsb_has_room(struct intel_dsb *dsb)
> > -- 
> > 2.43.0
> 

[PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Jouni Högander]

PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On
wake-up scanline counting starts from vblank_start - 1. We don't know if
wake-up is already ongoing when evasion starts. In worst case PIPEDSL could
start reading valid value right after checking the scanline. In this
scenario we wouldn't have enough time to write all registers. To tackle
this evade scanline 0 as well. As a drawback we have 1 frame delay in flip
when waking up.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index bb77ded8bf726..914f0be4491c4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct intel_atomic_state *state,
 	int latency = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, 20);
 	int start, end;
 
+	/*
+	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On
+	 * wake-up scanline counting starts from vblank_start - 1. We don't know
+	 * if wake-up is already ongoing when evasion starts. In worst case
+	 * PIPEDSL could start reading valid value right after checking the
+	 * scanline. In this scenario we wouldn't have enough time to write all
+	 * registers. To tackle this evade scanline 0 as well. As a drawback we
+	 * have 1 frame delay in flip when waking up.
+	 */
+	if (crtc_state->has_psr && !crtc_state->has_panel_replay)
+		intel_dsb_wait_scanline_out(state, dsb, 0, 0);
+
 	if (pre_commit_is_vrr_active(state, crtc)) {
 		int vblank_delay = intel_vrr_vblank_delay(crtc_state);
 
-- 
2.43.0

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote:
> PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On
> wake-up scanline counting starts from vblank_start - 1. We don't know if
> wake-up is already ongoing when evasion starts. In worst case PIPEDSL could
> start reading valid value right after checking the scanline. In this
> scenario we wouldn't have enough time to write all registers. To tackle
> this evade scanline 0 as well. As a drawback we have 1 frame delay in flip
> when waking up.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index bb77ded8bf726..914f0be4491c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct intel_atomic_state *state,
>  	int latency = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, 20);
>  	int start, end;
>  
> +	/*
> +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On
> +	 * wake-up scanline counting starts from vblank_start - 1. We don't know
> +	 * if wake-up is already ongoing when evasion starts. In worst case
> +	 * PIPEDSL could start reading valid value right after checking the
> +	 * scanline. In this scenario we wouldn't have enough time to write all
> +	 * registers. To tackle this evade scanline 0 as well. As a drawback we
> +	 * have 1 frame delay in flip when waking up.
> +	 */
> +	if (crtc_state->has_psr && !crtc_state->has_panel_replay)

What's the deal with panel replay here?

> +		intel_dsb_wait_scanline_out(state, dsb, 0, 0);

This needs to be a raw
intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_OUT, 0, 0)
because we need to evade the hw scanline 0. What the software
thinks is scanline 0 is a bit different (see scanline_offset).

> +
>  	if (pre_commit_is_vrr_active(state, crtc)) {
>  		int vblank_delay = intel_vrr_vblank_delay(crtc_state);
>  
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Hogander, Jouni]

On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote:
> > PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2).
> > On
> > wake-up scanline counting starts from vblank_start - 1. We don't
> > know if
> > wake-up is already ongoing when evasion starts. In worst case
> > PIPEDSL could
> > start reading valid value right after checking the scanline. In
> > this
> > scenario we wouldn't have enough time to write all registers. To
> > tackle
> > this evade scanline 0 as well. As a drawback we have 1 frame delay
> > in flip
> > when waking up.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index bb77ded8bf726..914f0be4491c4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct
> > intel_atomic_state *state,
> >  	int latency = intel_usecs_to_scanlines(&crtc_state-
> > >hw.adjusted_mode, 20);
> >  	int start, end;
> >  
> > +	/*
> > +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > DEEP_SLEEP(PSR2). On
> > +	 * wake-up scanline counting starts from vblank_start - 1.
> > We don't know
> > +	 * if wake-up is already ongoing when evasion starts. In
> > worst case
> > +	 * PIPEDSL could start reading valid value right after
> > checking the
> > +	 * scanline. In this scenario we wouldn't have enough time
> > to write all
> > +	 * registers. To tackle this evade scanline 0 as well. As
> > a drawback we
> > +	 * have 1 frame delay in flip when waking up.
> > +	 */
> > +	if (crtc_state->has_psr && !crtc_state->has_panel_replay)
> 
> What's the deal with panel replay here?

I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on
same setup with PSR I saw it, but with PR I couldn't see.

> 
> > +		intel_dsb_wait_scanline_out(state, dsb, 0, 0);
> 
> This needs to be a raw
> intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_OUT, 0, 0)
> because we need to evade the hw scanline 0. What the software
> thinks is scanline 0 is a bit different (see scanline_offset).

Ok, I will change this.

BR,

Jouni Högander

> 
> > +
> >  	if (pre_commit_is_vrr_active(state, crtc)) {
> >  		int vblank_delay =
> > intel_vrr_vblank_delay(crtc_state);
> >  
> > -- 
> > 2.43.0
> 

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote:
> On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote:
> > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2).
> > > On
> > > wake-up scanline counting starts from vblank_start - 1. We don't
> > > know if
> > > wake-up is already ongoing when evasion starts. In worst case
> > > PIPEDSL could
> > > start reading valid value right after checking the scanline. In
> > > this
> > > scenario we wouldn't have enough time to write all registers. To
> > > tackle
> > > this evade scanline 0 as well. As a drawback we have 1 frame delay
> > > in flip
> > > when waking up.
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > index bb77ded8bf726..914f0be4491c4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct
> > > intel_atomic_state *state,
> > >  	int latency = intel_usecs_to_scanlines(&crtc_state-
> > > >hw.adjusted_mode, 20);
> > >  	int start, end;
> > >  
> > > +	/*
> > > +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > DEEP_SLEEP(PSR2). On
> > > +	 * wake-up scanline counting starts from vblank_start - 1.
> > > We don't know
> > > +	 * if wake-up is already ongoing when evasion starts. In
> > > worst case
> > > +	 * PIPEDSL could start reading valid value right after
> > > checking the
> > > +	 * scanline. In this scenario we wouldn't have enough time
> > > to write all
> > > +	 * registers. To tackle this evade scanline 0 as well. As
> > > a drawback we
> > > +	 * have 1 frame delay in flip when waking up.
> > > +	 */
> > > +	if (crtc_state->has_psr && !crtc_state->has_panel_replay)
> > 
> > What's the deal with panel replay here?
> 
> I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on
> same setup with PSR I saw it, but with PR I couldn't see.

Hmm. Are you sure it's reaching DC5/6?

Though I suppose it's possible that panel replay (unlike PSR)
does pretty much everything from the DMC's DC state handler,
so as soon as you read the register it restores things fully
enough that you get the vblank_start-1 response...

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Hogander, Jouni]

On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote:
> > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote:
> > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > DEEP_SLEEP(PSR2).
> > > > On
> > > > wake-up scanline counting starts from vblank_start - 1. We
> > > > don't
> > > > know if
> > > > wake-up is already ongoing when evasion starts. In worst case
> > > > PIPEDSL could
> > > > start reading valid value right after checking the scanline. In
> > > > this
> > > > scenario we wouldn't have enough time to write all registers.
> > > > To
> > > > tackle
> > > > this evade scanline 0 as well. As a drawback we have 1 frame
> > > > delay
> > > > in flip
> > > > when waking up.
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > index bb77ded8bf726..914f0be4491c4 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct
> > > > intel_atomic_state *state,
> > > >  	int latency = intel_usecs_to_scanlines(&crtc_state-
> > > > > hw.adjusted_mode, 20);
> > > >  	int start, end;
> > > >  
> > > > +	/*
> > > > +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > DEEP_SLEEP(PSR2). On
> > > > +	 * wake-up scanline counting starts from vblank_start
> > > > - 1.
> > > > We don't know
> > > > +	 * if wake-up is already ongoing when evasion starts.
> > > > In
> > > > worst case
> > > > +	 * PIPEDSL could start reading valid value right after
> > > > checking the
> > > > +	 * scanline. In this scenario we wouldn't have enough
> > > > time
> > > > to write all
> > > > +	 * registers. To tackle this evade scanline 0 as well.
> > > > As
> > > > a drawback we
> > > > +	 * have 1 frame delay in flip when waking up.
> > > > +	 */
> > > > +	if (crtc_state->has_psr && !crtc_state-
> > > > >has_panel_replay)
> > > 
> > > What's the deal with panel replay here?
> > 
> > I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on
> > same setup with PSR I saw it, but with PR I couldn't see.
> 
> Hmm. Are you sure it's reaching DC5/6?
> 
> Though I suppose it's possible that panel replay (unlike PSR)
> does pretty much everything from the DMC's DC state handler,
> so as soon as you read the register it restores things fully
> enough that you get the vblank_start-1 response...

Maybe we just add that evasion without checking panel replay. It
shouldn't be too expensive anyways. What do you think?

BR,

Jouni Högander
> 

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:41:11PM +0000, Hogander, Jouni wrote:
> On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote:
> > > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote:
> > > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote:
> > > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > > DEEP_SLEEP(PSR2).
> > > > > On
> > > > > wake-up scanline counting starts from vblank_start - 1. We
> > > > > don't
> > > > > know if
> > > > > wake-up is already ongoing when evasion starts. In worst case
> > > > > PIPEDSL could
> > > > > start reading valid value right after checking the scanline. In
> > > > > this
> > > > > scenario we wouldn't have enough time to write all registers.
> > > > > To
> > > > > tackle
> > > > > this evade scanline 0 as well. As a drawback we have 1 frame
> > > > > delay
> > > > > in flip
> > > > > when waking up.
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
> > > > >  1 file changed, 12 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > index bb77ded8bf726..914f0be4491c4 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct
> > > > > intel_atomic_state *state,
> > > > >  	int latency = intel_usecs_to_scanlines(&crtc_state-
> > > > > > hw.adjusted_mode, 20);
> > > > >  	int start, end;
> > > > >  
> > > > > +	/*
> > > > > +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > > DEEP_SLEEP(PSR2). On
> > > > > +	 * wake-up scanline counting starts from vblank_start
> > > > > - 1.
> > > > > We don't know
> > > > > +	 * if wake-up is already ongoing when evasion starts.
> > > > > In
> > > > > worst case
> > > > > +	 * PIPEDSL could start reading valid value right after
> > > > > checking the
> > > > > +	 * scanline. In this scenario we wouldn't have enough
> > > > > time
> > > > > to write all
> > > > > +	 * registers. To tackle this evade scanline 0 as well.
> > > > > As
> > > > > a drawback we
> > > > > +	 * have 1 frame delay in flip when waking up.
> > > > > +	 */
> > > > > +	if (crtc_state->has_psr && !crtc_state-
> > > > > >has_panel_replay)
> > > > 
> > > > What's the deal with panel replay here?
> > > 
> > > I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on
> > > same setup with PSR I saw it, but with PR I couldn't see.
> > 
> > Hmm. Are you sure it's reaching DC5/6?
> > 
> > Though I suppose it's possible that panel replay (unlike PSR)
> > does pretty much everything from the DMC's DC state handler,
> > so as soon as you read the register it restores things fully
> > enough that you get the vblank_start-1 response...
> 
> Maybe we just add that evasion without checking panel replay. It
> shouldn't be too expensive anyways. What do you think?

Yeah, that seems fine to me.

But I still think you should try to double check that it
really reaches DC6 with panel replay despite PIPDSL not
getting reset, so that we at least understand a bit better
what is actually happening.

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 11/13] drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled

[Hogander, Jouni]

On Fri, 2025-01-24 at 14:49 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:41:11PM +0000, Hogander, Jouni wrote:
> > On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote:
> > > > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote:
> > > > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander
> > > > > wrote:
> > > > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > > > DEEP_SLEEP(PSR2).
> > > > > > On
> > > > > > wake-up scanline counting starts from vblank_start - 1. We
> > > > > > don't
> > > > > > know if
> > > > > > wake-up is already ongoing when evasion starts. In worst
> > > > > > case
> > > > > > PIPEDSL could
> > > > > > start reading valid value right after checking the
> > > > > > scanline. In
> > > > > > this
> > > > > > scenario we wouldn't have enough time to write all
> > > > > > registers.
> > > > > > To
> > > > > > tackle
> > > > > > this evade scanline 0 as well. As a drawback we have 1
> > > > > > frame
> > > > > > delay
> > > > > > in flip
> > > > > > when waking up.
> > > > > > 
> > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++
> > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > > index bb77ded8bf726..914f0be4491c4 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct
> > > > > > intel_atomic_state *state,
> > > > > >  	int latency =
> > > > > > intel_usecs_to_scanlines(&crtc_state-
> > > > > > > hw.adjusted_mode, 20);
> > > > > >  	int start, end;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * PIPEDSL is reading as 0 when in SRDENT(PSR1) or
> > > > > > DEEP_SLEEP(PSR2). On
> > > > > > +	 * wake-up scanline counting starts from
> > > > > > vblank_start
> > > > > > - 1.
> > > > > > We don't know
> > > > > > +	 * if wake-up is already ongoing when evasion
> > > > > > starts.
> > > > > > In
> > > > > > worst case
> > > > > > +	 * PIPEDSL could start reading valid value right
> > > > > > after
> > > > > > checking the
> > > > > > +	 * scanline. In this scenario we wouldn't have
> > > > > > enough
> > > > > > time
> > > > > > to write all
> > > > > > +	 * registers. To tackle this evade scanline 0 as
> > > > > > well.
> > > > > > As
> > > > > > a drawback we
> > > > > > +	 * have 1 frame delay in flip when waking up.
> > > > > > +	 */
> > > > > > +	if (crtc_state->has_psr && !crtc_state-
> > > > > > > has_panel_replay)
> > > > > 
> > > > > What's the deal with panel replay here?
> > > > 
> > > > I couldn't see PIPEDSL returning 0 when using Panel Replay.
> > > > Even on
> > > > same setup with PSR I saw it, but with PR I couldn't see.
> > > 
> > > Hmm. Are you sure it's reaching DC5/6?
> > > 
> > > Though I suppose it's possible that panel replay (unlike PSR)
> > > does pretty much everything from the DMC's DC state handler,
> > > so as soon as you read the register it restores things fully
> > > enough that you get the vblank_start-1 response...
> > 
> > Maybe we just add that evasion without checking panel replay. It
> > shouldn't be too expensive anyways. What do you think?
> 
> Yeah, that seems fine to me.
> 
> But I still think you should try to double check that it
> really reaches DC6 with panel replay despite PIPDSL not
> getting reset, so that we at least understand a bit better
> what is actually happening.
 
Here are my observations:

1. TGL/PSR2, no DMC firmware : PIPEDSL is reading as vblank - 1 when in
DEEP_SLEEP

2. TGL/PSR2, DMC firmware loaded: PIPEDSL is reading as 0 when in
DEEP_SLEEP

3. TGL/PSR2, DMC fimware loaded , DC states disabled : PIPEDSL reading
is running all the time

4. LNL/Panel Replay, no DMC firmware : PIPEDSL reading is running all
the time

5. LNL/Panel Replay, DMC firmware loaded : PIPEDSL is reading as vblank
- 1 when in SLEEP

6. LNL/Panel Replay, DMC firmware loaded, DC state disabled : PIPEDSL
reading is running all the time

Results are differing. Based on this: Current evation is handling cases
where PIPEDSL reading is running or vblank - 1. PIPEDSL reading as 0 is
handled by this adding scanline 0 evation.

PSR2 commits not using DSB currently might be actually affected by your
concern. I.e. wake-up is already ongoing when performing vblank
evation. PSR1 shouldn't have this problem as there we are waiting PSR
state to become idle. Maybe we should add scanline 0 there as well? Not
related to this patch though.

BR,

Jouni Högander

> 

[PATCH v4 12/13] drm/i915/display: Ensure we have "Frame Change" event in DSB commit

[Jouni Högander]

We may have commit which doesn't have any non-arming plane register
writes. In that case there aren't "Frame Change" event before DSB vblank
evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
triggering the "Frame Change" event.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a189aa437d972..cd7e960bd29f1 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		intel_atomic_get_old_crtc_state(state, crtc);
 	struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_display *display = to_intel_display(crtc);
 
 	if (!new_crtc_state->hw.active)
 		return;
@@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
 					       state, crtc);
 
+		/*
+		 * Ensure we have "Frame Change" event when PSR state is
+		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
+		 * evasion hangs as PIPEDSL is reading as 0.
+		 */
+		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
+			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
+					   CURSURFLIVE(display, crtc->pipe), 0);
+
 		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
 
 		if (intel_crtc_needs_color_update(new_crtc_state))
-- 
2.43.0

Re: [PATCH v4 12/13] drm/i915/display: Ensure we have "Frame Change" event in DSB commit

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> We may have commit which doesn't have any non-arming plane register
> writes. In that case there aren't "Frame Change" event before DSB vblank
> evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
> SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> triggering the "Frame Change" event.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a189aa437d972..cd7e960bd29f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_display *display = to_intel_display(crtc);
>  
>  	if (!new_crtc_state->hw.active)
>  		return;
> @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
>  					       state, crtc);
>  
> +		/*
> +		 * Ensure we have "Frame Change" event when PSR state is
> +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
> +		 * evasion hangs as PIPEDSL is reading as 0.
> +		 */
> +		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
> +			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
> +					   CURSURFLIVE(display, crtc->pipe), 0);

I don't really want to see the low level detais right here.
So we should probably should stuff this into some kind of 
intel_dsb_ensure_psr_frame_change() function or something
along those lines.

> +
>  		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
>  
>  		if (intel_crtc_needs_color_update(new_crtc_state))
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 12/13] drm/i915/display: Ensure we have "Frame Change" event in DSB commit

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 01:43:25PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> > We may have commit which doesn't have any non-arming plane register
> > writes. In that case there aren't "Frame Change" event before DSB vblank
> > evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
> > SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> > triggering the "Frame Change" event.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a189aa437d972..cd7e960bd29f1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_crtc_state *new_crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_display *display = to_intel_display(crtc);
> >  
> >  	if (!new_crtc_state->hw.active)
> >  		return;
> > @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> >  		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
> >  					       state, crtc);
> >  
> > +		/*
> > +		 * Ensure we have "Frame Change" event when PSR state is
> > +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
> > +		 * evasion hangs as PIPEDSL is reading as 0.
> > +		 */
> > +		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
> > +			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
> > +					   CURSURFLIVE(display, crtc->pipe), 0);
> 
> I don't really want to see the low level detais right here.
> So we should probably should stuff this into some kind of 
> intel_dsb_ensure_psr_frame_change() function or something
> along those lines.

Oh, and I guess this really should be using
intel_pre_commit_crtc_state() as well. I suppose it doesn't
actually matter if we skip commits that change PSR stuff anyway,
but at some point the plan is to attempt full fastsets with
the DSB (if actually possible).

> 
> > +
> >  		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
> >  
> >  		if (intel_crtc_needs_color_update(new_crtc_state))
> > -- 
> > 2.43.0
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 12/13] drm/i915/display: Ensure we have "Frame Change" event in DSB commit

[Hogander, Jouni]

On Fri, 2025-01-24 at 13:43 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> > We may have commit which doesn't have any non-arming plane register
> > writes. In that case there aren't "Frame Change" event before DSB
> > vblank
> > evasion which hangs as PIPEDSL register is reading as 0 when PSR
> > state is
> > SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> > triggering the "Frame Change" event.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a189aa437d972..cd7e960bd29f1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct
> > intel_atomic_state *state,
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_crtc_state *new_crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_display *display = to_intel_display(crtc);
> >  
> >  	if (!new_crtc_state->hw.active)
> >  		return;
> > @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct
> > intel_atomic_state *state,
> >  		intel_crtc_planes_update_noarm(new_crtc_state-
> > >dsb_commit,
> >  					       state, crtc);
> >  
> > +		/*
> > +		 * Ensure we have "Frame Change" event when PSR
> > state is
> > +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB
> > vblank
> > +		 * evasion hangs as PIPEDSL is reading as 0.
> > +		 */
> > +		if (new_crtc_state->has_psr && !new_crtc_state-
> > >has_panel_replay)
> > +			intel_de_write_dsb(display,
> > new_crtc_state->dsb_commit,
> > +					   CURSURFLIVE(display,
> > crtc->pipe), 0);
> 
> I don't really want to see the low level detais right here.
> So we should probably should stuff this into some kind of 
> intel_dsb_ensure_psr_frame_change() function or something
> along those lines.

Ok, I will write such function.

BR,

Jouni Högander
> 
> > +
> >  		intel_dsb_vblank_evade(state, new_crtc_state-
> > >dsb_commit);
> >  
> >  		if (intel_crtc_needs_color_update(new_crtc_state))
> > -- 
> > 2.43.0
> 

[PATCH v4 13/13] drm/i915/psr: Allow DSB usage when PSR is enabled

[Jouni Högander]

Now as we have correct PSR2_MAN_TRK_CTL handling in place we can allow DSB
usage also when PSR is enabled for LunarLake onwards.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index cd7e960bd29f1..08743e85382ee 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7679,7 +7679,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		new_crtc_state->update_planes &&
 		!new_crtc_state->vrr.enable &&
 		!new_crtc_state->do_async_flip &&
-		!new_crtc_state->has_psr &&
+		(DISPLAY_VER(display) >= 20 || !new_crtc_state->has_psr) &&
 		!new_crtc_state->scaler_state.scaler_users &&
 		!old_crtc_state->scaler_state.scaler_users &&
 		!intel_crtc_needs_modeset(new_crtc_state) &&
-- 
2.43.0

✗ CI.Patch_applied: failure for PSR DSB support (rev4)

[Patchwork]

== Series Details ==

Series: PSR DSB support (rev4)
URL   : https://patchwork.freedesktop.org/series/142521/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 5b338c20b801 drm-tip: 2025y-01m-24d-10h-28m-05s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/i915/display/intel_display.c:7679
error: drivers/gpu/drm/i915/display/intel_display.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/i915/psr: Use PSR2_MAN_TRK_CTL CFF bit only to send full update
Applying: drm/i915/psr: Rename psr_force_hw_tracking_exit as intel_psr_force_update
Applying: drm/i915/psr: Split setting sff and cff bits away from intel_psr_force_update
Applying: drm/i915/psr: Add register definitions for SFF_CTL and CFF_CTL registers
Applying: drm/i915/psr: Use SFF_CTL on invalidate/flush for LunarLake onwards
Applying: drm/i915/psr: Allow writing PSR2_MAN_TRK_CTL using DSB
Applying: drm/i915/psr: Changes for PSR2_MAN_TRK_CTL handling when DSB is in use
Applying: drm/i915/psr: Add intel_psr_is_psr_mode_changing
Applying: drm/i915/display: Don't use DSB if psr mode changing
Applying: drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit
Applying: drm/i915/display: Evade scanline 0 as well if PSR1 or PSR2 is enabled
Applying: drm/i915/display: Ensure we have "Frame Change" event in DSB commit
Applying: drm/i915/psr: Allow DSB usage when PSR is enabled
Patch failed at 0013 drm/i915/psr: Allow DSB usage when PSR is enabled
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".