Re: [Intel-gfx] [PATCH 03/53] drm/i915: Cache ringbuf pointer in request structure
On 05/03/2015 13:56, Tomas Elf wrote:
On 19/02/2015 17:17, john.c.harri...@intel.com wrote:
From: John Harrison
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
Thanks,
Tomas
For: VIZ-5115
Signed-off-by: John Harrison
---
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)
kr
Re: [Intel-gfx] [PATCH 03/53] drm/i915: Cache ringbuf pointer in request structure
On 19/02/2015 17:17, john.c.harri...@intel.com wrote:
From: John Harrison
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
Thanks,
Tomas
For: VIZ-5115
Signed-off-by: John Harrison
---
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/drive
[Intel-gfx] [PATCH 03/53] drm/i915: Cache ringbuf pointer in request structure
From: John Harrison
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.
For: VIZ-5115
Signed-off-by: John Harrison
---
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);
--
1.7.9.5
