[PATCH v2, 0/4] improve handling of the driver's default context

[PATCH v2, 0/4] improve handling of the driver's default context

[Dave Gordon]

A reworking of the previous patchset, incorporating Daniel Vetter's
point that we don't need intel_context::file_priv and Chris Wilson's
wish to abolish engine::default_context.

Patch 1/4 starts the process by eliminating file_priv, which was only used
in one place. Patch 2/4 removes lots of references to default_context,
wherever it was used in a comparison. Patch 3/4 continues by removing
more references to default_context, wherever it was used for allocation.
And at the last, patch 4/4 replaces the remaining per-engine uses with a
single per-device pointer, thus finally making the refcounting sane.

Enjoy :)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2, 1/4] drm/i915: remove intel_context::file_priv, add flag for default context

[Dave Gordon]

The file_priv member was used in only one place, where we already had
the file_priv and therefore didn't need to derive it from the context
(in fact, we just found the context by reference to the file_priv).
So by fixing up that one use, we can drop file_priv entirely.

OTOH sometimes we need to identify the global (default) context, which
belongs to the driver as whole and is not associated with any user file
descriptor, so here we add a flag for that specific purpose. This will
then be used instead of the current unsafe practice of check each engine
to see whether its default_context pointer refers back to the context
being operated on. In a subsequent patch, these backpointers will go
away too :)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  3 ++-
 drivers/gpu/drm/i915/i915_gem_context.c | 22 ++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b82c45..4fb15f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -854,6 +854,7 @@ struct i915_ctx_hang_stats {
  * @ref: reference count.
  * @user_handle: userspace tracking identity for this context.
  * @remap_slice: l3 row remapping information.
+ * @is_default: true iff this is the global default context
  * @flags: context specific flags:
  *         CONTEXT_NO_ZEROMAP: do not allow mapping things to page 0.
  * @file_priv: filp associated with this context (NULL for global default
@@ -872,9 +873,9 @@ struct intel_context {
 	struct kref ref;
 	int user_handle;
 	uint8_t remap_slice;
+	uint8_t is_global_default;
 	struct drm_i915_private *i915;
 	int flags;
-	struct drm_i915_file_private *file_priv;
 	struct i915_ctx_hang_stats hang_stats;
 	struct i915_hw_ppgtt *ppgtt;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..43437af 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -236,16 +236,17 @@ __create_hw_context(struct drm_device *dev,
 	}
 
 	/* Default context will never have a file_priv */
-	if (file_priv != NULL) {
+	if (file_priv == NULL) {
+		ctx->is_global_default = true;
+		ret = DEFAULT_CONTEXT_HANDLE;
+	} else {
 		ret = idr_alloc(&file_priv->context_idr, ctx,
 				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
 		if (ret < 0)
 			goto err_out;
-	} else
-		ret = DEFAULT_CONTEXT_HANDLE;
-
-	ctx->file_priv = file_priv;
+	}
 	ctx->user_handle = ret;
+
 	/* NB: Mark all slices as needing a remap so that when the context first
 	 * loads it will restore whatever remap state already exists. If there
 	 * is no remap info, it will be a NOP. */
@@ -269,7 +270,6 @@ static struct intel_context *
 i915_gem_create_context(struct drm_device *dev,
 			struct drm_i915_file_private *file_priv)
 {
-	const bool is_global_default_ctx = file_priv == NULL;
 	struct intel_context *ctx;
 	int ret = 0;
 
@@ -279,8 +279,9 @@ i915_gem_create_context(struct drm_device *dev,
 	if (IS_ERR(ctx))
 		return ctx;
 
-	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) {
-		/* We may need to do things with the shrinker which
+	if (ctx->is_global_default && ctx->legacy_hw_ctx.rcs_state) {
+		/*
+		 * We may need to do things with the shrinker which
 		 * require us to immediately switch back to the default
 		 * context. This can cause a problem as pinning the
 		 * default context also requires GTT space which may not
@@ -313,7 +314,7 @@ i915_gem_create_context(struct drm_device *dev,
 	return ctx;
 
 err_unpin:
-	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state)
+	if (ctx->is_global_default && ctx->legacy_hw_ctx.rcs_state)
 		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
 err_destroy:
 	idr_remove(&file_priv->context_idr, ctx->user_handle);
@@ -381,6 +382,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		}
 	}
 
+	/* NULL second parameter indicates this is the global default context */
 	ctx = i915_gem_create_context(dev, NULL);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
@@ -896,7 +898,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return PTR_ERR(ctx);
 	}
 
-	idr_remove(&ctx->file_priv->context_idr, ctx->user_handle);
+	idr_remove(&file_priv->context_idr, ctx->user_handle);
 	i915_gem_context_unreference(ctx);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Dave Gordon]

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,
all these tests these can be rewritten to use it. This makes the
logic more obvious, and usually saves at least one memory reference.
In addition, with these uses eliminated, a future patch will be able
to get rid of engine::default_context entirely.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++---------
 drivers/gpu/drm/i915/i915_gem.c     |  6 ++----
 drivers/gpu/drm/i915/intel_lrc.c    | 40 +++++++++++++++++--------------------
 3 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0fc38bb..5e6e02c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
 
 		seq_puts(m, "HW context ");
 		describe_ctx(m, ctx);
-		for_each_ring(ring, dev_priv, i) {
-			if (ring->default_context == ctx)
-				seq_printf(m, "(default context %s) ",
-					   ring->name);
-		}
+		if (ctx->is_global_default)
+			seq_printf(m, "(default context) ");
 
 		if (i915.enable_execlists) {
 			seq_putc(m, '\n');
@@ -2037,13 +2034,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..d9a3304 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);
 	}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3aa6147..cf76d28 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);
@@ -2367,22 +2364,21 @@ void intel_lr_context_free(struct intel_context *ctx)
 {
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for (i = I915_NUM_RINGS; --i >= 0; ) {
+		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
 		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
 
-		if (ctx_obj) {
-			struct intel_ringbuffer *ringbuf =
-					ctx->engine[i].ringbuf;
-			struct intel_engine_cs *ring = ringbuf->ring;
+		if (!ctx_obj)
+			continue;
 
-			if (ctx == ring->default_context) {
-				intel_unpin_ringbuffer_obj(ringbuf);
-				i915_gem_object_ggtt_unpin(ctx_obj);
-			}
-			WARN_ON(ctx->engine[ring->id].pin_count);
-			intel_ringbuffer_free(ringbuf);
-			drm_gem_object_unreference(&ctx_obj->base);
+		if (ctx->is_global_default) {
+			intel_unpin_ringbuffer_obj(ringbuf);
+			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
+
+		WARN_ON(ctx->engine[i].pin_count);
+		intel_ringbuffer_free(ringbuf);
+		drm_gem_object_unreference(&ctx_obj->base);
 	}
 }
 
@@ -2443,7 +2439,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
  */
 
 int intel_lr_context_deferred_alloc(struct intel_context *ctx,
-				     struct intel_engine_cs *ring)
+				    struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_gem_object *ctx_obj;
@@ -2480,7 +2476,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,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2, 3/4] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

There are a number of places where the driver needs a request, but isn't
working on behalf of any specific user or in a specific context. At
present, we associate them with the per-engine default context. A future
patch will abolish those per-engine context pointers; but we can already
eliminate a lot of the references to them just by funneling all such
allocations through a single function, which will mean that the callers
don't need to know anything about how an appropriate context is found.

So this patch provides a shorthand for doing such request allocations
and changes all such calls to use the new function.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_gem.c      | 39 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_display.c |  6 ++++--
 drivers/gpu/drm/i915/intel_overlay.c | 24 +++++++++++-----------
 4 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4fb15f5..56d149e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2269,6 +2269,8 @@ struct drm_i915_gem_request {
 int i915_gem_request_alloc(struct intel_engine_cs *ring,
 			   struct intel_context *ctx,
 			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request * __must_check
+	i915_gem_request_alloc_anon(struct intel_engine_cs *ring);
 void i915_gem_request_cancel(struct drm_i915_gem_request *req);
 void i915_gem_request_free(struct kref *req_ref);
 int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d9a3304..927ebc8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2751,6 +2751,22 @@ err:
 	return ret;
 }
 
+/*
+ * Allocate a request associated with the default context for the given
+ * ring. This can be used where the driver needs a request for internal
+ * purposes not directly related to a user batch submission.
+ */
+struct drm_i915_gem_request *
+i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
+{
+	struct drm_i915_private *dev_priv = to_i915(ring->dev);
+	struct drm_i915_gem_request *req;
+	int err;
+
+	err = i915_gem_request_alloc(ring, ring->default_context, &req);
+	return err ? ERR_PTR(err) : req;
+}
+
 void i915_gem_request_cancel(struct drm_i915_gem_request *req)
 {
 	intel_ring_reserved_space_cancel(req->ringbuf);
@@ -3168,9 +3184,13 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			return 0;
 
 		if (*to_req == NULL) {
-			ret = i915_gem_request_alloc(to, to->default_context, to_req);
-			if (ret)
-				return ret;
+			struct drm_i915_gem_request *req;
+
+			req = i915_gem_request_alloc_anon(to);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
+
+			*to_req = req;
 		}
 
 		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
@@ -3370,9 +3390,9 @@ int i915_gpu_idle(struct drm_device *dev)
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
-			ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-			if (ret)
-				return ret;
+			req = i915_gem_request_alloc_anon(ring);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
 
 			ret = i915_switch_context(req);
 			if (ret) {
@@ -4867,10 +4887,9 @@ 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) {
+		req = i915_gem_request_alloc_anon(ring);
+		if (IS_ERR(req)) {
+			ret = PTR_ERR(req);
 			i915_gem_cleanup_ringbuffer(dev);
 			goto out;
 		}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index abd2d29..5716f4a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11662,9 +11662,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 					obj->last_write_req);
 	} else {
 		if (!request) {
-			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
-			if (ret)
+			request = i915_gem_request_alloc_anon(ring);
+			if (IS_ERR(request)) {
+				ret = PTR_ERR(request);
 				goto cleanup_unpin;
+			}
 		}
 
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 76f1980..57cd503 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -240,9 +240,9 @@ static int intel_overlay_on(struct intel_overlay *overlay)
 	WARN_ON(overlay->active);
 	WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc_anon(ring);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 4);
 	if (ret) {
@@ -283,9 +283,9 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
 	if (tmp & (1 << 17))
 		DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc_anon(ring);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 2);
 	if (ret) {
@@ -349,9 +349,9 @@ static int intel_overlay_off(struct intel_overlay *overlay)
 	 * of the hw. Do it in both cases */
 	flip_addr |= OFC_UPDATE;
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc_anon(ring);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 6);
 	if (ret) {
@@ -423,9 +423,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 		/* synchronous slowpath */
 		struct drm_i915_gem_request *req;
 
-		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-		if (ret)
-			return ret;
+		req = i915_gem_request_alloc_anon(ring);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
 
 		ret = intel_ring_begin(req, 2);
 		if (ret) {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2, 4/4] drm/i915: abolish separate per-engine default_context pointers

[Dave Gordon]

Now that we've eliminated a lot of uses of engine::default_context,
we can eliminate the pointer itself.

All the engines share the same default intel_context, so we can just
keep a single reference to it in the dev_priv structure rather than one
in each of the engine[] elements. This make refcounting more sensible
too, as we now have a refcount of one for the one pointer, rather than
a refcount of one but multiple pointers.

From an idea by Chris Wilson.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 ++
 drivers/gpu/drm/i915/i915_gem.c            |  4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c    | 22 ++++++++--------------
 drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  6 +++---
 drivers/gpu/drm/i915/intel_lrc.c           | 14 ++++++++------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
 7 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56d149e..8ed89a1f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1946,6 +1946,8 @@ struct drm_i915_private {
 		void (*stop_ring)(struct intel_engine_cs *ring);
 	} gt;
 
+	struct intel_context *kernel_context;
+
 	bool edp_low_vswing;
 
 	/* perform PHY state sanity checks? */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 927ebc8..01b1eea 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2763,7 +2763,7 @@ i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
 	struct drm_i915_gem_request *req;
 	int err;
 
-	err = i915_gem_request_alloc(ring, ring->default_context, &req);
+	err = i915_gem_request_alloc(ring, dev_priv->kernel_context, &req);
 	return err ? ERR_PTR(err) : req;
 }
 
@@ -4850,7 +4850,7 @@ i915_gem_init_hw(struct drm_device *dev)
 	 */
 	init_unused_rings(dev);
 
-	BUG_ON(!dev_priv->ring[RCS].default_context);
+	BUG_ON(!dev_priv->kernel_context);
 
 	ret = i915_ppgtt_init_hw(dev);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 43437af..c0efec3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -355,11 +355,10 @@ int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
-	int i;
 
 	/* Init should only be called once per module load. Eventually the
 	 * restriction on the context_disabled check can be loosened. */
-	if (WARN_ON(dev_priv->ring[RCS].default_context))
+	if (WARN_ON(dev_priv->kernel_context))
 		return 0;
 
 	if (intel_vgpu_active(dev) && HAS_LOGICAL_RING_CONTEXTS(dev)) {
@@ -390,12 +389,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_engine_cs *ring = &dev_priv->ring[i];
-
-		/* NB: RCS will hold a ref for all rings */
-		ring->default_context = ctx;
-	}
+	dev_priv->kernel_context = ctx;
 
 	DRM_DEBUG_DRIVER("%s context support initialized\n",
 			i915.enable_execlists ? "LR" :
@@ -406,7 +400,7 @@ int i915_gem_context_init(struct drm_device *dev)
 void i915_gem_context_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
+	struct intel_context *dctx = dev_priv->kernel_context;
 	int i;
 
 	if (dctx->legacy_hw_ctx.rcs_state) {
@@ -433,17 +427,17 @@ void i915_gem_context_fini(struct drm_device *dev)
 		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for (i = I915_NUM_RINGS; --i >= 0;) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
-		if (ring->last_context)
+		if (ring->last_context) {
 			i915_gem_context_unreference(ring->last_context);
-
-		ring->default_context = NULL;
-		ring->last_context = NULL;
+			ring->last_context = NULL;
+		}
 	}
 
 	i915_gem_context_unreference(dctx);
+	dev_priv->kernel_context = NULL;
 }
 
 int i915_gem_context_enable(struct drm_i915_gem_request *req)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 06ca408..7eeb244 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1050,7 +1050,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 			if (request)
 				rbuf = request->ctx->engine[ring->id].ringbuf;
 			else
-				rbuf = ring->default_context->engine[ring->id].ringbuf;
+				rbuf = dev_priv->kernel_context->engine[ring->id].ringbuf;
 		} else
 			rbuf = ring->buffer;
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 9cc3b84..70d0da6 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -863,7 +863,7 @@ int i915_guc_submission_enable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_context *ctx = dev_priv->ring[RCS].default_context;
+	struct intel_context *ctx = dev_priv->kernel_context;
 	struct i915_guc_client *client;
 
 	/* client for execbuf submission */
@@ -917,7 +917,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->ring[RCS].default_context;
+	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -943,7 +943,7 @@ int intel_guc_resume(struct drm_device *dev)
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->ring[RCS].default_context;
+	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index cf76d28..743ac81 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1471,7 +1471,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
-				ring->default_context->engine[ring->id].state);
+				dev_priv->kernel_context->engine[ring->id].state);
 
 	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
 	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
@@ -1913,6 +1913,9 @@ 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 drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
+	int ring_id = ring->id;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -1933,15 +1936,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",
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 49574ff..e950f6c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -305,7 +305,6 @@ struct  intel_engine_cs {
 
 	wait_queue_head_t irq_queue;
 
-	struct intel_context *default_context;
 	struct intel_context *last_context;
 
 	struct intel_ring_hangcheck hangcheck;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Chris Wilson]

On Wed, Dec 23, 2015 at 07:33:53PM +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,
> all these tests these can be rewritten to use it. This makes the
> logic more obvious, and usually saves at least one memory reference.
> In addition, with these uses eliminated, a future patch will be able
> to get rid of engine::default_context entirely.

All the execlists use of ctx != ring->default_context stems from a
misstep in execlists - if you stop treating that default_context as
special during request processing and just take the pin/unpin at
init/fini of the ring, they all disappear.

And please stop conflating is_global_context when we have already a very
good expression for when the context is owned by no file.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ec0382c73cb1adc972bebdd94afad3f0ea117114 drm-intel-nightly: 2015y-12m-23d-22h-28m-25s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (bdw-nuci7)
                pass       -> DMESG-WARN (skl-i7k-2)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (byt-nuc)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (bdw-ultra)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (byt-nuc)

bdw-nuci7        total:132  pass:121  dwarn:2   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:119  dwarn:3   dfail:0   fail:0   skip:13 
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_818/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Dave Gordon]

On 23/12/15 21:02, Chris Wilson wrote:
> On Wed, Dec 23, 2015 at 07:33:53PM +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,
>> all these tests these can be rewritten to use it. This makes the
>> logic more obvious, and usually saves at least one memory reference.
>> In addition, with these uses eliminated, a future patch will be able
>> to get rid of engine::default_context entirely.
>
> All the execlists use of ctx != ring->default_context stems from a
> misstep in execlists - if you stop treating that default_context as
> special during request processing and just take the pin/unpin at
> init/fini of the ring, they all disappear.

We do already pin/unpin the default context at creation/deletion; AFAICS 
the extra tests are probably an attempt not to do an extra pin/unpin on 
an object which is by definition already pinned. And I'd be quite happy 
to get rid of those tests, and just issue a pin for *every* request 
issues on a context -- indeed, I think Nick may have just such a patch. 
But his changes are blocked on getting the elimination of 
ring->default_context (patch 4 of THIS set) merged first, since having 
those backpointers dictates the order of creation and destruction.

> And please stop conflating is_global_context when we have already a very
> good expression for when the context is owned by no file.
> -Chris

No existing code uses that as a test. And patch 1 of this set dropped 
file_priv anyway, since there was no actual use for it anywhere at all!
This way we don't risk having dangling pointers to deallocated file objects.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Chris Wilson]

On Mon, Jan 04, 2016 at 05:43:10PM +0000, Dave Gordon wrote:
> On 23/12/15 21:02, Chris Wilson wrote:
> >On Wed, Dec 23, 2015 at 07:33:53PM +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,
> >>all these tests these can be rewritten to use it. This makes the
> >>logic more obvious, and usually saves at least one memory reference.
> >>In addition, with these uses eliminated, a future patch will be able
> >>to get rid of engine::default_context entirely.
> >
> >All the execlists use of ctx != ring->default_context stems from a
> >misstep in execlists - if you stop treating that default_context as
> >special during request processing and just take the pin/unpin at
> >init/fini of the ring, they all disappear.
> 
> We do already pin/unpin the default context at creation/deletion;
> AFAICS the extra tests are probably an attempt not to do an extra
> pin/unpin on an object which is by definition already pinned. And
> I'd be quite happy to get rid of those tests, and just issue a pin
> for *every* request issues on a context -- indeed, I think Nick may
> have just such a patch. But his changes are blocked on getting the
> elimination of ring->default_context (patch 4 of THIS set) merged
> first, since having those backpointers dictates the order of
> creation and destruction.

This series is NAKed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Jesse Barnes]

On 01/04/2016 11:39 AM, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 05:43:10PM +0000, Dave Gordon wrote:
>> On 23/12/15 21:02, Chris Wilson wrote:
>>> On Wed, Dec 23, 2015 at 07:33:53PM +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,
>>>> all these tests these can be rewritten to use it. This makes the
>>>> logic more obvious, and usually saves at least one memory reference.
>>>> In addition, with these uses eliminated, a future patch will be able
>>>> to get rid of engine::default_context entirely.
>>>
>>> All the execlists use of ctx != ring->default_context stems from a
>>> misstep in execlists - if you stop treating that default_context as
>>> special during request processing and just take the pin/unpin at
>>> init/fini of the ring, they all disappear.
>>
>> We do already pin/unpin the default context at creation/deletion;
>> AFAICS the extra tests are probably an attempt not to do an extra
>> pin/unpin on an object which is by definition already pinned. And
>> I'd be quite happy to get rid of those tests, and just issue a pin
>> for *every* request issues on a context -- indeed, I think Nick may
>> have just such a patch. But his changes are blocked on getting the
>> elimination of ring->default_context (patch 4 of THIS set) merged
>> first, since having those backpointers dictates the order of
>> creation and destruction.
> 
> This series is NAKed.

