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