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

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 05/03/2015 13:56, Tomas Elf wrote:
> On 19/02/2015 17:17, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> Thanks,
> Tomas
>
> > For: VIZ-5115
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx