Re: [PATCH 3/4] drm/i915: Store HPLL frequency in dev_priv on VLV

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 06, 2013 at 10:41:22AM +0200, Ville Syrjälä wrote:
> On Tue, Nov 05, 2013 at 01:02:58PM -0800, Jesse Barnes wrote:
> > On Tue,  5 Nov 2013 22:42:30 +0200
> > ville.syrjala@linux.intel.com wrote:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Keep the HPLL frequencey in dev_priv on VLV instead of reading
> > > it from CCK every time it's needed.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      | 2 +-
> > >  drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4bae871..dd40925 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1360,7 +1360,7 @@ typedef struct drm_i915_private {
> > >  	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
> > >  	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
> > >  
> > > -	unsigned int fsb_freq, mem_freq, is_ddr3;
> > > +	unsigned int fsb_freq, mem_freq, is_ddr3, hpll_vco;
> > >  
> > >  	/**
> > >  	 * wq - Driver workqueue for GEM.
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 48f4990..f97e895 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3898,13 +3898,18 @@ int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > >  {
> > >  	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> > >  
> > > +	if (dev_priv->hpll_vco)
> > > +		return dev_priv->hpll_vco;
> > > +
> > >  	/* Obtain SKU information */
> > >  	mutex_lock(&dev_priv->dpio_lock);
> > >  	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> > >  		CCK_FUSE_HPLL_FREQ_MASK;
> > >  	mutex_unlock(&dev_priv->dpio_lock);
> > >  
> > > -	return vco_freq[hpll_freq];
> > > +	dev_priv->hpll_vco = vco_freq[hpll_freq];
> > > +
> > > +	return dev_priv->hpll_vco;
> > >  }
> > >  
> > >  /* Adjust CDclk dividers to allow high res or save power if possible */
> > 
> > I'd just move this to init_clock_gating or something, then use
> > dev_priv->hpll_vco everywhere, rather than this conditional lazy
> > initialization.
> 
> The problem was the we need it at gmbus init time, and we do that very
> early. So I couldn't figure out a nice place to stick it, and so
> I ended up doing the lazy thing.
> 
> I suspect the best thing to do would be to move gmbus init to happen
> later, alongside other modeset setup. But I'm feeling a bit lazy and
> don't want to tackle that task right now.
> 
> Anyways, since we don't want patch 4/4, I think for now we can just
> drop this one as well. The places where we call it currently aren't
> really that frequent or performance sensitive.

init ordering hell strikes again. If we opt for a fixed order (the lazy
ordering seems to not be too evil though in this case) I'd like to have a
big WARN in there in case we get it wrong.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch