diff mbox

[RFC,1/4] drm/i915: Move releasing of the GEM request from free to retire/cancel

Message ID 1460123698-16832-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin April 8, 2016, 1:54 p.m. UTC 
From: Chris Wilson <chris@chris-wilson.co.uk>

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.

v3: Extracted, rebased and fixed for GuC. (Tvrtko Ursulin)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 14 ----------
 drivers/gpu/drm/i915/i915_gem.c      | 50 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      |  2 +-
 4 files changed, 27 insertions(+), 41 deletions(-)

Comments

Chris Wilson April 8, 2016, 3:01 p.m. UTC | #1 
On Fri, Apr 08, 2016 at 02:54:55PM +0100, Tvrtko Ursulin wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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.
> 
> v3: Extracted, rebased and fixed for GuC. (Tvrtko Ursulin)

After you mentioned the unbalanced, the patches I reordered to fix that
are:

https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=83dcde26caa26f4113c3e441c3c96c504fd88e13
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9f386a21d3f28db763102b5c4f97a90bd0dcfd08
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9afd878e2c9f7825b99dc839c7b5deb553b62e32
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=a842a2b0b7e90148966f35488209c969a9a9da54
-Chris
Tvrtko Ursulin April 11, 2016, 9:19 a.m. UTC | #2 
On 08/04/16 16:01, Chris Wilson wrote:
> On Fri, Apr 08, 2016 at 02:54:55PM +0100, Tvrtko Ursulin wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> 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.
>>
>> v3: Extracted, rebased and fixed for GuC. (Tvrtko Ursulin)
>
> After you mentioned the unbalanced, the patches I reordered to fix that
> are:
>
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=83dcde26caa26f4113c3e441c3c96c504fd88e13
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9f386a21d3f28db763102b5c4f97a90bd0dcfd08
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9afd878e2c9f7825b99dc839c7b5deb553b62e32
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=a842a2b0b7e90148966f35488209c969a9a9da54

Want to send these four as standalone straight away for review then?

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index da403f4fc679..fa199b7e51b5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2352,23 +2352,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->engine->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->engine->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/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 239452933a89..c1df696f9002 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1410,10 +1410,31 @@  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)
+{
+	if (i915.enable_execlists) {
+		if (request->ctx != request->i915->kernel_context)
+			intel_lr_context_unpin(request->ctx, request->engine);
+	}
+
+	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);
+	__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
 	 * of tail of the request to update the last known position
@@ -1424,10 +1445,7 @@  static void i915_gem_request_retire(struct drm_i915_gem_request *request)
 	 */
 	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);
 }
 
 static void
@@ -2723,18 +2741,7 @@  static void i915_set_reset_status(struct drm_i915_private *dev_priv,
 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 && ctx != req->i915->kernel_context)
-			intel_lr_context_unpin(ctx, req->engine);
-
-		i915_gem_context_unreference(ctx);
-	}
+							typeof(*req), ref);
 
 	kmem_cache_free(req->i915->requests, req);
 }
@@ -2830,13 +2837,6 @@  i915_gem_request_alloc(struct intel_engine_cs *engine,
 	return err ? ERR_PTR(err) : req;
 }
 
-void i915_gem_request_cancel(struct drm_i915_gem_request *req)
-{
-	intel_ring_reserved_space_cancel(req->ringbuf);
-
-	i915_gem_request_unreference(req);
-}
-
 struct drm_i915_gem_request *
 i915_gem_find_active_request(struct intel_engine_cs *engine)
 {
@@ -3194,7 +3194,7 @@  i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 			ret = __i915_wait_request(req[i], reset_counter, 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;
 
@@ -4216,7 +4216,7 @@  i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
-	i915_gem_request_unreference__unlocked(target);
+	i915_gem_request_unreference(target);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index feb7028341b8..f2ce7310e2b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11362,7 +11362,7 @@  static void intel_mmio_flip_work_func(struct work_struct *work)
 					    mmio_flip->crtc->reset_counter,
 					    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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 43b24a1f5ee6..346e353b4fe8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7378,7 +7378,7 @@  static void __intel_rps_boost_work(struct work_struct *work)
 		gen6_rps_boost(to_i915(req->engine->dev), NULL,
 			       req->emitted_jiffies);
 
-	i915_gem_request_unreference__unlocked(req);
+	i915_gem_request_unreference(req);
 	kfree(boost);
 }