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

John Harrison
2015-03-06T12:28:19Z

On 05/03/2015 13:56, Tomas Elf wrote:
> 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.
Bit late now, it has already been merged as is.

>
> 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);
>>
>