[Intel-gfx] [PATCH 2/2] drm/i915: Enable DPLL VGA mode before P1/P2 divider write

[Chris Wilson]

On Thu, Oct 08, 2015 at 10:19:14AM +0200, Daniel Vetter wrote:
> On Wed, Oct 07, 2015 at 10:08:25PM +0300, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Apparently writing the DPLL register P1/P2 divider fields won't trigger
> > an actual change in the DPLL output unless VGA mode is enabled for
> > prior to the register write that changes the P1/P2 dividers. The write
> > with the new P1/P2 divider can itself disable VGA mode again without
> > problems.
> > 
> > I tested the behaviour on my 946GZ, and when manually frobbing the
> > register with the display on, the behaviour is very clear. However I
> > can't explain why this machine actually works. The P1/P2 divider
> > changes caused by normal modesets do seem to make it through to the
> > hardware somehow since I get a stable picture on the monitor with
> > any resolution. Maybe it's the "three times for luck" stuff that
> > somehow masks the problem, or something.
> > 
> > But apparently there are machines (eg. Nick Bowler's G45) where that
> > isn't the case and we fail to get the correct clock from the DPLL.
> > 
> > Things used to work because we enabled VGA mode for disabled DPLLs,
> > so when re-enabling the DPLL VGA mode was enabled just prior to the
> > first register write, and hence the P1/P2 change went through without
> > a hitch. That got changed in
> > 
> > b8afb9113c51 drm/i915: Keep GMCH DPLL VGA mode always disabled
> > 
> > in the name of consistency. In order to keep the consistency part,
> > leave VGA mode disabled for disabled DPLLs, but turn it on just prior
> > to updating the P1/P2 dividers to make sure the hardware picks up
> > on the new values.
> > 
> > Cc: Nick Bowler <nbowler at draconx.ca>
> > Reported-by: Nick Bowler <nbowler at draconx.ca>
> > Tested-by: Nick Bowler <nbowler at draconx.ca>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f4fdff9..ce51dbc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1743,6 +1743,13 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >  			   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);
> 
> Again POSTING_READ for ordering? Otherwise Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Not that there's a lot to review since Bspec is silent ...

One option is to start using mmiowb() for documenting the write ordering
(which is a no-op on x86 and so heaven forbid we get to use WC ever.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre