Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

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

On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
> > From: Deepak S <deepak.s@linux.intel.com>
> > 
> > In chv, we have two power wells Render & Media. We need to use
> > corresponsing forcewake count. If we dont follow this we are getting
> > error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
> > multiple entry into __vlv_force_wake_get.
> > 
> > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index bd1b28d..bafd38b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> >  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> >  	 */
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -	if (dev_priv->uncore.forcewake_count++ == 0)
> > -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > +		if (dev_priv->uncore.fw_rendercount++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_RENDER);
> > +		if (dev_priv->uncore.fw_mediacount++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_MEDIA);
> 
> This will wake both wells. Is that needed or should we just pick one
> based on the ring?

Also unlike the comment says runtime_pm_get() can't sleep since someone
must already be holding a reference, othwewise we surely can't go
writing any registers. So in theory we should be able to call
gen6_gt_force_wake_get() here, but maybe that would trigger a
might_sleep() warning. the current force wake code duplication (esp.
outside intel_uncore.c) is rather unfortunate and I'd like to see it
killed off. Maybe we just need to pull the rpm get/put outside
gen6_gt_force_wake_get()? I never really liked hiding it there anyway.

> 
> > +	} else {
> > +		if (dev_priv->uncore.forcewake_count++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_ALL);
> > +	}
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> >  
> >  	I915_WRITE(RING_ELSP(ring), desc[1]);
> > @@ -315,8 +325,19 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> >  
> >  	/* Release Force Wakeup (see the big comment above). */
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -	if (--dev_priv->uncore.forcewake_count == 0)
> > -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > +		if (--dev_priv->uncore.fw_rendercount == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_RENDER);
> > +		if (--dev_priv->uncore.fw_mediacount == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_MEDIA);
> > +	} else {
> > +		if (--dev_priv->uncore.forcewake_count == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_ALL);
> > +	}
> > +
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> >  }
> >  
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC