Re: [PATCH] drm/i915: Simplify i830 DVO 2x clock handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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. 

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx