Re: [PATCH 2/6] drm/i915: Move all ring resets before setting the HWS page

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 02, 2014 at 02:58:54PM -0700, Jesse Barnes wrote:
> On Wed,  2 Apr 2014 16:36:07 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> > Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > Date:   Wed Mar 12 16:39:40 2014 +0530
> > 
> >     drm/i915: disable rings before HW status page setup
> > 
> > we reordered stopping the rings to do so before we set the HWS register.
> > However, there is an extra workaround for g45 to reset the rings twice,
> > and for consistency we should apply that workaround before setting the
> > HWS to be sure that the rings are truly stopped.
> > 
> > Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  1 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 54 +++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
> >  3 files changed, 36 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 393f93ecd41a..6dd35f849781 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -915,6 +915,7 @@ enum punit_power_well {
> >  # define MI_FLUSH_ENABLE				(1 << 12)
> >  # define ASYNC_FLIP_PERF_DISABLE			(1 << 14)
> >  # define MODE_IDLE					(1 << 9)
> > +# define STOP_RING					(1 << 8)
> >  
> >  #define GEN6_GT_MODE	0x20d0
> >  #define GEN7_GT_MODE	0x7008
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 475391ce671a..98ba7d594718 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -439,32 +439,41 @@ static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
> >  	I915_WRITE(HWS_PGA, addr);
> >  }
> >  
> > -static int init_ring_common(struct intel_ring_buffer *ring)
> > +static bool stop_ring(struct intel_ring_buffer *ring)
> >  {
> > -	struct drm_device *dev = ring->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct drm_i915_gem_object *obj = ring->obj;
> > -	int ret = 0;
> > -	u32 head;
> > +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> >  
> > -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	if (!IS_GEN2(ring->dev)) {
> > +		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
> > +		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
> > +			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> > +			return false;
> > +		}
> > +	}
> >  
> > -	/* Stop the ring if it's running. */
> >  	I915_WRITE_CTL(ring, 0);
> >  	I915_WRITE_HEAD(ring, 0);
> >  	ring->write_tail(ring, 0);
> > -	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
> > -		DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> >  
> > -	if (I915_NEED_GFX_HWS(dev))
> > -		intel_ring_setup_status_page(ring);
> > -	else
> > -		ring_setup_phys_status_page(ring);
> > +	if (!IS_GEN2(ring->dev)) {
> > +		(void)I915_READ_CTL(ring);
> > +		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
> > +	}
> >  
> > -	head = I915_READ_HEAD(ring) & HEAD_ADDR;
> > +	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
> > +}
> >  
> > -	/* G45 ring initialization fails to reset head to zero */
> > -	if (head != 0) {
> > +static int init_ring_common(struct intel_ring_buffer *ring)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_gem_object *obj = ring->obj;
> > +	int ret = 0;
> > +
> > +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +	if (!stop_ring(ring)) {
> > +		/* G45 ring initialization often fails to reset head to zero */
> >  		DRM_DEBUG_KMS("%s head not reset to zero "
> >  			      "ctl %08x head %08x tail %08x start %08x\n",
> >  			      ring->name,
> > @@ -473,9 +482,7 @@ static int init_ring_common(struct intel_ring_buffer *ring)
> >  			      I915_READ_TAIL(ring),
> >  			      I915_READ_START(ring));
> >  
> > -		I915_WRITE_HEAD(ring, 0);
> > -
> > -		if (I915_READ_HEAD(ring) & HEAD_ADDR) {
> > +		if (!stop_ring(ring)) {
> >  			DRM_ERROR("failed to set %s head to zero "
> >  				  "ctl %08x head %08x tail %08x start %08x\n",
> >  				  ring->name,
> > @@ -483,9 +490,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
> >  				  I915_READ_HEAD(ring),
> >  				  I915_READ_TAIL(ring),
> >  				  I915_READ_START(ring));
> > +			ret = -EIO;
> > +			goto out;
> >  		}
> >  	}
> >  
> > +	if (I915_NEED_GFX_HWS(dev))
> > +		intel_ring_setup_status_page(ring);
> > +	else
> > +		ring_setup_phys_status_page(ring);
> > +
> >  	/* Initialize the ring. This must happen _after_ we've cleared the ring
> >  	 * registers with the above sequence (the readback of the HEAD registers
> >  	 * also enforces ordering), otherwise the hw might lose the new ring
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index 270a6a973438..2b91c4b4d34b 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -34,6 +34,7 @@ struct  intel_hw_status_page {
> >  #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
> >  
> >  #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
> > +#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
> >  
> >  enum intel_ring_hangcheck_action {
> >  	HANGCHECK_IDLE = 0,
> 
> Bad Chris, mixing a nice refactor and a nice fix in the same patch.
> I'll still give you a cookie though.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch. I'll let the other settle a bit
more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch