diff mbox

[6/7] drm/i915: Store last context instead of the obj

Message ID 1365118914-15753-8-git-send-email-ben@bwidawsk.net (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky April 4, 2013, 11:41 p.m. UTC 
Storing the last context requires refcounting. Mika recently submitted
some refcounting patches which leverages our request mechanism. This is
insufficient for my needs because we want to know the last context even
if the request has ended, ie. doing the kref_put when a request is
finished isn't okay (unless we switch back to the default context, and
wait for the switch)

Context lifecycle works identically to the way the context object
lifecycle works.

As of now, I'm not making use of the context_status field. It seems like
it will be useful at least for debugging, but we can drop it if desired.

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++
 drivers/gpu/drm/i915/i915_gem.c         |  2 +
 drivers/gpu/drm/i915/i915_gem_context.c | 83 ++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
 4 files changed, 67 insertions(+), 24 deletions(-)

Comments

Chris Wilson April 5, 2013, 7:51 a.m. UTC | #1 
On Thu, Apr 04, 2013 at 04:41:53PM -0700, Ben Widawsky wrote:
> Storing the last context requires refcounting. Mika recently submitted
> some refcounting patches which leverages our request mechanism. This is
> insufficient for my needs because we want to know the last context even
> if the request has ended, ie. doing the kref_put when a request is
> finished isn't okay (unless we switch back to the default context, and
> wait for the switch)

This does not address Mika's requirements for tracking a ctx on each
request - but note that request->ctx can be used here instead.
-Chris
Ben Widawsky April 5, 2013, 5:28 p.m. UTC | #2 
On Fri, Apr 05, 2013 at 08:51:57AM +0100, Chris Wilson wrote:
> On Thu, Apr 04, 2013 at 04:41:53PM -0700, Ben Widawsky wrote:
> > Storing the last context requires refcounting. Mika recently submitted
> > some refcounting patches which leverages our request mechanism. This is
> > insufficient for my needs because we want to know the last context even
> > if the request has ended, ie. doing the kref_put when a request is
> > finished isn't okay (unless we switch back to the default context, and
> > wait for the switch)
> 
> This does not address Mika's requirements for tracking a ctx on each
> request - but note that request->ctx can be used here instead.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

First of all, I've realized my patch fails to accomplish what I set out
to accomplish.

My patch and Mika's patch are both subject to the same problem. Keep in
mind that I wanted access to the last context for the ring. If userspace
calls destroy, and the request completes, the refcount will drop to 0.
If you tried to access last context at that point, you're in trouble.

As I mentioned in response on patch 0/7 I now think I can get away
without needing the last context (at least for current GEN) because the
PDEs are global to all rings, so we can ignore this for now. I'll go
back and debug why Mika's patch was blowing up when I tried it before.

