diff mbox

[1/6] drm/i915: mark the global default (intel_)context as such

Message ID 1450713885-5828-2-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
Some of the LRC-specific context-destruction code has to special-case
the global default context, bacause the HWSP is part of that context. At
present it deduces it indirectly by checking for the backpointer from
the engine to the context, but that's an unsafe assumption if the setup
and teardown code is reorganised. (It could also test ctx->file_priv ==
NULL, but again that's a detail that might be subject to change).

So here we explicitly flag the default context at the point of creation,
and then reorganise the code in intel_lr_context_free() not to rely on
the ring->default_pointer (still) being set up; to iterate over engines
in reverse, as this is teardown code; and to reduce the nesting
level so it's easier to read.

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

Comments

Chris Wilson Dec. 22, 2015, 9:08 a.m. UTC | #1
On Mon, Dec 21, 2015 at 04:04:40PM +0000, Dave Gordon wrote:
> Some of the LRC-specific context-destruction code has to special-case
> the global default context, bacause the HWSP is part of that context. At
> present it deduces it indirectly by checking for the backpointer from
> the engine to the context, but that's an unsafe assumption if the setup
> and teardown code is reorganised. (It could also test ctx->file_priv ==
> NULL, but again that's a detail that might be subject to change).
> 
> So here we explicitly flag the default context at the point of creation,
> and then reorganise the code in intel_lr_context_free() not to rely on
> the ring->default_pointer (still) being set up; to iterate over engines
> in reverse, as this is teardown code; and to reduce the nesting
> level so it's easier to read.

I still strongly disagree with the confusion inherent here.
-Chris
Dave Gordon Dec. 22, 2015, 11:26 a.m. UTC | #2
On 22/12/15 09:08, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:40PM +0000, Dave Gordon wrote:
>> Some of the LRC-specific context-destruction code has to special-case
>> the global default context, bacause the HWSP is part of that context. At
>> present it deduces it indirectly by checking for the backpointer from
>> the engine to the context, but that's an unsafe assumption if the setup
>> and teardown code is reorganised. (It could also test ctx->file_priv ==
>> NULL, but again that's a detail that might be subject to change).
>>
>> So here we explicitly flag the default context at the point of creation,
>> and then reorganise the code in intel_lr_context_free() not to rely on
>> the ring->default_pointer (still) being set up; to iterate over engines
>> in reverse, as this is teardown code; and to reduce the nesting
>> level so it's easier to read.
>
> I still strongly disagree with the confusion inherent here.
> -Chris

I'm not sure that anything could be *less* confusing than indicating the 
global default context with a flag called "is_global_default".

There may indeed be all sorts of confusion in the LRC code, but I'm not 
trying to fix all of that in one go. This is just an enabler so that one 
small piece of that confusion can be replaced by something clear and 
explicit. Once that's done, we'll move on and see what else can also be 
clarified.

.Dave.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b82c45..666d07c 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,6 +873,7 @@  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;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..5d3e287 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -241,8 +241,10 @@  __create_hw_context(struct drm_device *dev,
 				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
 		if (ret < 0)
 			goto err_out;
-	} else
+	} else {
+		ctx->is_global_default = true;
 		ret = DEFAULT_CONTEXT_HANDLE;
+	}
 
 	ctx->file_priv = file_priv;
 	ctx->user_handle = ret;
@@ -269,7 +271,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 +280,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 +315,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);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3aa6147..23f90b2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2367,22 +2367,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 +2442,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;