| Message ID | 20190304134113.13024-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Simplify i830 DVO 2x clock handling | expand |
Quoting Ville Syrjala (2019-03-04 13:41:13) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Let's just always enable the DVO 2x clock on i830. This way we don't > have to track if DVO is being used or not. The spec does suggest we > should disable the clock when it isn't needed, but this does appear > to work just fine. And after i830, 2X_MODE seems to be the default? Whereas the only reason for i830 being special is that both pipes must be using the same mode, ergo we didn't do either (or something like that). > This removes another crtc->config usage. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > @@ -1468,26 +1455,12 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, > /* > * Apparently we need to have VGA mode enabled prior to changing > * the P1/P2 dividers. Otherwise the DPLL will keep using the old > * dividers, even though the register value does change. > */ > - I915_WRITE(reg, 0); > - > + I915_WRITE(reg, dpll & ~DPLL_VGA_MODE_DIS); This looks to be a separate tweak? > I915_WRITE(reg, dpll); > > /* Wait for the clocks to stabilize. */ > @@ -7494,7 +7457,19 @@ static void i8xx_compute_dpll(struct intel_crtc *crtc, > dpll |= PLL_P2_DIVIDE_BY_4; > } > > - if (!IS_I830(dev_priv) && > + /* > + * Bspec: > + * "[Almador Errata}: For the correct operation of the muxed DVO pins > + * (GDEVSELB/ I 2 Cdata, GIRDBY/I 2 CClk) and (GFRAMEB/DVI_Data, > + * GTRDYB/DVI_Clk): Bit 31 (DPLL VCO Enable) and Bit 30 (2X Clock > + * Enable) must be set to “1” in both the DPLL A Control Register > + * (06014h-06017h) and DPLL B Control Register (06018h-0601Bh)." > + * > + * For simplicity We simply keep both bits always enabled in > + * both DPLLS. The spec says we should disable the DVO 2X clock > + * when not needed, but this seems to work fine in practice. > + */ > + if (IS_I830(dev_priv) || > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DVO)) > dpll |= DPLL_DVO_2X_MODE; Is VGA/CRT a native encoder? That might be a useful test to check that still works. I couldn't see anything you missed for DPLL_DVO_2X_MODE, so happy that this does what it says on the tin, and I trust that you actually ran this on an i830... Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, Mar 04, 2019 at 03:52:30PM +0000, Chris Wilson wrote: > Quoting Ville Syrjala (2019-03-04 13:41:13) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Let's just always enable the DVO 2x clock on i830. This way we don't > > have to track if DVO is being used or not. The spec does suggest we > > should disable the clock when it isn't needed, but this does appear > > to work just fine. > > And after i830, 2X_MODE seems to be the default? Whereas the only reason > for i830 being special is that both pipes must be using the same mode, > ergo we didn't do either (or something like that). Post i830 the hw was fixed so we only have to enable the 2x clock when the pipe is driving a DVO port. Driving the CRT and LVDS ports does not require the 2x clock. > > > This removes another crtc->config usage. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > @@ -1468,26 +1455,12 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, > > /* > > * Apparently we need to have VGA mode enabled prior to changing > > * the P1/P2 dividers. Otherwise the DPLL will keep using the old > > * dividers, even though the register value does change. > > */ > > - I915_WRITE(reg, 0); > > - > > + I915_WRITE(reg, dpll & ~DPLL_VGA_MODE_DIS); > > This looks to be a separate tweak? I didn't want to termporarily disable the DPLL, even for a miniscule amount of time. Since the old code did work I suppose it's not really needed, but this seems more consistent with the whole premise of keeping both DPLLs on all the time. I suppose I could extract this to a separate patch for clarity. > > > I915_WRITE(reg, dpll); > > > > /* Wait for the clocks to stabilize. */ > > > @@ -7494,7 +7457,19 @@ static void i8xx_compute_dpll(struct intel_crtc *crtc, > > dpll |= PLL_P2_DIVIDE_BY_4; > > } > > > > - if (!IS_I830(dev_priv) && > > + /* > > + * Bspec: > > + * "[Almador Errata}: For the correct operation of the muxed DVO pins > > + * (GDEVSELB/ I 2 Cdata, GIRDBY/I 2 CClk) and (GFRAMEB/DVI_Data, > > + * GTRDYB/DVI_Clk): Bit 31 (DPLL VCO Enable) and Bit 30 (2X Clock > > + * Enable) must be set to “1” in both the DPLL A Control Register > > + * (06014h-06017h) and DPLL B Control Register (06018h-0601Bh)." > > + * > > + * For simplicity We simply keep both bits always enabled in > > + * both DPLLS. The spec says we should disable the DVO 2X clock > > + * when not needed, but this seems to work fine in practice. > > + */ > > + if (IS_I830(dev_priv) || > > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DVO)) > > dpll |= DPLL_DVO_2X_MODE; > > Is VGA/CRT a native encoder? That might be a useful test to check that > still works. Yes, and yes it still works. > > I couldn't see anything you missed for DPLL_DVO_2X_MODE, so happy that > this does what it says on the tin, and I trust that you actually ran > this on an i830... Indeed I did. And I also tested the 's/0/dpll & ~DPLL_VGA_MODE_DIS/' change on my ctg as those are known to suffer from the p1/p2 dividers not latching issue. The new sequence still forces p1/p2 to latch. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7c5e84ef5171..ac0af94369f1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1441,19 +1441,6 @@ static void chv_enable_pll(struct intel_crtc *crtc, } } -static int intel_num_dvo_pipes(struct drm_i915_private *dev_priv) -{ - struct intel_crtc *crtc; - int count = 0; - - for_each_intel_crtc(&dev_priv->drm, crtc) { - count += crtc->base.state->active && - intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DVO); - } - - return count; -} - static void i9xx_enable_pll(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state) { @@ -1468,26 +1455,12 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv)) assert_panel_unlocked(dev_priv, crtc->pipe); - /* Enable DVO 2x clock on both PLLs if necessary */ - if (IS_I830(dev_priv) && intel_num_dvo_pipes(dev_priv) > 0) { - /* - * It appears to be important that we don't enable this - * for the current pipe before otherwise configuring the - * PLL. No idea how this should be handled if multiple - * DVO outputs are enabled simultaneosly. - */ - dpll |= DPLL_DVO_2X_MODE; - I915_WRITE(DPLL(!crtc->pipe), - I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE); - } - /* * Apparently we need to have VGA mode enabled prior to changing * the P1/P2 dividers. Otherwise the DPLL will keep using the old * dividers, even though the register value does change. */ - I915_WRITE(reg, 0); - + I915_WRITE(reg, dpll & ~DPLL_VGA_MODE_DIS); I915_WRITE(reg, dpll); /* Wait for the clocks to stabilize. */ @@ -1520,16 +1493,6 @@ static void i9xx_disable_pll(const struct intel_crtc_state *crtc_state) struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum pipe pipe = crtc->pipe; - /* Disable DVO 2x clock on both PLLs if necessary */ - if (IS_I830(dev_priv) && - intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DVO) && - !intel_num_dvo_pipes(dev_priv)) { - I915_WRITE(DPLL(PIPE_B), - I915_READ(DPLL(PIPE_B)) & ~DPLL_DVO_2X_MODE); - I915_WRITE(DPLL(PIPE_A), - I915_READ(DPLL(PIPE_A)) & ~DPLL_DVO_2X_MODE); - } - /* Don't disable pipe or pipe PLLs if needed */ if (IS_I830(dev_priv)) return; @@ -7494,7 +7457,19 @@ static void i8xx_compute_dpll(struct intel_crtc *crtc, dpll |= PLL_P2_DIVIDE_BY_4; } - if (!IS_I830(dev_priv) && + /* + * Bspec: + * "[Almador Errata}: For the correct operation of the muxed DVO pins + * (GDEVSELB/ I 2 Cdata, GIRDBY/I 2 CClk) and (GFRAMEB/DVI_Data, + * GTRDYB/DVI_Clk): Bit 31 (DPLL VCO Enable) and Bit 30 (2X Clock + * Enable) must be set to “1” in both the DPLL A Control Register + * (06014h-06017h) and DPLL B Control Register (06018h-0601Bh)." + * + * For simplicity We simply keep both bits always enabled in + * both DPLLS. The spec says we should disable the DVO 2X clock + * when not needed, but this seems to work fine in practice. + */ + if (IS_I830(dev_priv) || intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DVO)) dpll |= DPLL_DVO_2X_MODE; @@ -8218,14 +8193,6 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, } pipe_config->dpll_hw_state.dpll = I915_READ(DPLL(crtc->pipe)); if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) { - /* - * DPLL_DVO_2X_MODE must be enabled for both DPLLs - * on 830. Filter it out here so that we don't - * report errors due to that. - */ - if (IS_I830(dev_priv)) - pipe_config->dpll_hw_state.dpll &= ~DPLL_DVO_2X_MODE; - pipe_config->dpll_hw_state.fp0 = I915_READ(FP0(crtc->pipe)); pipe_config->dpll_hw_state.fp1 = I915_READ(FP1(crtc->pipe)); } else { @@ -15613,7 +15580,7 @@ void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) pipe_name(pipe), clock.vco, clock.dot); fp = i9xx_dpll_compute_fp(&clock); - dpll = (I915_READ(DPLL(pipe)) & DPLL_DVO_2X_MODE) | + dpll = DPLL_DVO_2X_MODE | DPLL_VGA_MODE_DIS | ((clock.p1 - 2) << DPLL_FPA01_P1_POST_DIV_SHIFT) | PLL_P2_DIVIDE_BY_4 |