Re: [PATCH 03/53] drm/i915: Cache ringbuf pointer in request structure

Tomas Elf
2015-03-05T13:56:15Z

On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@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@intel.com>

Thanks,
Tomas

> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@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);
>