Re: [PATCH 05/10] [v2] drm/i915: Don't initialize watermark stuff with PCH_NOP

From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, "Mcallister,
	Jeffrey" <jeffrey.mcallister@intel.com>
Subject: Re: [PATCH 05/10] [v2] drm/i915: Don't initialize watermark stuff with PCH_NOP
Date: Mon, 18 Mar 2013 17:45:16 -0700	[thread overview]
Message-ID: <20130319004516.GA10155@bwidawsk.net> (raw)
In-Reply-To: <20130317210244.GW9021@phenom.ffwll.local>

On Sun, Mar 17, 2013 at 10:02:44PM +0100, Daniel Vetter wrote:
> On Wed, Mar 13, 2013 at 02:06:12PM -0700, Ben Widawsky wrote:
> > v2: Move check to the top (Chris)
> > Add BUG_ON for !ivybridge, since it's all we support for now (Ben)
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 52203fd..8e7908b 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4153,7 +4153,12 @@ void intel_init_pm(struct drm_device *dev)
> >  		i915_ironlake_get_mem_freq(dev);
> >  
> >  	/* For FIFO watermark updates */
> > -	if (HAS_PCH_SPLIT(dev)) {
> > +	if (HAS_PCH_NOP(dev)) {
> > +		BUG_ON(!IS_IVYBRIDGE(dev));
> > +		dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
> > +		dev_priv->display.update_wm = NULL;
> > +		dev_priv->display.update_sprite_wm = NULL;
> > +	} else if (HAS_PCH_SPLIT(dev)) {
> >  		if (IS_GEN5(dev)) {
> >  			if (I915_READ(MLTR_ILK) & ILK_SRLT_MASK)
> >  				dev_priv->display.update_wm = ironlake_update_wm;
> > @@ -4175,7 +4180,7 @@ void intel_init_pm(struct drm_device *dev)
> >  			dev_priv->display.init_clock_gating = gen6_init_clock_gating;
> >  		} else if (IS_IVYBRIDGE(dev)) {
> >  			/* FIXME: detect B0+ stepping and use auto training */
> > -			if (SNB_READ_WM0_LATENCY()) {
> > +			if (SNB_READ_WM0_LATENCY() && !HAS_PCH_NOP(dev)) {
> >  				dev_priv->display.update_wm = ivybridge_update_wm;
> >  				dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
> >  			} else {
> 
> I'm confused why we need this patch here - update_wm functions should only
> be called when we have a pipe. If there's any caller left I think we
> should fix those up, not paper over it here.
> 
> And imo it's ok to have non-NULL vfuncs here (or anywhere else, e.g.
> pageflips) as long as we don't call them. After all the num_pips/PCH_NOP
> checks are here to make this as unobtrusive as possible.
> -Daniel

I think you're right, so I've dropped this patch entirely.

-- 
Ben Widawsky, Intel Open Source Technology Center