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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center