[Intel-gfx] [PATCH] drm/i915: Poison rings after use

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> On retiring the request, we should not re-use these elements in the ring
> (at least not until we fill the ringbuffer and knowingly reuse the space).
> Leave behind some poison to (hopefully) trap ourselves if we make a
> mistake.
>
> Suggested-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_request.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 0ecc2cf64216..9ee7bf0200b0 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -203,6 +203,19 @@ static void free_capture_list(struct i915_request *request)
>  	}
>  }
>  
> +static void __i915_request_fill(struct i915_request *rq, u8 val)
> +{
> +	void *vaddr = rq->ring->vaddr;
> +	u32 head;
> +
> +	head = rq->infix;
> +	if (rq->postfix < head) {
> +		memset(vaddr + head, val, rq->ring->size - head);
> +		head = 0;
> +	}
> +	memset(vaddr + head, val, rq->postfix - head);
> +}
> +
>  static void remove_from_engine(struct i915_request *rq)
>  {
>  	struct intel_engine_cs *engine, *locked;
> @@ -247,6 +260,9 @@ bool i915_request_retire(struct i915_request *rq)
>  	 */
>  	GEM_BUG_ON(!list_is_first(&rq->link,
>  				  &i915_request_timeline(rq)->requests));
> +	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> +		/* Poison before we release our space in the ring */
> +		__i915_request_fill(rq, POISON_FREE);

Would it be too detrimental for perf to check for POISON_FREE when
we emit the requests?

I think it is a positive problem that we are in brink
of a DEBUG_GEM_LEVEL split :O

Regardless, with this we get gpu helping on revealing
our bookkeepping failures.

Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>

>  	rq->ring->head = rq->postfix;
>  
>  	/*
> @@ -1175,9 +1191,6 @@ i915_request_await_object(struct i915_request *to,
>  
>  void i915_request_skip(struct i915_request *rq, int error)
>  {
> -	void *vaddr = rq->ring->vaddr;
> -	u32 head;
> -
>  	GEM_BUG_ON(!IS_ERR_VALUE((long)error));
>  	dma_fence_set_error(&rq->fence, error);
>  
> @@ -1189,12 +1202,7 @@ void i915_request_skip(struct i915_request *rq, int error)
>  	 * context, clear out all the user operations leaving the
>  	 * breadcrumb at the end (so we get the fence notifications).
>  	 */
> -	head = rq->infix;
> -	if (rq->postfix < head) {
> -		memset(vaddr + head, 0, rq->ring->size - head);
> -		head = 0;
> -	}
> -	memset(vaddr + head, 0, rq->postfix - head);
> +	__i915_request_fill(rq, 0);
>  	rq->infix = rq->postfix;
>  }
>  
> -- 
> 2.25.0