Why?  Because you want things in a different order?  Or do you object to something in Dave's reply?

Jesse

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Chris Wilson]

On Mon, Jan 04, 2016 at 01:38:26PM -0800, Jesse Barnes wrote:
> On 01/04/2016 11:39 AM, Chris Wilson wrote:
> > This series is NAKed.
> 
> Why?  Because you want things in a different order?  Or do you object to something in Dave's reply?

The series was intended as a code cleanup and in the process tried to
introduce a false concept that I objected to. Since the cleanup was not
predicated upon that idea, the patches would have been much tidier
without it.

The fundamental issue at stake here is execlists behaves badly and we
have to futz around in higher level code to undo that mistake.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2, 2/4] drm/i915: simplify testing for the global default context

[Dave Gordon]

On 05/01/16 10:06, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 01:38:26PM -0800, Jesse Barnes wrote:
>> On 01/04/2016 11:39 AM, Chris Wilson wrote:
>>> This series is NAKed.
>>
>> Why?  Because you want things in a different order?  Or do you object to something in Dave's reply?
>
> The series was intended as a code cleanup and in the process tried to
> introduce a false concept that I objected to. Since the cleanup was not
> predicated upon that idea, the patches would have been much tidier
> without it.

I don't think it's a false concept; there very evidently *IS* a global 
default context, so why not flag it as such by name, rather than by 
implication. and the subsequent cleanup *does* require it.

> The fundamental issue at stake here is execlists behaves badly and we
> have to futz around in higher level code to undo that mistake.
> -Chris

Now I agree that the execlist->default_context is a bad idea, and that's 
exactly what we will get rid of in patch 4, but we can't do it until 
these precursor patches have separated the various purposes for which it 
is used into distinct categories, each of which is then replaced by a 
cleaner alternative.

So it *is* a step towards rewriting execlists, even if it doesn't (at 
this stage) fix everything you might eventually want.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx