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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > --- > > @@ -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@xxxxxxxxxxxxxxxxxx> Thanks. -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
