>-----Original Message-----
>From: Intel-gfx <intel-gfx-boun...@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.syrj...@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.syrj...@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.
Good way to figure this out I guess is our CI results. So if the CI passes,
this is
Reviewed-by: Uma Shankar <uma.shan...@intel.com>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
