On Fri, Nov 15, 2019 at 04:08:45PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Intel-gfx <[email protected]> On Behalf Of Ville > >Syrjala > >Sent: Thursday, October 24, 2019 5:52 PM > >To: [email protected] > >Subject: [Intel-gfx] [PATCH 3/3] drm/i915: Fix frame start delay programming > > > >From: Ville Syrjälä <[email protected]> > > > >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ä <[email protected]> > >--- > > 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 <[email protected]> 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 > >[email protected] > >https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
