[Intel-gfx] [PATCH 03/51] drm/i915: Cache ringbuf pointer in request structure

[Daniel Vetter]

On Fri, Feb 13, 2015 at 11:48:12AM +0000, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
> 
> In execlist mode, the ringbuf is a function of the ring and context whereas in
> legacy mode, it is derived from the ring alone. Thus the calculation required to
> determine the ringbuf pointer from the ring (and context) also needs to test
> execlist mode or not. This is messy.
> 
> Further, the request structure holds a pointer to both the ring and the context
> for which it was created. Thus, given a request, it is possible to derive the
> ringbuf in either legacy or execlist mode. Hence it is necessary to pass just
> the request in to all the low level functions rather than some combination of
> request, ring, context and ringbuf. However, rather than recalculating it each
> time, it is much simpler to just cache the ringbuf pointer in the request
> structure itself.
> 
> Caching the pointer means the calculation is done one at request creation time

s/one/once/ I guess?

> and all further code and simply read it directly from the request structure.
> 
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    3 ++-
>  drivers/gpu/drm/i915/i915_gem.c         |   14 +-------------
>  drivers/gpu/drm/i915/intel_lrc.c        |    6 ++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    1 +
>  4 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f2a825e..e90b786 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2137,8 +2137,9 @@ struct drm_i915_gem_request {
>  	/** Position in the ringbuffer of the end of the whole request */
>  	u32 tail;
>  
> -	/** Context related to this request */
> +	/** Context and ring buffer related to this request */
>  	struct intel_context *ctx;
> +	struct intel_ringbuffer *ringbuf;
>  
>  	/** Batch buffer related to this request if any */
>  	struct drm_i915_gem_object *batch_obj;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c26d36c..2ebe914 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2758,7 +2758,6 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  
>  	while (!list_empty(&ring->request_list)) {
>  		struct drm_i915_gem_request *request;
> -		struct intel_ringbuffer *ringbuf;
>  
>  		request = list_first_entry(&ring->request_list,
>  					   struct drm_i915_gem_request,
> @@ -2769,23 +2768,12 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  
>  		trace_i915_gem_request_retire(request);
>  
> -		/* This is one of the few common intersection points
> -		 * between legacy ringbuffer submission and execlists:
> -		 * we need to tell them apart in order to find the correct
> -		 * ringbuffer to which the request belongs to.
> -		 */
> -		if (i915.enable_execlists) {
> -			struct intel_context *ctx = request->ctx;
> -			ringbuf = ctx->engine[ring->id].ringbuf;
> -		} else
> -			ringbuf = ring->buffer;
> -
>  		/* We know the GPU must have read the request to have
>  		 * sent us the seqno + interrupt, so use the position
>  		 * of tail of the request to update the last known position
>  		 * of the GPU head.
>  		 */
> -		ringbuf->last_retired_head = request->postfix;
> +		request->ringbuf->last_retired_head = request->postfix;
>  
>  		i915_gem_free_request(request);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 73c1861..762136b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -878,12 +878,14 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
>  		return ret;
>  	}
>  
> -	/* Hold a reference to the context this request belongs to
> +	/*
> +	 * Hold a reference to the context this request belongs to
>  	 * (we will need it when the time comes to emit/retire the
> -	 * request).
> +	 * request). Likewise, the ringbuff is useful to keep track of.
>  	 */

Imo this comment is a bit too obvious so I just deleted it ;-)
-Daniel

>  	request->ctx = ctx;
>  	i915_gem_context_reference(request->ctx);
> +	request->ringbuf = ctx->engine[ring->id].ringbuf;
>  
>  	ring->outstanding_lazy_request = request;
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d611608..ca9e7e6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2100,6 +2100,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
>  
>  	kref_init(&request->ref);
>  	request->ring = ring;
> +	request->ringbuf = ring->buffer;
>  	request->uniq = dev_private->request_uniq++;
>  
>  	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch