Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 13, 2014 at 01:10:37PM +0000, Chris Wilson wrote:
> On Thu, Mar 13, 2014 at 03:00:58PM +0200, Mika Kuoppala wrote:
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int	ret;
> > > -	unsigned long irqflags;
> > > -	u32 fw_engine = 0;
> > > -
> > > -	/* Hold uncore.lock across reset to prevent any register access
> > > -	 * with forcewake not set correctly
> > > -	 */
> > > -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > >  
> > >  	/* Reset the chip */
> > >  
> > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev)
> > >  	/* Spin waiting for the device to ack the reset request */
> > >  	ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500);
> > 
> > We used to go through the reset with uncore lock held,
> > which is not the case anymore. Will it create problems?
> 
> I don't think so. gen6_do_reset() is itself serialised and there should
> be no other access to GDRST anyway, so from that perspective we won't
> trigger hw errors. So the only thing that the uncore lock was protecting
> in this function is the forcewake dance.

Yes, protecting the forcewake dance was the only reason to hold the uncore
lock over gen6 reset. As long as we hold the uncore lock while restoring
any forcewake references after reset we're good, no need to shove the
entire reset under that spinlock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch