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

2013-04-05 Thread 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013-04-05 Thread 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013-04-04 Thread 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.kuopp...@linux.intel.com
Signed-off-by: Ben Widawsky b...@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,