On 11/01/16 09:16, Chris Wilson wrote:
If we move the release of the GEM request (i.e. decoupling it from the
various lists used for client and context tracking) after it is complete
(either by the GPU retiring the request, or by the caller cancelling the
request), we can remove the requirement that the final unreference of
the GEM request need to be under the struct_mutex.
v2: Execlists as always is badly asymetric and year old patches still
haven't landed to fix it up.
Looks good and pretty standalone to me (depends on i915_gem_request code
movement only I think). Just one question below.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 4 +--
drivers/gpu/drm/i915/i915_gem_request.c | 50 ++++++++++++++------------------
drivers/gpu/drm/i915/i915_gem_request.h | 14 ---------
drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 6 ++--
drivers/gpu/drm/i915/intel_pm.c | 2 +-
7 files changed, 30 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 68a25617ca7a..6d8d65304abf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2502,7 +2502,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
ret = __i915_wait_request(req[i], true,
args->timeout_ns > 0 ? &args->timeout_ns : NULL,
to_rps_client(file));
- i915_gem_request_unreference__unlocked(req[i]);
+ i915_gem_request_unreference(req[i]);
}
return ret;
@@ -3505,7 +3505,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
return 0;
ret = __i915_wait_request(target, true, NULL, NULL);
- i915_gem_request_unreference__unlocked(target);
+ i915_gem_request_unreference(target);
return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index b4ede6dd7b20..1c4f4d83a3c2 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -184,13 +184,6 @@ err:
return ret;
}
-void i915_gem_request_cancel(struct drm_i915_gem_request *req)
-{
- intel_ring_reserved_space_cancel(req->ringbuf);
-
- i915_gem_request_unreference(req);
-}
-
int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
struct drm_file *file)
{
@@ -235,9 +228,28 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
request->pid = NULL;
}
+static void __i915_gem_request_release(struct drm_i915_gem_request *request)
+{
+ i915_gem_request_remove_from_client(request);
+
+ i915_gem_context_unreference(request->ctx);
+ i915_gem_request_unreference(request);
+}
+
+void i915_gem_request_cancel(struct drm_i915_gem_request *req)
+{
+ intel_ring_reserved_space_cancel(req->ringbuf);
+ if (i915.enable_execlists) {
+ if (req->ctx != req->ring->default_context)
+ intel_lr_context_unpin(req);
+ }
+ __i915_gem_request_release(req);
+}
+
static void i915_gem_request_retire(struct drm_i915_gem_request *request)
{
trace_i915_gem_request_retire(request);
+ list_del_init(&request->list);
/* We know the GPU must have read the request to have
* sent us the seqno + interrupt, so use the position
@@ -248,11 +260,7 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
* completion order.
*/
request->ringbuf->last_retired_head = request->postfix;
-
- list_del_init(&request->list);
- i915_gem_request_remove_from_client(request);
-
- i915_gem_request_unreference(request);
+ __i915_gem_request_release(request);
}
void
@@ -639,21 +647,7 @@ i915_wait_request(struct drm_i915_gem_request *req)
void i915_gem_request_free(struct kref *req_ref)
{
- struct drm_i915_gem_request *req = container_of(req_ref,
- typeof(*req), ref);
- struct intel_context *ctx = req->ctx;
-
- if (req->file_priv)
- i915_gem_request_remove_from_client(req);
-
- if (ctx) {
- if (i915.enable_execlists) {
- if (ctx != req->ring->default_context)
- intel_lr_context_unpin(req);
- }
-
- i915_gem_context_unreference(ctx);
- }
-
+ struct drm_i915_gem_request *req =
+ container_of(req_ref, typeof(*req), ref);
kmem_cache_free(req->i915->requests, req);
}
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index d46f22f30b0a..af1b825fce50 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -154,23 +154,9 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
static inline void
i915_gem_request_unreference(struct drm_i915_gem_request *req)
{
- WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
kref_put(&req->ref, i915_gem_request_free);
}
-static inline void
-i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
-{
- struct drm_device *dev;
-
- if (!req)
- return;
-
- dev = req->ring->dev;
- if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
- mutex_unlock(&dev->struct_mutex);
-}
-
static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
struct drm_i915_gem_request *src)
{
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 0ea01bd6811c..f6731aac7fcf 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -390,7 +390,7 @@ static int intel_breadcrumbs_signaler(void *arg)
*/
intel_engine_remove_wait(engine, &signal->wait);
- i915_gem_request_unreference__unlocked(signal->request);
+ i915_gem_request_unreference(signal->request);
/* Find the next oldest signal. Note that as we have
* not been holding the lock, another client may
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 57c54c9bc82b..32885b8d5c02 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11431,7 +11431,7 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
WARN_ON(__i915_wait_request(mmio_flip->req,
false, NULL,
&mmio_flip->i915->rps.mmioflips));
- i915_gem_request_unreference__unlocked(mmio_flip->req);
+ i915_gem_request_unreference(mmio_flip->req);
}
/* For framebuffer backed by dmabuf, wait for fence */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b634e7d7a92b..7a3069a2beb2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -587,9 +587,6 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- if (request->ctx != ring->default_context)
- intel_lr_context_pin(request);
-
Since you remove LRC pin from queue, the lifetime is now either:
1. From request create to cancel.
2. From request create to execlist retirement.
Would it be more logical to leave the LRC pin in queue, but remove it
from request creation instead? That would make the LRC pin lifetime only
a single possibility, from queue to execlist retire.
i915_gem_request_reference(request);
spin_lock_irq(&ring->execlist_lock);
@@ -1071,6 +1068,8 @@ static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
ret = intel_lr_context_do_pin(ring, ctx_obj, ringbuf);
if (ret)
goto reset_pin_count;
+
+ i915_gem_context_reference(rq->ctx);
}
return ret;
@@ -1090,6 +1089,7 @@ void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
if (--rq->ctx->engine[ring->id].pin_count == 0) {
intel_unpin_ringbuffer_obj(ringbuf);
i915_gem_object_ggtt_unpin(ctx_obj);
+ i915_gem_context_unreference(rq->ctx);
}
}
}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e51ba529a97e..0e13135aefaa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7289,7 +7289,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
gen6_rps_boost(to_i915(req->ring->dev), NULL,
req->emitted_jiffies);
- i915_gem_request_unreference__unlocked(req);
+ i915_gem_request_unreference(req);
kfree(boost);
}
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
