Re: [PATCH 3/3] drm/i915: Fix frame start delay programming

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 15, 2019 at 04:08:45PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> >Sent: Thursday, October 24, 2019 5:52 PM
> >To: intel-gfx@lists.freedesktop.org
> >Subject: [Intel-gfx] [PATCH 3/3] drm/i915: Fix frame start delay programming
> >
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Currently we're blindly poking at the frame start delay bits in PIPECONF when trying to
> >sanitize the hardware state. Those bits decided to move elsewhere on HSW, so on
> >many platforms we're not doing anything at all here. Also we're forgetting about the
> >PCH transcoder entirely.
> >
> >Add all the bit definitions for the various homes these bits have had throughout the
> >years, and reset them all to zero.
> >
> >However I'm not entirely sure this is a safe thing to do. If not I guess we'd want full
> >readout+statecheck for this stuff.
> >For now let's stick to the current logic and hope for the best.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_display.c | 101 ++++++++++++++++---
> > drivers/gpu/drm/i915/i915_reg.h              |  12 ++-
> > drivers/gpu/drm/i915/intel_pm.c              |   1 -
> > 3 files changed, 95 insertions(+), 19 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >b/drivers/gpu/drm/i915/display/intel_display.c
> >index 579655675b08..2896cf864f61 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >@@ -1645,11 +1645,16 @@ static void ironlake_enable_pch_transcoder(const struct
> >intel_crtc_state *crtc_s
> > 	assert_fdi_rx_enabled(dev_priv, pipe);
> >
> > 	if (HAS_PCH_CPT(dev_priv)) {
> >-		/* Workaround: Set the timing override bit before enabling the
> >-		 * pch transcoder. */
> > 		reg = TRANS_CHICKEN2(pipe);
> > 		val = I915_READ(reg);
> >+		/*
> >+		 * Workaround: Set the timing override bit
> >+		 * before enabling the pch transcoder.
> >+		 */
> > 		val |= TRANS_CHICKEN2_TIMING_OVERRIDE;
> >+		/* Configure frame start delay to match the CPU */
> >+		val &= ~TRANS_CHICKEN2_FRAME_START_DELAY_MASK;
> >+		val |= TRANS_CHICKEN2_FRAME_START_DELAY(0);
> > 		I915_WRITE(reg, val);
> > 	}
> >
> >@@ -1658,6 +1663,10 @@ static void ironlake_enable_pch_transcoder(const struct
> >intel_crtc_state *crtc_s
> > 	pipeconf_val = I915_READ(PIPECONF(pipe));
> >
> > 	if (HAS_PCH_IBX(dev_priv)) {
> >+		/* Configure frame start delay to match the CPU */
> >+		val &= ~TRANS_FRAME_START_DELAY_MASK;
> >+		val |= TRANS_FRAME_START_DELAY(0);
> >+
> > 		/*
> > 		 * Make the BPC in transcoder be consistent with
> > 		 * that in pipeconf reg. For HDMI we must use 8bpc @@ -1695,9
> >+1704,12 @@ static void lpt_enable_pch_transcoder(struct drm_i915_private
> >*dev_priv,
> > 	assert_fdi_tx_enabled(dev_priv, (enum pipe) cpu_transcoder);
> > 	assert_fdi_rx_enabled(dev_priv, PIPE_A);
> >
> >+	val = I915_READ(TRANS_CHICKEN2(PIPE_A));
> > 	/* Workaround: set timing override bit. */
> >-	val = I915_READ(TRANS_CHICKEN2(PIPE_A));
> > 	val |= TRANS_CHICKEN2_TIMING_OVERRIDE;
> >+	/* Configure frame start delay to match the CPU */
> >+	val &= ~TRANS_CHICKEN2_FRAME_START_DELAY_MASK;
> >+	val |= TRANS_CHICKEN2_FRAME_START_DELAY(0);
> > 	I915_WRITE(TRANS_CHICKEN2(PIPE_A), val);
> >
> > 	val = TRANS_ENABLE;
> >@@ -6452,6 +6464,19 @@ static void icl_pipe_mbus_enable(struct intel_crtc *crtc)
> > 	I915_WRITE(PIPE_MBUS_DBOX_CTL(pipe), val);  }
> >
> >+static void hsw_set_frame_start_delay(const struct intel_crtc_state
> >+*crtc_state) {
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >+	i915_reg_t reg = CHICKEN_TRANS(crtc_state->cpu_transcoder);
> >+	u32 val;
> >+
> >+	val = I915_READ(reg);
> >+	val &= ~HSW_FRAME_START_DELAY_MASK;
> >+	val |= HSW_FRAME_START_DELAY(0);
> >+	I915_WRITE(reg, val);
> >+}
> >+
> > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > 				struct intel_atomic_state *state)
> > {
> >@@ -6494,8 +6519,10 @@ static void haswell_crtc_enable(struct intel_crtc_state
> >*pipe_config,
> > 					     &pipe_config->fdi_m_n, NULL);
> > 	}
> >
> >-	if (!transcoder_is_dsi(cpu_transcoder))
> >+	if (!transcoder_is_dsi(cpu_transcoder)) {
> >+		hsw_set_frame_start_delay(pipe_config);
> > 		haswell_set_pipeconf(pipe_config);
> >+	}
> >
> > 	if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > 		bdw_set_pipemisc(pipe_config);
> >@@ -8394,6 +8421,8 @@ static void i9xx_set_pipeconf(const struct intel_crtc_state
> >*crtc_state)
> >
> > 	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> >
> >+	pipeconf |= PIPECONF_FRAME_START_DELAY(0);
> >+
> > 	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
> > 	POSTING_READ(PIPECONF(crtc->pipe));
> > }
> >@@ -9474,6 +9503,8 @@ static void ironlake_set_pipeconf(const struct
> >intel_crtc_state *crtc_state)
> >
> > 	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> >
> >+	val |= PIPECONF_FRAME_START_DELAY(0);
> >+
> > 	I915_WRITE(PIPECONF(pipe), val);
> > 	POSTING_READ(PIPECONF(pipe));
> > }
> >@@ -16919,25 +16950,69 @@ static bool has_pch_trancoder(struct
> >drm_i915_private *dev_priv,
> > 		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == PIPE_A);  }
> >
> >+static void intel_sanitize_frame_start_delay(const struct
> >+intel_crtc_state *crtc_state) {
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >+
> >+	if (INTEL_GEN(dev_priv) >= 9 ||
> >+	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) {
> >+		i915_reg_t reg = CHICKEN_TRANS(cpu_transcoder);
> >+		u32 val;
> >+
> >+		if (transcoder_is_dsi(cpu_transcoder))
> >+			return;
> >+
> >+		val = I915_READ(reg);
> >+		val &= ~HSW_FRAME_START_DELAY_MASK;
> >+		val |= HSW_FRAME_START_DELAY(0);
> >+		I915_WRITE(reg, val);
> >+	} else {
> >+		i915_reg_t reg = PIPECONF(cpu_transcoder);
> >+		u32 val;
> >+
> >+		val = I915_READ(reg);
> >+		val &= ~PIPECONF_FRAME_START_DELAY_MASK;
> >+		val |= PIPECONF_FRAME_START_DELAY(0);
> >+		I915_WRITE(reg, val);
> >+	}
> >+
> >+	if (!crtc_state->has_pch_encoder)
> >+		return;
> >+
> >+	if (HAS_PCH_IBX(dev_priv)) {
> >+		i915_reg_t reg = PCH_TRANSCONF(crtc->pipe);
> >+		u32 val;
> >+
> >+		val = I915_READ(reg);
> >+		val &= ~TRANS_FRAME_START_DELAY_MASK;
> >+		val |= TRANS_FRAME_START_DELAY(0);
> >+		I915_WRITE(reg, val);
> >+	} else {
> >+		i915_reg_t reg = TRANS_CHICKEN2(crtc->pipe);
> >+		u32 val;
> >+
> >+		val = I915_READ(reg);
> >+		val &= ~TRANS_CHICKEN2_FRAME_START_DELAY_MASK;
> >+		val |= TRANS_CHICKEN2_FRAME_START_DELAY(0);
> >+		I915_WRITE(reg, val);
> >+	}
> >+}
> >+
> > static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > 				struct drm_modeset_acquire_ctx *ctx)  {
> > 	struct drm_device *dev = crtc->base.dev;
> > 	struct drm_i915_private *dev_priv = to_i915(dev);
> > 	struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state);
> >-	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >-
> >-	/* Clear any frame start delays used for debugging left by the BIOS */
> >-	if (crtc->active && !transcoder_is_dsi(cpu_transcoder)) {
> >-		i915_reg_t reg = PIPECONF(cpu_transcoder);
> >-
> >-		I915_WRITE(reg,
> >-			   I915_READ(reg) &
> >~PIPECONF_FRAME_START_DELAY_MASK);
> >-	}
> >
> > 	if (crtc_state->base.active) {
> > 		struct intel_plane *plane;
> >
> >+		/* Clear any frame start delays used for debugging left by the BIOS */
> >+		intel_sanitize_frame_start_delay(crtc_state);
> >+
> > 		/* Disable everything but the primary plane */
> > 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > 			const struct intel_plane_state *plane_state = diff --git
> >a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index
> >50c2fa0f2cab..cb2e0f4679c4 100644
> >--- a/drivers/gpu/drm/i915/i915_reg.h
> >+++ b/drivers/gpu/drm/i915/i915_reg.h
> >@@ -5671,7 +5671,8 @@ enum {
> > #define   PIPECONF_DOUBLE_WIDE	(1 << 30)
> > #define   I965_PIPECONF_ACTIVE	(1 << 30)
> > #define   PIPECONF_DSI_PLL_LOCKED	(1 << 29) /* vlv & pipe A only */
> >-#define   PIPECONF_FRAME_START_DELAY_MASK (3 << 27)
> >+#define   PIPECONF_FRAME_START_DELAY_MASK	(3 << 27) /* pre-hsw */
> >+#define   PIPECONF_FRAME_START_DELAY(x)		((x) << 27) /* pre-hsw: 0-3
> >*/
> > #define   PIPECONF_SINGLE_WIDE	0
> > #define   PIPECONF_PIPE_UNLOCKED 0
> > #define   PIPECONF_PIPE_LOCKED	(1 << 25)
> >@@ -7627,6 +7628,8 @@ enum {
> > 					    [TRANSCODER_B] = _CHICKEN_TRANS_B,
> >\
> > 					    [TRANSCODER_C] = _CHICKEN_TRANS_C,
> >\
> > 					    [TRANSCODER_D] =
> >_CHICKEN_TRANS_D))
> >+#define  HSW_FRAME_START_DELAY_MASK	(3 << 27)
> >+#define  HSW_FRAME_START_DELAY(x)	((x) << 27) /* 0-3 */
> > #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
> > #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
> > #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
> >@@ -8341,10 +8344,8 @@ enum {
> > #define  TRANS_STATE_MASK       (1 << 30)
> > #define  TRANS_STATE_DISABLE    (0 << 30)
> > #define  TRANS_STATE_ENABLE     (1 << 30)
> >-#define  TRANS_FSYNC_DELAY_HB1  (0 << 27) -#define  TRANS_FSYNC_DELAY_HB2
> >(1 << 27) -#define  TRANS_FSYNC_DELAY_HB3  (2 << 27) -#define
> >TRANS_FSYNC_DELAY_HB4  (3 << 27)
> >+#define  TRANS_FRAME_START_DELAY_MASK	(3 << 27) /* ibx */
> >+#define  TRANS_FRAME_START_DELAY(x)	((x) << 27) /* ibx: 0-3 */
> > #define  TRANS_INTERLACE_MASK   (7 << 21)
> > #define  TRANS_PROGRESSIVE      (0 << 21)
> > #define  TRANS_INTERLACED       (3 << 21)
> >@@ -8365,6 +8366,7 @@ enum {
> > #define  TRANS_CHICKEN2_TIMING_OVERRIDE			(1 << 31)
> > #define  TRANS_CHICKEN2_FDI_POLARITY_REVERSED		(1 << 29)
> > #define  TRANS_CHICKEN2_FRAME_START_DELAY_MASK		(3 << 27)
> >+#define  TRANS_CHICKEN2_FRAME_START_DELAY(x)		((x) << 27) /* 0-3 */
> > #define  TRANS_CHICKEN2_DISABLE_DEEP_COLOR_COUNTER	(1 << 26)
> > #define  TRANS_CHICKEN2_DISABLE_DEEP_COLOR_MODESWITCH	(1 << 25)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index
> >362234449087..8b1fbdb36537 100644
> >--- a/drivers/gpu/drm/i915/intel_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_pm.c
> >@@ -8079,7 +8079,6 @@ static void cpt_init_clock_gating(struct drm_i915_private
> >*dev_priv)
> > 		val &= ~TRANS_CHICKEN2_FDI_POLARITY_REVERSED;
> > 		if (dev_priv->vbt.fdi_rx_polarity_inverted)
> > 			val |= TRANS_CHICKEN2_FDI_POLARITY_REVERSED;
> >-		val &= ~TRANS_CHICKEN2_FRAME_START_DELAY_MASK;
> 
> Is there any reason not to let it be set at 0.
> 
> Overall, I looked at the register and bit definitions for various platforms and this looks
> a very reasonable change rectifying the frame start delay programming.
> 
> However I am not sure if for all platforms 0 will be the preferred value. If the default values
> of these bits are 0 in hardware register (or the BIOS set them at 0 itself), this should
> work seamlessly. Only caveat will be if the defaults are not 0 on all platforms, we may have
> issues. 

Default is 0, but supposedly some VBIOSen have left other values
in there for whatever reason. Or else we probably wouldn't have this
code to sanitize things.

I *may* want to try bumping this to 3 across the board to give
gamma programming more breathing room during the vblank, but I'm
not sure that's entirely safe to do. Hence I started with just
cleaning up the current mess.

IIRC there was also some obnoxious workaround on LVDS+IBX (or maybe
it was CPT) that would require some extra handling if we were to
program this to some non-zero value.

> 
> Good way to figure this out I guess is our CI results. So if the CI passes, this is
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>

Ta.

> 
> > 		val &= ~TRANS_CHICKEN2_DISABLE_DEEP_COLOR_COUNTER;
> > 		val &= ~TRANS_CHICKEN2_DISABLE_DEEP_COLOR_MODESWITCH;
> > 		I915_WRITE(TRANS_CHICKEN2(pipe), val);
> >--
> >2.21.0
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx