* [PATCH] drm/i915: Free requests after object release when retiring requests
@ 2014-01-07 11:45 Chris Wilson
2014-01-09 23:39 ` Ben Widawsky
0 siblings, 1 reply; 2+ messages in thread
From: Chris Wilson @ 2014-01-07 11:45 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
Freeing a request triggers the destruction of the context. This needs to
occur after all objects are themselves unbound from the context, and so
the free request needs to occur after the object release during retire.
This tidies up
commit e20780439b26ba95aeb29d3e27cd8cc32bc82a4c
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Fri Dec 6 14:11:22 2013 -0800
drm/i915: Defer request freeing
by simply swapping the order of operations rather than introducing
further complexity - as noted during review.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem.c | 47 ++++++++++++++++++-----------------------
1 file changed, 21 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a5579a317b85..0a2055b736c4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2560,8 +2560,6 @@ void i915_gem_reset(struct drm_device *dev)
void
i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
{
- LIST_HEAD(deferred_request_free);
- struct drm_i915_gem_request *request;
uint32_t seqno;
if (list_empty(&ring->request_list))
@@ -2571,7 +2569,27 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
seqno = ring->get_seqno(ring, true);
+ /* Move any buffers on the active list that are no longer referenced
+ * by the ringbuffer to the flushing/inactive lists as appropriate,
+ * before we free the context associated with the requests.
+ */
+ while (!list_empty(&ring->active_list)) {
+ struct drm_i915_gem_object *obj;
+
+ obj = list_first_entry(&ring->active_list,
+ struct drm_i915_gem_object,
+ ring_list);
+
+ if (!i915_seqno_passed(seqno, obj->last_read_seqno))
+ break;
+
+ i915_gem_object_move_to_inactive(obj);
+ }
+
+
while (!list_empty(&ring->request_list)) {
+ struct drm_i915_gem_request *request;
+
request = list_first_entry(&ring->request_list,
struct drm_i915_gem_request,
list);
@@ -2587,23 +2605,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
*/
ring->last_retired_head = request->tail;
- list_move_tail(&request->list, &deferred_request_free);
- }
-
- /* Move any buffers on the active list that are no longer referenced
- * by the ringbuffer to the flushing/inactive lists as appropriate.
- */
- while (!list_empty(&ring->active_list)) {
- struct drm_i915_gem_object *obj;
-
- obj = list_first_entry(&ring->active_list,
- struct drm_i915_gem_object,
- ring_list);
-
- if (!i915_seqno_passed(seqno, obj->last_read_seqno))
- break;
-
- i915_gem_object_move_to_inactive(obj);
+ i915_gem_free_request(request);
}
if (unlikely(ring->trace_irq_seqno &&
@@ -2612,13 +2614,6 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
ring->trace_irq_seqno = 0;
}
- /* Finish processing active list before freeing request */
- while (!list_empty(&deferred_request_free)) {
- request = list_first_entry(&deferred_request_free,
- struct drm_i915_gem_request,
- list);
- i915_gem_free_request(request);
- }
WARN_ON(i915_verify_lists(ring->dev));
}
--
1.8.5.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drm/i915: Free requests after object release when retiring requests
2014-01-07 11:45 [PATCH] drm/i915: Free requests after object release when retiring requests Chris Wilson
@ 2014-01-09 23:39 ` Ben Widawsky
0 siblings, 0 replies; 2+ messages in thread
From: Ben Widawsky @ 2014-01-09 23:39 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Jan 07, 2014 at 11:45:14AM +0000, Chris Wilson wrote:
> Freeing a request triggers the destruction of the context. This needs to
> occur after all objects are themselves unbound from the context, and so
> the free request needs to occur after the object release during retire.
>
> This tidies up
>
> commit e20780439b26ba95aeb29d3e27cd8cc32bc82a4c
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Fri Dec 6 14:11:22 2013 -0800
>
> drm/i915: Defer request freeing
>
> by simply swapping the order of operations rather than introducing
> further complexity - as noted during review.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 47 ++++++++++++++++++-----------------------
> 1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a5579a317b85..0a2055b736c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2560,8 +2560,6 @@ void i915_gem_reset(struct drm_device *dev)
> void
> i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> {
> - LIST_HEAD(deferred_request_free);
> - struct drm_i915_gem_request *request;
> uint32_t seqno;
>
> if (list_empty(&ring->request_list))
> @@ -2571,7 +2569,27 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>
> seqno = ring->get_seqno(ring, true);
>
> + /* Move any buffers on the active list that are no longer referenced
> + * by the ringbuffer to the flushing/inactive lists as appropriate,
> + * before we free the context associated with the requests.
> + */
> + while (!list_empty(&ring->active_list)) {
> + struct drm_i915_gem_object *obj;
> +
> + obj = list_first_entry(&ring->active_list,
> + struct drm_i915_gem_object,
> + ring_list);
> +
> + if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> + break;
> +
> + i915_gem_object_move_to_inactive(obj);
> + }
> +
> +
> while (!list_empty(&ring->request_list)) {
> + struct drm_i915_gem_request *request;
> +
> request = list_first_entry(&ring->request_list,
> struct drm_i915_gem_request,
> list);
> @@ -2587,23 +2605,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> */
> ring->last_retired_head = request->tail;
>
> - list_move_tail(&request->list, &deferred_request_free);
> - }
> -
> - /* Move any buffers on the active list that are no longer referenced
> - * by the ringbuffer to the flushing/inactive lists as appropriate.
> - */
> - while (!list_empty(&ring->active_list)) {
> - struct drm_i915_gem_object *obj;
> -
> - obj = list_first_entry(&ring->active_list,
> - struct drm_i915_gem_object,
> - ring_list);
> -
> - if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> - break;
> -
> - i915_gem_object_move_to_inactive(obj);
> + i915_gem_free_request(request);
> }
>
> if (unlikely(ring->trace_irq_seqno &&
> @@ -2612,13 +2614,6 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> ring->trace_irq_seqno = 0;
> }
>
> - /* Finish processing active list before freeing request */
> - while (!list_empty(&deferred_request_free)) {
> - request = list_first_entry(&deferred_request_free,
> - struct drm_i915_gem_request,
> - list);
> - i915_gem_free_request(request);
> - }
> WARN_ON(i915_verify_lists(ring->dev));
> }
>
> --
> 1.8.5.2
>
I had a reason for not doing this originally - but I can no longer
reconstruct what it was. Looking at it again now, it seems the only
difference is with setting ring->last_retired_head, with your patch that
gets set after the object is on the inactive list. Nothing seems wrong
with that, to me.
So with the caveat that I thought this was a bad idea at one point in
time:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-01-09 23:39 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-07 11:45 [PATCH] drm/i915: Free requests after object release when retiring requests Chris Wilson
2014-01-09 23:39 ` Ben Widawsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox