| Message ID | 1424366285-29232-4-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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); >> >
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);