diff mbox

[2/6] drm/i915: simplify testing for the global default context

Message ID 1450713885-5828-3-git-send-email-david.s.gordon@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Gordon Dec. 21, 2015, 4:04 p.m. UTC 
There are quite a number of places where the driver tests whether a
given context is or is not the global default context, usually by
checking whether an engine's default_pointer points to the context. Now
that we have a 'is_global_default' flag in the context itself, these can
be rewritten to use it. This makes the logic more obvious, and usually
saves at least one memory reference.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 +++-----
 drivers/gpu/drm/i915/i915_gem.c     |  8 ++------
 drivers/gpu/drm/i915/intel_lrc.c    | 26 ++++++++++++--------------
 3 files changed, 17 insertions(+), 25 deletions(-)

Comments

Chris Wilson Dec. 22, 2015, 9:05 a.m. UTC | #1 
On Mon, Dec 21, 2015 at 04:04:41PM +0000, Dave Gordon wrote:
> There are quite a number of places where the driver tests whether a
> given context is or is not the global default context, usually by
> checking whether an engine's default_pointer points to the context. Now
> that we have a 'is_global_default' flag in the context itself, these can
> be rewritten to use it. This makes the logic more obvious, and usually
> saves at least one memory reference.

All these places do not need to exist. Please just fix execlists.
-Chris
Dave Gordon Dec. 22, 2015, 11:35 a.m. UTC | #2 
On 22/12/15 09:05, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:41PM +0000, Dave Gordon wrote:
>> There are quite a number of places where the driver tests whether a
>> given context is or is not the global default context, usually by
>> checking whether an engine's default_pointer points to the context. Now
>> that we have a 'is_global_default' flag in the context itself, these can
>> be rewritten to use it. This makes the logic more obvious, and usually
>> saves at least one memory reference.
>
> All these places do not need to exist. Please just fix execlists.
> -Chris

The patchset "to fix execlists" in one go would be too large to be 
accepted here and would take too long to develop, given the nature of 
the moving target. Ergo, we can fix execlists only by taking every 
opportunity to move towards a clearer design, even though each step 
fails to "fix execlists" on its own.

We therefore have to judge each patch on the basis of "does it make 
things better or worse", not "does it fix all known problems". IMHO, 
this patch (and the rest of the set) are small steps towards a better 
design, and you should therefore support their adoption, unless of 
course you think it actually makes things worse - in which case, point 
out what's worse and I'll change it.

.Dave.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0fc38bb..13261fc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2037,13 +2037,11 @@  static int i915_dump_lrc(struct seq_file *m, void *unused)
 	if (ret)
 		return ret;
 
-	list_for_each_entry(ctx, &dev_priv->context_list, link) {
-		for_each_ring(ring, dev_priv, i) {
-			if (ring->default_context != ctx)
+	list_for_each_entry(ctx, &dev_priv->context_list, link)
+		if (!ctx->is_global_default)
+			for_each_ring(ring, dev_priv, i)
 				i915_dump_lrc_obj(m, ring,
 						  ctx->engine[i].state);
-		}
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6c60e04..be1f984 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2677,10 +2677,8 @@  void i915_gem_request_free(struct kref *req_ref)
 		i915_gem_request_remove_from_client(req);
 
 	if (ctx) {
-		if (i915.enable_execlists) {
-			if (ctx != req->ring->default_context)
-				intel_lr_context_unpin(req);
-		}
+		if (i915.enable_execlists && !ctx->is_global_default)
+			intel_lr_context_unpin(req);
 
 		i915_gem_context_unreference(ctx);
 	}
@@ -4869,8 +4867,6 @@  i915_gem_init_hw(struct drm_device *dev)
 	for_each_ring(ring, dev_priv, i) {
 		struct drm_i915_gem_request *req;
 
-		WARN_ON(!ring->default_context);
-
 		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
 		if (ret) {
 			i915_gem_cleanup_ringbuffer(dev);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 23f90b2..c44bd86 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -571,7 +571,7 @@  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)
+	if (!request->ctx->is_global_default)
 		intel_lr_context_pin(request);
 
 	i915_gem_request_reference(request);
@@ -660,17 +660,14 @@  static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 
 int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
 {
-	int ret;
+	int ret = 0;
 
 	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
 
-	if (request->ctx != request->ring->default_context) {
+	if (!request->ctx->is_global_default)
 		ret = intel_lr_context_pin(request);
-		if (ret)
-			return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
@@ -967,7 +964,7 @@  void intel_execlists_retire_requests(struct intel_engine_cs *ring)
 		struct drm_i915_gem_object *ctx_obj =
 				ctx->engine[ring->id].state;
 
-		if (ctx_obj && (ctx != ring->default_context))
+		if (ctx_obj && !ctx->is_global_default)
 			intel_lr_context_unpin(req);
 		list_del(&req->execlist_link);
 		i915_gem_request_unreference(req);
@@ -1916,6 +1913,8 @@  void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
 {
+	struct intel_context *dctx = ring->default_context;
+	int ring_id = ring->id;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -1936,15 +1935,14 @@  static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 	if (ret)
 		goto error;
 
-	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
+	ret = intel_lr_context_deferred_alloc(dctx, ring);
 	if (ret)
 		goto error;
 
 	/* As this is the default context, always pin it */
-	ret = intel_lr_context_do_pin(
-			ring,
-			ring->default_context->engine[ring->id].state,
-			ring->default_context->engine[ring->id].ringbuf);
+	ret = intel_lr_context_do_pin(ring,
+				      dctx->engine[ring_id].state,
+				      dctx->engine[ring_id].ringbuf);
 	if (ret) {
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
@@ -2479,7 +2477,7 @@  int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	ctx->engine[ring->id].ringbuf = ringbuf;
 	ctx->engine[ring->id].state = ctx_obj;
 
-	if (ctx != ring->default_context && ring->init_context) {
+	if (!ctx->is_global_default && ring->init_context) {
 		struct drm_i915_gem_request *req;
 
 		ret = i915_gem_request_alloc(ring,