[PATCH 0/7] [RFC] Context reference counting

[PATCH 0/7] [RFC] Context reference counting

[Ben Widawsky]

These patches implement full context reference counting. In the patch that
actually adds the reference counting, I explain why I think Mika's reference
counting isn't sufficient for me. Please see/respond to that patch if you
disagree.

Almost all of the actual work occurs in:
"drm/i915: Store last context instead of the obj"

This work is preliminary work for what I'm actually doing at the moment which
is proper PPGTT support. The patches split off quite nicely here, so I'm
submitting it for review separately. I believe these patches provide a superset
of the functionality needed by Mika for ARB_Robustness.

The primary reason I want to track context destruction is for PPGTT support
we'd like to teardown ppgtt state when a context goes away, but can only do so
when we're absolutely certain those PTEs are no longer needed. In my design,
I've made a 1:1 relationship with context->ppgtt, and so refcounting the
context makes sense there. The crux of the solution here is to store the
context pointer in the object that backs it. I could probably use that alone to
solve my problem, but I've gone a bit further and also stored the last context
in the ring, instead of the last context object. I can't show code yet, but
I believe there are a couple of other niceties to having the last context, and
not an object.

There is at least one sticking point in this patch series, which is the
aforementioned storing of the context in the object backs the context. That
patch has been rejected before. I'm open to other ways to handle this.
At the very least, I require being able to teardown PPGTT when a context
dies.

For reference, here is the last time the patch was shot down:
http://lists.freedesktop.org/archives/intel-gfx/2012-June/thread.html#18000

Ben Widawsky (7):
  drm/i915: Mark context switch likely
  drm/i915: Move context special case to get()
  drm/i915: Make object aware that it backs a context
  drm/i915: Better context messages
  drm/i915: Track context status
  drm/i915: Store last context instead of the obj
  drm/i915: Print all contexts in debugfs

 drivers/gpu/drm/i915/i915_debugfs.c     |  30 +++++++-
 drivers/gpu/drm/i915/i915_drv.h         |  10 ++-
 drivers/gpu/drm/i915/i915_gem.c         |   2 +
 drivers/gpu/drm/i915/i915_gem_context.c | 127 ++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +-
 5 files changed, 129 insertions(+), 42 deletions(-)

-- 
1.8.2

[PATCH 1/7] drm/i915: Mark context switch likely

[Ben Widawsky]

Only the very first switch doesn't take the path.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 94d873a..aa080ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -390,7 +390,7 @@ 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 (from_obj != NULL) {
+	if (likely(from_obj)) {
 		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
 		i915_gem_object_move_to_active(from_obj, ring);
 		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
-- 
1.8.2

[PATCH 2/7] drm/i915: Move context special case to get()

[Ben Widawsky]

This allows us to make upcoming refcounting code a bit simpler, and
cleaner. In addition, I think it makes the interface a bit nicer if the
caller doesn't need to figure out default contexts and such.

The interface works very similarly to the gem object ref counting, and
I believe it makes sense to do so as we'll use it in a very similar way
to objects (we currently use objects as a somewhat hackish way to manage
context lifecycles).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index aa080ea..6211637 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -95,8 +95,6 @@
  */
 #define CONTEXT_ALIGN (64<<10)
 
-static struct i915_hw_context *
-i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
 static int do_switch(struct i915_hw_context *to);
 
 static int get_context_size(struct drm_device *dev)
@@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 }
 
 static struct i915_hw_context *
-i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
+i915_gem_context_get(struct intel_ring_buffer *ring,
+		     struct drm_i915_file_private *file_priv, u32 id)
 {
-	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	struct i915_hw_context *ctx;
+
+	if (id == DEFAULT_CONTEXT_ID)
+		ctx = ring->default_context;
+	else
+		ctx = (struct i915_hw_context *)
+			idr_find(&file_priv->context_idr, id);
+
+	return ctx;
 }
 
 static inline int
@@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	if (ring != &dev_priv->ring[RCS])
 		return 0;
 
-	if (to_id == DEFAULT_CONTEXT_ID) {
-		to = ring->default_context;
-	} else {
-		if (file == NULL)
-			return -EINVAL;
+	if (file == NULL)
+		return -EINVAL;
 
-		to = i915_gem_context_get(file->driver_priv, to_id);
-		if (to == NULL)
-			return -ENOENT;
-	}
+	to = i915_gem_context_get(ring, file->driver_priv, to_id);
+	if (to == NULL)
+		return -ENOENT;
 
 	return do_switch(to);
 }
@@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 	if (!(dev->driver->driver_features & DRIVER_GEM))
 		return -ENODEV;
 
+	if (args->ctx_id == DEFAULT_CONTEXT_ID)
+		return -ENOENT;
+
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
 
-	ctx = i915_gem_context_get(file_priv, args->ctx_id);
+	ctx = i915_gem_context_get(NULL, file_priv, args->ctx_id);
 	if (!ctx) {
 		mutex_unlock(&dev->struct_mutex);
 		return -ENOENT;
-- 
1.8.2

[PATCH 3/7] drm/i915: Make object aware that it backs a context

[Ben Widawsky]

It's really simple to keep track of objects which back the context.
Doing so allows will be really helpful in properly refcounting contexts

Daniel: please see the last patch in the series before commenting on
this patch. I'm open to other ideas, but this seems like the simplest
way to do it, and storing the context info in the object has never been
offensive to me.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 11 ++++++++++-
 drivers/gpu/drm/i915/i915_drv.h         |  2 ++
 drivers/gpu/drm/i915/i915_gem_context.c |  2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7df8351..375c36a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -90,6 +90,14 @@ static const char *get_tiling_flag(struct drm_i915_gem_object *obj)
 	}
 }
 
+static const char *get_context_flag(struct drm_i915_gem_object *obj)
+{
+	if (obj->ctx)
+		return "c";
+	else
+		return " ";
+}
+
 static const char *cache_level_str(int type)
 {
 	switch (type) {
@@ -103,10 +111,11 @@ static const char *cache_level_str(int type)
 static void
 describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 {
-	seq_printf(m, "%p: %s%s %8zdKiB %02x %02x %d %d %d%s%s%s",
+	seq_printf(m, "%p: %s%s%s %8zdKiB %02x %02x %d %d %d%s%s%s",
 		   &obj->base,
 		   get_pin_flag(obj),
 		   get_tiling_flag(obj),
+		   get_context_flag(obj),
 		   obj->base.size / 1024,
 		   obj->base.read_domains,
 		   obj->base.write_domain,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1657d873..da071a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1214,6 +1214,8 @@ struct drm_i915_gem_object {
 
 	/** for phy allocated objects */
 	struct drm_i915_gem_phys_object *phys_obj;
+
+	struct i915_hw_context *ctx;
 };
 #define to_gem_object(obj) (&((struct drm_i915_gem_object *)(obj))->base)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6211637..8e218ad 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -150,6 +150,8 @@ create_hw_context(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	ctx->obj->ctx = ctx;
+
 	/* The ring associated with the context object is handled by the normal
 	 * object tracking code. We give an initial ring value simple to pass an
 	 * assertion in the context switch code.
-- 
1.8.2

[PATCH 4/7] drm/i915: A bit better messaging for contexts

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8e218ad..5ac93f4 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,6 +97,11 @@
 
 static int do_switch(struct i915_hw_context *to);
 
+static inline bool is_default_context(struct i915_hw_context *ctx)
+{
+	return (ctx == ctx->ring->default_context);
+}
+
 static int get_context_size(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -124,8 +129,10 @@ static int get_context_size(struct drm_device *dev)
 
 static void do_destroy(struct i915_hw_context *ctx)
 {
-	if (ctx->file_priv)
+	if (ctx->file_priv) {
+		WARN_ON(!is_default_context(ctx));
 		idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	}
 
 	drm_gem_object_unreference(&ctx->obj->base);
 	kfree(ctx);
@@ -177,11 +184,6 @@ err_out:
 	return ERR_PTR(ret);
 }
 
-static inline bool is_default_context(struct i915_hw_context *ctx)
-{
-	return (ctx == ctx->ring->default_context);
-}
-
 /**
  * The default context needs to exist per ring that uses contexts. It stores the
  * context state of the GPU for applications that don't utilize HW contexts, as
@@ -213,7 +215,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_unpin;
 
-	DRM_DEBUG_DRIVER("Default HW context loaded\n");
+	DRM_DEBUG_DRIVER("Default HW context loaded (%p)\n", ctx);
 	return 0;
 
 err_unpin:
@@ -275,6 +277,7 @@ 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);
 
 	return 0;
@@ -453,8 +456,11 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 		return -EINVAL;
 
 	to = i915_gem_context_get(ring, file->driver_priv, to_id);
-	if (to == NULL)
+	if (unlikely(!to)) {
+		BUG_ON(to_id == DEFAULT_CONTEXT_ID);
+		DRM_DEBUG_DRIVER("Couldn't find context %d\n", to_id);
 		return -ENOENT;
+	}
 
 	return do_switch(to);
 }
@@ -517,6 +523,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 
 	mutex_unlock(&dev->struct_mutex);
 
-	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
+	DRM_DEBUG_DRIVER("User destroyed context %d\n", args->ctx_id);
 	return 0;
 }
-- 
1.8.2

[PATCH 4/7] drm/i915: Better context messages

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8e218ad..8b2e73a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -213,7 +213,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_unpin;
 
-	DRM_DEBUG_DRIVER("Default HW context loaded\n");
+	DRM_DEBUG_DRIVER("Default HW context loaded (%p)\n", ctx);
 	return 0;
 
 err_unpin:
@@ -275,6 +275,7 @@ 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);
 
 	return 0;
@@ -453,8 +454,11 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 		return -EINVAL;
 
 	to = i915_gem_context_get(ring, file->driver_priv, to_id);
-	if (to == NULL)
+	if (unlikely(!to)) {
+		BUG_ON(to_id == DEFAULT_CONTEXT_ID);
+		DRM_DEBUG_DRIVER("Couldn't find context %d\n", to_id);
 		return -ENOENT;
+	}
 
 	return do_switch(to);
 }
@@ -517,6 +521,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 
 	mutex_unlock(&dev->struct_mutex);
 
-	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
+	DRM_DEBUG_DRIVER("User destroyed context %d\n", args->ctx_id);
 	return 0;
 }
-- 
1.8.2

[PATCH 5/7] drm/i915: Track context status

[Ben Widawsky]

Instead of using just an is_initialized flag, it will be helpful to have
a bit more information about the context's actual status primarily for
the case when the file is closed before the context has actually be
destroyed.

Could also be useful for debugging.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         | 4 +++-
 drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index da071a0..25cdade 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -439,7 +439,9 @@ struct i915_hw_ppgtt {
 #define DEFAULT_CONTEXT_ID 0
 struct i915_hw_context {
 	int id;
-	bool is_initialized;
+	enum {
+		I915_CTX_INITIALIZED=1,
+	} status;
 	struct drm_i915_file_private *file_priv;
 	struct intel_ring_buffer *ring;
 	struct drm_i915_gem_object *obj;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8b2e73a..41be2a5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -383,7 +383,7 @@ static int do_switch(struct i915_hw_context *to)
 	if (!to->obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(to->obj, to->obj->cache_level);
 
-	if (!to->is_initialized || is_default_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 */
 		hw_flags |= MI_FORCE_RESTORE;
@@ -419,7 +419,7 @@ static int do_switch(struct i915_hw_context *to)
 
 	drm_gem_object_reference(&to->obj->base);
 	ring->last_context_obj = to->obj;
-	to->is_initialized = true;
+	to->status = I915_CTX_INITIALIZED;
 
 	return 0;
 }
-- 
1.8.2

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

[Ben Widawsky]

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(-)

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;
 };
-- 
1.8.2

[PATCH 7/7] drm/i915: Print all contexts in debugfs

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 375c36a..eef2575 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1471,11 +1471,23 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int context_show(int id, void *p, void *data)
+{
+	struct i915_hw_context *ctx = p;
+	struct seq_file *m = data;
+
+	seq_printf(m, "context = %d\n", id);
+	describe_obj(m, ctx->obj);
+	seq_printf(m, "\n");
+	return 0;
+}
+
 static int i915_context_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_file *file;
 	struct intel_ring_buffer *ring;
 	int ret, i;
 
@@ -1503,6 +1515,13 @@ static int i915_context_status(struct seq_file *m, void *unused)
 		}
 	}
 
+	list_for_each_entry(file, &dev->filelist, lhead) {
+		struct drm_i915_file_private *file_priv = file->driver_priv;
+		seq_printf(m, "File = %p ", file);
+		idr_for_each(&file_priv->context_idr, context_show, m);
+		seq_printf(m, "\n");
+	}
+
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return 0;
-- 
1.8.2

Re: [PATCH 0/7] [RFC] Context reference counting

[Ben Widawsky]

On Thu, Apr 04, 2013 at 04:41:46PM -0700, Ben Widawsky wrote:
> These patches implement full context reference counting. In the patch that
> actually adds the reference counting, I explain why I think Mika's reference
> counting isn't sufficient for me. Please see/respond to that patch if you
> disagree.
> 
> Almost all of the actual work occurs in:
> "drm/i915: Store last context instead of the obj"
> 
> This work is preliminary work for what I'm actually doing at the moment which
> is proper PPGTT support. The patches split off quite nicely here, so I'm
> submitting it for review separately. I believe these patches provide a superset
> of the functionality needed by Mika for ARB_Robustness.
> 
> The primary reason I want to track context destruction is for PPGTT support
> we'd like to teardown ppgtt state when a context goes away, but can only do so
> when we're absolutely certain those PTEs are no longer needed. In my design,
> I've made a 1:1 relationship with context->ppgtt, and so refcounting the
> context makes sense there. The crux of the solution here is to store the
> context pointer in the object that backs it. I could probably use that alone to
> solve my problem, but I've gone a bit further and also stored the last context
> in the ring, instead of the last context object. I can't show code yet, but
> I believe there are a couple of other niceties to having the last context, and
> not an object.

I sent the wrong version of this text. What I meant is, the crux of the
solution is reference counting + storing the context pointer in the
object. I believe I am not /required/ to store the last_context as I've
done.

> 
> There is at least one sticking point in this patch series, which is the
> aforementioned storing of the context in the object backs the context. That
> patch has been rejected before. I'm open to other ways to handle this.
> At the very least, I require being able to teardown PPGTT when a context
> dies.
> 
> For reference, here is the last time the patch was shot down:
> http://lists.freedesktop.org/archives/intel-gfx/2012-June/thread.html#18000
> 
> Ben Widawsky (7):
>   drm/i915: Mark context switch likely
>   drm/i915: Move context special case to get()
>   drm/i915: Make object aware that it backs a context
>   drm/i915: Better context messages
>   drm/i915: Track context status
>   drm/i915: Store last context instead of the obj
>   drm/i915: Print all contexts in debugfs
> 
>  drivers/gpu/drm/i915/i915_debugfs.c     |  30 +++++++-
>  drivers/gpu/drm/i915/i915_drv.h         |  10 ++-
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +
>  drivers/gpu/drm/i915/i915_gem_context.c | 127 ++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +-
>  5 files changed, 129 insertions(+), 42 deletions(-)
> 
> -- 
> 1.8.2
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/7] [RFC] Context reference counting

[Ben Widawsky]

On Thu, Apr 04, 2013 at 04:41:58PM -0700, Ben Widawsky wrote:
> On Thu, Apr 04, 2013 at 04:41:46PM -0700, Ben Widawsky wrote:
> > These patches implement full context reference counting. In the patch that
> > actually adds the reference counting, I explain why I think Mika's reference
> > counting isn't sufficient for me. Please see/respond to that patch if you
> > disagree.
> > 
> > Almost all of the actual work occurs in:
> > "drm/i915: Store last context instead of the obj"
> > 
> > This work is preliminary work for what I'm actually doing at the moment which
> > is proper PPGTT support. The patches split off quite nicely here, so I'm
> > submitting it for review separately. I believe these patches provide a superset
> > of the functionality needed by Mika for ARB_Robustness.
> > 
> > The primary reason I want to track context destruction is for PPGTT support
> > we'd like to teardown ppgtt state when a context goes away, but can only do so
> > when we're absolutely certain those PTEs are no longer needed. In my design,
> > I've made a 1:1 relationship with context->ppgtt, and so refcounting the
> > context makes sense there. The crux of the solution here is to store the
> > context pointer in the object that backs it. I could probably use that alone to
> > solve my problem, but I've gone a bit further and also stored the last context
> > in the ring, instead of the last context object. I can't show code yet, but
> > I believe there are a couple of other niceties to having the last context, and
> > not an object.
> 
> I sent the wrong version of this text. What I meant is, the crux of the
> solution is reference counting + storing the context pointer in the
> object. I believe I am not /required/ to store the last_context as I've
> done.

I've managed to convince myself storing the last context vs. the last
context object really isn't important. Just the refcounting matters.
Since we have refcounting though, I feel storing last_context makes a
bit more sense, but I don't really care about that aspect.

For PPGTT (at least for current gens) we have to store the current
address space globally as opposed to per ring.

> 
> > 
> > There is at least one sticking point in this patch series, which is the
> > aforementioned storing of the context in the object backs the context. That
> > patch has been rejected before. I'm open to other ways to handle this.
> > At the very least, I require being able to teardown PPGTT when a context
> > dies.
> > 
> > For reference, here is the last time the patch was shot down:
> > http://lists.freedesktop.org/archives/intel-gfx/2012-June/thread.html#18000
> > 
> > Ben Widawsky (7):
> >   drm/i915: Mark context switch likely
> >   drm/i915: Move context special case to get()
> >   drm/i915: Make object aware that it backs a context
> >   drm/i915: Better context messages
> >   drm/i915: Track context status
> >   drm/i915: Store last context instead of the obj
> >   drm/i915: Print all contexts in debugfs
> > 
> >  drivers/gpu/drm/i915/i915_debugfs.c     |  30 +++++++-
> >  drivers/gpu/drm/i915/i915_drv.h         |  10 ++-
> >  drivers/gpu/drm/i915/i915_gem.c         |   2 +
> >  drivers/gpu/drm/i915/i915_gem_context.c | 127 ++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +-
> >  5 files changed, 129 insertions(+), 42 deletions(-)
> > 
> > -- 
> > 1.8.2
> > 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: Mark context switch likely

[Jani Nikula]

On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> Only the very first switch doesn't take the path.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 94d873a..aa080ea 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -390,7 +390,7 @@ 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 (from_obj != NULL) {
> +	if (likely(from_obj)) {

Looking at the function, is this, uh, likely to make a difference?

Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's
*two* patches 4/7 in the series! :o)

I'm just generally wary of adding (un)likely annotations. I don't think
it matters that the annotation itself is obviously correct; IMHO the
performance impact should matter.

</bikeshed>

BR,
Jani.


>  		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
>  		i915_gem_object_move_to_active(from_obj, ring);
>  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> -- 
> 1.8.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

Re: [PATCH 2/7] drm/i915: Move context special case to get()

[Ville Syrjälä]

On Thu, Apr 04, 2013 at 04:41:48PM -0700, Ben Widawsky wrote:
> This allows us to make upcoming refcounting code a bit simpler, and
> cleaner. In addition, I think it makes the interface a bit nicer if the
> caller doesn't need to figure out default contexts and such.
> 
> The interface works very similarly to the gem object ref counting, and
> I believe it makes sense to do so as we'll use it in a very similar way
> to objects (we currently use objects as a somewhat hackish way to manage
> context lifecycles).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index aa080ea..6211637 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -95,8 +95,6 @@
>   */
>  #define CONTEXT_ALIGN (64<<10)
>  
> -static struct i915_hw_context *
> -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
>  static int do_switch(struct i915_hw_context *to);
>  
>  static int get_context_size(struct drm_device *dev)
> @@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  }
>  
>  static struct i915_hw_context *
> -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> +i915_gem_context_get(struct intel_ring_buffer *ring,
> +		     struct drm_i915_file_private *file_priv, u32 id)
>  {
> -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> +	struct i915_hw_context *ctx;
> +
> +	if (id == DEFAULT_CONTEXT_ID)
> +		ctx = ring->default_context;
> +	else
> +		ctx = (struct i915_hw_context *)
> +			idr_find(&file_priv->context_idr, id);
> +
> +	return ctx;
>  }
>  
>  static inline int
> @@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	if (ring != &dev_priv->ring[RCS])
>  		return 0;
>  
> -	if (to_id == DEFAULT_CONTEXT_ID) {
> -		to = ring->default_context;
> -	} else {
> -		if (file == NULL)
> -			return -EINVAL;
> +	if (file == NULL)
> +		return -EINVAL;

This looks wrong. Before the NULL check was only done when to_id !=
DEFAULT_CONTEXT_ID. Now it's done always, which means the call from
i915_gpu_idle() will always fail.

>  
> -		to = i915_gem_context_get(file->driver_priv, to_id);
> -		if (to == NULL)
> -			return -ENOENT;
> -	}
> +	to = i915_gem_context_get(ring, file->driver_priv, to_id);
> +	if (to == NULL)
> +		return -ENOENT;
>  
>  	return do_switch(to);
>  }
> @@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  	if (!(dev->driver->driver_features & DRIVER_GEM))
>  		return -ENODEV;
>  
> +	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> +		return -ENOENT;
> +
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
>  		return ret;
>  
> -	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> +	ctx = i915_gem_context_get(NULL, file_priv, args->ctx_id);
>  	if (!ctx) {
>  		mutex_unlock(&dev->struct_mutex);
>  		return -ENOENT;
> -- 
> 1.8.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/7] drm/i915: Mark context switch likely

[Ben Widawsky]

On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote:
> On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Only the very first switch doesn't take the path.
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 94d873a..aa080ea 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -390,7 +390,7 @@ 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 (from_obj != NULL) {
> > +	if (likely(from_obj)) {
> 
> Looking at the function, is this, uh, likely to make a difference?
> 

Probably not.

> Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's
> *two* patches 4/7 in the series! :o)

I don't see two, weird.

> 
> I'm just generally wary of adding (un)likely annotations. I don't think
> it matters that the annotation itself is obviously correct; IMHO the
> performance impact should matter.
> 
> </bikeshed>

Right. I actually stuck it in by accident, and decided to keep it only
because it serves as nice documentation. I suppose a comment would do
the same thing, but this also had a chance (albeit slight) to improve
things.

> 
> BR,
> Jani.
> 
> 
> >  		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> >  		i915_gem_object_move_to_active(from_obj, ring);
> >  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> > -- 
> > 1.8.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/7] drm/i915: Move context special case to get()

[Ben Widawsky]

On Fri, Apr 05, 2013 at 01:41:11PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 04, 2013 at 04:41:48PM -0700, Ben Widawsky wrote:
> > This allows us to make upcoming refcounting code a bit simpler, and
> > cleaner. In addition, I think it makes the interface a bit nicer if the
> > caller doesn't need to figure out default contexts and such.
> > 
> > The interface works very similarly to the gem object ref counting, and
> > I believe it makes sense to do so as we'll use it in a very similar way
> > to objects (we currently use objects as a somewhat hackish way to manage
> > context lifecycles).
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index aa080ea..6211637 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -95,8 +95,6 @@
> >   */
> >  #define CONTEXT_ALIGN (64<<10)
> >  
> > -static struct i915_hw_context *
> > -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> >  static int do_switch(struct i915_hw_context *to);
> >  
> >  static int get_context_size(struct drm_device *dev)
> > @@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  }
> >  
> >  static struct i915_hw_context *
> > -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> > +i915_gem_context_get(struct intel_ring_buffer *ring,
> > +		     struct drm_i915_file_private *file_priv, u32 id)
> >  {
> > -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> > +	struct i915_hw_context *ctx;
> > +
> > +	if (id == DEFAULT_CONTEXT_ID)
> > +		ctx = ring->default_context;
> > +	else
> > +		ctx = (struct i915_hw_context *)
> > +			idr_find(&file_priv->context_idr, id);
> > +
> > +	return ctx;
> >  }
> >  
> >  static inline int
> > @@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> >  	if (ring != &dev_priv->ring[RCS])
> >  		return 0;
> >  
> > -	if (to_id == DEFAULT_CONTEXT_ID) {
> > -		to = ring->default_context;
> > -	} else {
> > -		if (file == NULL)
> > -			return -EINVAL;
> > +	if (file == NULL)
> > +		return -EINVAL;
> 
> This looks wrong. Before the NULL check was only done when to_id !=
> DEFAULT_CONTEXT_ID. Now it's done always, which means the call from
> i915_gpu_idle() will always fail.
> 

Yeah, this definitely is incorrect. I wonder why I didn't hit any
problems on the i-g-t suite. Thanks for finding it. I'll come up with
some fix locally.

> >  
> > -		to = i915_gem_context_get(file->driver_priv, to_id);
> > -		if (to == NULL)
> > -			return -ENOENT;
> > -	}
> > +	to = i915_gem_context_get(ring, file->driver_priv, to_id);
> > +	if (to == NULL)
> > +		return -ENOENT;
> >  
> >  	return do_switch(to);
> >  }
> > @@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> >  	if (!(dev->driver->driver_features & DRIVER_GEM))
> >  		return -ENODEV;
> >  
> > +	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> > +		return -ENOENT;
> > +
> >  	ret = i915_mutex_lock_interruptible(dev);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > +	ctx = i915_gem_context_get(NULL, file_priv, args->ctx_id);
> >  	if (!ctx) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		return -ENOENT;
> > -- 
> > 1.8.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ben Widawsky]

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 ;-)

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: Mark context switch likely

[Daniel Vetter]

On Fri, Apr 05, 2013 at 09:44:54AM -0700, Ben Widawsky wrote:
> On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote:
> > On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > Only the very first switch doesn't take the path.
> > >
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 94d873a..aa080ea 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -390,7 +390,7 @@ 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 (from_obj != NULL) {
> > > +	if (likely(from_obj)) {
> > 
> > Looking at the function, is this, uh, likely to make a difference?
> > 
> 
> Probably not.
> 
> > Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's
> > *two* patches 4/7 in the series! :o)
> 
> I don't see two, weird.
> 
> > 
> > I'm just generally wary of adding (un)likely annotations. I don't think
> > it matters that the annotation itself is obviously correct; IMHO the
> > performance impact should matter.
> > 
> > </bikeshed>
> 
> Right. I actually stuck it in by accident, and decided to keep it only
> because it serves as nice documentation. I suppose a comment would do
> the same thing, but this also had a chance (albeit slight) to improve
> things.

I have to admit that I like the documentation a likely/unlikely can
provide for the extreme case (like here where it only happens on a fresh
gpu iirc). But ofthen the same can be achieved with a goto slowpath or an
appropriately called function for the slowpath (or naming the check in an
obvious way like not_initiailized or so). But often the slowpath is
already pretty clear, e.g. handling -EFAULT for copy_from|to_user_atomic.

Definite bikeshed territory ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/7] drm/i915: Mark context switch likely

[Jani Nikula]

On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote:
>> Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's
>> *two* patches 4/7 in the series! :o)
>
> I don't see two, weird.

Hmm, it's not only my mail setup:

http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/20106/focus=20108

>> I'm just generally wary of adding (un)likely annotations. I don't think
>> it matters that the annotation itself is obviously correct; IMHO the
>> performance impact should matter.
>> 
>> </bikeshed>
>
> Right. I actually stuck it in by accident, and decided to keep it only
> because it serves as nice documentation. I suppose a comment would do
> the same thing, but this also had a chance (albeit slight) to improve
> things.

That's true too, and I do like code that documents itself. No biggie.


BR,
Jani.