[Intel-gfx] [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail

[Tvrtko Ursulin]

On 08/06/2018 18:25, Chris Wilson wrote:
> The HW only accepts offsets within ring->size, and fails peculiarly if
> the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
> set ring->head/ring->tail we want to make sure it is within value (using
> intel_ring_wrap()).
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld at gmail.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c |  5 +++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6ac3b65373fe..9fac0e0f078e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
>   		DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
>   				 engine->name, I915_READ_HEAD(engine));
>   
> +	/* Check that the ring offsets point within the ring! */
> +	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> +	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
> +
>   	intel_ring_update_space(ring);
>   	I915_WRITE_HEAD(engine, ring->head);
>   	I915_WRITE_TAIL(engine, ring->tail);
> @@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
>   
>   void intel_ring_reset(struct intel_ring *ring, u32 tail)
>   {
> +	tail = intel_ring_wrap(ring, tail);
>   	ring->tail = tail;
>   	ring->head = tail;
>   	ring->emit = tail;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b44c67849749..1d8140ac2016 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
>   	return pos & (ring->size - 1);
>   }
>   
> +static inline bool
> +intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
> +{
> +	if (pos & -ring->size) /* must be strictly within the ring */
> +		return false;
> +
> +	if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
> +		return false;
> +
> +	return true;
> +}

Looks like we have assert_ring_tail_valid and intel_ring_set_tail 
already in the tree. So could just use the latter in intel_ring_reset if 
needed. But also since intel_ring_reset is only setting the tail to 
either zero or existing ring->tail it sounds like the check would be 
better placed where the tail advances?

Regards,

Tvrtko

> +
>   static inline u32 intel_ring_offset(const struct i915_request *rq, void *addr)
>   {
>   	/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
>