Thanks for taking the time to review. If you're bored, check if any of
the other patches are interesting ;-)
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 25cdade..3025b65 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -438,9 +438,12 @@  struct i915_hw_ppgtt {
 /* This must match up with the value previously used for execbuf2.rsvd1. */
 #define DEFAULT_CONTEXT_ID 0
 struct i915_hw_context {
+	struct kref ref;
 	int id;
 	enum {
 		I915_CTX_INITIALIZED=1,
+		I915_CTX_FILE_CLOSED,
+		I915_CTX_DESTROY_IOCTL,
 	} status;
 	struct drm_i915_file_private *file_priv;
 	struct intel_ring_buffer *ring;
@@ -1666,6 +1669,7 @@  struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 				struct drm_gem_object *gem_obj, int flags);
 
 /* i915_gem_context.c */
+void ctx_release(struct kref *kref);
 void i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8a2cbee..4097173 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1924,6 +1924,8 @@  i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	obj->fenced_gpu_access = false;
 
 	obj->active = 0;
+	if (obj->ctx)
+		kref_put(&obj->ctx->ref, ctx_release);
 	drm_gem_object_unreference(&obj->base);
 
 	WARN_ON(i915_verify_lists(dev));
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 41be2a5..f57c91a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -124,13 +124,25 @@  static int get_context_size(struct drm_device *dev)
 
 static void do_destroy(struct i915_hw_context *ctx)
 {
-	if (ctx->file_priv)
+	/* Need to remove the idr if close/destroy was called while the context
+	 * was active
+	 */
+	if (ctx->status == I915_CTX_DESTROY_IOCTL)
 		idr_remove(&ctx->file_priv->context_idr, ctx->id);
 
 	drm_gem_object_unreference(&ctx->obj->base);
+	if (WARN_ON(kref_get_unless_zero(&ctx->ref)))
+		kref_put(&ctx->ref, ctx_release);
 	kfree(ctx);
 }
 
+void ctx_release(struct kref *kref)
+{
+	struct i915_hw_context *ctx = container_of(kref,
+						   struct i915_hw_context, ref);
+	do_destroy(ctx);
+}
+
 static struct i915_hw_context *
 create_hw_context(struct drm_device *dev,
 		  struct drm_i915_file_private *file_priv)
@@ -159,8 +171,10 @@  create_hw_context(struct drm_device *dev,
 	ctx->ring = &dev_priv->ring[RCS];
 
 	/* Default context will never have a file_priv */
-	if (file_priv == NULL)
+	if (file_priv == NULL) {
+		kref_init(&ctx->ref);
 		return ctx;
+	}
 
 	ctx->file_priv = file_priv;
 
@@ -169,6 +183,7 @@  create_hw_context(struct drm_device *dev,
 	if (ret < 0)
 		goto err_out;
 	ctx->id = ret;
+	kref_init(&ctx->ref);
 
 	return ctx;
 
@@ -209,6 +224,8 @@  static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_destroy;
 
+	kref_get(&ctx->ref);
+
 	ret = do_switch(ctx);
 	if (ret)
 		goto err_unpin;
@@ -266,7 +283,8 @@  void i915_gem_context_fini(struct drm_device *dev)
 
 	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
 
-	do_destroy(dev_priv->ring[RCS].default_context);
+	while (!kref_put(&dev_priv->ring[RCS].default_context->ref,
+			 ctx_release));
 }
 
 static int context_idr_cleanup(int id, void *p, void *data)
@@ -275,8 +293,11 @@  static int context_idr_cleanup(int id, void *p, void *data)
 
 	BUG_ON(id == DEFAULT_CONTEXT_ID);
 
-	DRM_DEBUG_DRIVER("Context %d closed before destroy.\n", ctx->id);
-	do_destroy(ctx);
+	if (ctx->status != I915_CTX_DESTROY_IOCTL) {
+		DRM_DEBUG_DRIVER("Context %d closed before destroy.\n", ctx->id);
+		kref_put(&ctx->ref, ctx_release);
+	}
+	ctx->status = I915_CTX_FILE_CLOSED;
 
 	return 0;
 }
@@ -303,6 +324,9 @@  i915_gem_context_get(struct intel_ring_buffer *ring,
 		ctx = (struct i915_hw_context *)
 			idr_find(&file_priv->context_idr, id);
 
+	if (likely(ctx))
+		kref_get(&ctx->ref);
+
 	return ctx;
 }
 
@@ -356,18 +380,20 @@  mi_set_context(struct intel_ring_buffer *ring,
 static int do_switch(struct i915_hw_context *to)
 {
 	struct intel_ring_buffer *ring = to->ring;
-	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
+	struct i915_hw_context *from_ctx = ring->last_context;
 	u32 hw_flags = 0;
 	int ret;
 
-	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
+	BUG_ON(from_ctx != NULL && from_ctx->obj->pin_count == 0);
 
-	if (from_obj == to->obj)
-		return 0;
+	if (from_ctx == to) {
+		ret = 0;
+		goto err_out;
+	}
 
 	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false, false);
 	if (ret)
-		return ret;
+		goto err_out;
 
 	/* Clear this page out of any CPU caches for coherent swap-in/out. Note
 	 * that thanks to write = false in this call and us not setting any gpu
@@ -377,7 +403,7 @@  static int do_switch(struct i915_hw_context *to)
 	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
 	if (ret) {
 		i915_gem_object_unpin(to->obj);
-		return ret;
+		goto err_out;
 	}
 
 	if (!to->obj->has_global_gtt_mapping)
@@ -385,13 +411,13 @@  static int do_switch(struct i915_hw_context *to)
 
 	if (!to->status || is_default_context(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
-	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
+	else if (WARN_ON_ONCE(from_ctx == to)) /* not yet expected */
 		hw_flags |= MI_FORCE_RESTORE;
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret) {
 		i915_gem_object_unpin(to->obj);
-		return ret;
+		goto err_out;
 	}
 
 	/* The backing object for the context is done after switching to the
@@ -400,9 +426,9 @@  static int do_switch(struct i915_hw_context *to)
 	 * is a bit suboptimal because the retiring can occur simply after the
 	 * MI_SET_CONTEXT instead of when the next seqno has completed.
 	 */
-	if (likely(from_obj)) {
-		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		i915_gem_object_move_to_active(from_obj, ring);
+	if (likely(from_ctx)) {
+		from_ctx->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		i915_gem_object_move_to_active(from_ctx->obj, ring);
 		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
 		 * whole damn pipeline, we don't need to explicitly mark the
 		 * object dirty. The only exception is that the context must be
@@ -410,18 +436,22 @@  static int do_switch(struct i915_hw_context *to)
 		 * able to defer doing this until we know the object would be
 		 * swapped, but there is no way to do that yet.
 		 */
-		from_obj->dirty = 1;
-		BUG_ON(from_obj->ring != ring);
-		i915_gem_object_unpin(from_obj);
+		from_ctx->obj->dirty = 1;
+		BUG_ON(from_ctx->obj->ring != ring);
+		i915_gem_object_unpin(from_ctx->obj);
 
-		drm_gem_object_unreference(&from_obj->base);
+		drm_gem_object_unreference(&from_ctx->obj->base);
 	}
 
 	drm_gem_object_reference(&to->obj->base);
-	ring->last_context_obj = to->obj;
+	ring->last_context = to;
 	to->status = I915_CTX_INITIALIZED;
 
 	return 0;
+
+err_out:
+	kref_put(&to->ref, ctx_release);
+	return ret;
 }
 
 /**
@@ -458,6 +488,9 @@  int i915_switch_context(struct intel_ring_buffer *ring,
 		BUG_ON(to_id == DEFAULT_CONTEXT_ID);
 		DRM_DEBUG_DRIVER("Couldn't find context %d\n", to_id);
 		return -ENOENT;
+	} else if (to->status == I915_CTX_DESTROY_IOCTL) {
+		BUG_ON(kref_put(&to->ref, ctx_release));
+		return -ENOENT;
 	}
 
 	return do_switch(to);
@@ -488,7 +521,7 @@  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 		return PTR_ERR(ctx);
 
 	args->ctx_id = ctx->id;
-	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
+	DRM_DEBUG_DRIVER("HW context %d created (%p)\n", args->ctx_id, ctx);
 
 	return 0;
 }
@@ -517,7 +550,11 @@  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
-	do_destroy(ctx);
+	BUG_ON(kref_put(&ctx->ref, ctx_release));
+	ctx->status = I915_CTX_DESTROY_IOCTL;
+	/* NB: We can't remove from the idr yet because a user might create a
+	 * new context, and we need to reserve this id */
+	kref_put(&ctx->ref, ctx_release);
 
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index d66208c..dac1614 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -135,7 +135,7 @@  struct  intel_ring_buffer {
 	 */
 	bool itlb_before_ctx_switch;
 	struct i915_hw_context *default_context;
-	struct drm_i915_gem_object *last_context_obj;
+	struct i915_hw_context *last_context;
 
 	void *private;
 };