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

[John Harrison <John.C.Harrison@Intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx