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

[Tomas Elf]

On 19/02/2015 17:17, 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
> and all further code and simply read it directly from the request structure.
>

"Caching the pointer means the calculation is done one at request 
creation time and all further code and simply read it directly from the 
request structure"

Nitpick: Broken sentence, you might want to fix that. Aside from that, 
no major problems with this patch.

Reviewed-by: Tomas Elf <tomas.elf at intel.com>

Thanks,
Tomas

> 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 2dedd43..ba09137 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2144,8 +2144,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 61134ab..7a0dc7c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2763,7 +2763,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,
> @@ -2774,23 +2773,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 637cbb7..f14b517 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -888,12 +888,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.
>   	 */
>   	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 ca7de3d..7fd89e5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2179,6 +2179,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);
>