[PATCH 1/2] drm/i915: reference count for i915_hw_contexts

[PATCH 1/2] drm/i915: reference count for i915_hw_contexts

[Mika Kuoppala]

Enabling PPGTT and also the need to track which context was guilty of
gpu hang (arb robustness enabling) have put pressure for struct i915_hw_context
to be more than just a placeholder for hw context state.

In order to track object lifetime properly in a multi peer usage, add reference
counting for i915_hw_context.

v2: track i915_hw_context pointers instead of using ctx_ids
(from Chris Wilson)

v3 (Ben): Get rid of do_release() and handle refcounting more compactly.
(recommended by Chis)

v4: kref_* put inside static inlines (Daniel Vetter)
remove code duplication on freeing context (Chris Wilson)

v5: idr_remove and ctx->file_priv = NULL in destroy ioctl (Chris)
This actually will cause a problem if one destroys a context and later
refers to the idea of the context (multiple contexts may have the same
id, but only 1 will exist in the idr).

v6: Strip out the request related stuff. Reworded commit message.
Got rid of do_destroy and introduced i915_gem_context_release_handle,
suggested by Chris Wilson.

v7: idr_remove can't be called inside idr_for_each (Chris Wilson)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v2)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v3)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v4)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v5)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v7)
---
 drivers/gpu/drm/i915/i915_drv.h         |   12 ++++++++++++
 drivers/gpu/drm/i915/i915_gem_context.c |   28 ++++++++++++++--------------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14156f2..7f83be4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -452,6 +452,7 @@ 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;
 	bool is_initialized;
 	struct drm_i915_file_private *file_priv;
@@ -1701,6 +1702,17 @@ void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
 			struct drm_file *file, int to_id);
+void i915_gem_context_free(struct kref *ctx_ref);
+static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
+{
+	kref_get(&ctx->ref);
+}
+
+static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
+{
+	kref_put(&ctx->ref, i915_gem_context_free);
+}
+
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file);
 int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a1e8ecb..9e8c685 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -124,10 +124,10 @@ static int get_context_size(struct drm_device *dev)
 	return ret;
 }
 
-static void do_destroy(struct i915_hw_context *ctx)
+void i915_gem_context_free(struct kref *ctx_ref)
 {
-	if (ctx->file_priv)
-		idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	struct i915_hw_context *ctx = container_of(ctx_ref,
+						   typeof(*ctx), ref);
 
 	drm_gem_object_unreference(&ctx->obj->base);
 	kfree(ctx);
@@ -145,6 +145,7 @@ create_hw_context(struct drm_device *dev,
 	if (ctx == NULL)
 		return ERR_PTR(-ENOMEM);
 
+	kref_init(&ctx->ref);
 	ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
 	if (ctx->obj == NULL) {
 		kfree(ctx);
@@ -169,18 +170,18 @@ create_hw_context(struct drm_device *dev,
 	if (file_priv == NULL)
 		return ctx;
 
-	ctx->file_priv = file_priv;
-
 	ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID + 1, 0,
 			GFP_KERNEL);
 	if (ret < 0)
 		goto err_out;
+
+	ctx->file_priv = file_priv;
 	ctx->id = ret;
 
 	return ctx;
 
 err_out:
-	do_destroy(ctx);
+	i915_gem_context_unreference(ctx);
 	return ERR_PTR(ret);
 }
 
@@ -226,7 +227,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 err_unpin:
 	i915_gem_object_unpin(ctx->obj);
 err_destroy:
-	do_destroy(ctx);
+	i915_gem_context_unreference(ctx);
 	return ret;
 }
 
@@ -262,6 +263,7 @@ void 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 i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
@@ -271,9 +273,8 @@ void i915_gem_context_fini(struct drm_device *dev)
 	 * other code, leading to spurious errors. */
 	intel_gpu_reset(dev);
 
-	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
-
-	do_destroy(dev_priv->ring[RCS].default_context);
+	i915_gem_object_unpin(dctx->obj);
+	i915_gem_context_unreference(dctx);
 }
 
 static int context_idr_cleanup(int id, void *p, void *data)
@@ -282,8 +283,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
 
 	BUG_ON(id == DEFAULT_CONTEXT_ID);
 
-	do_destroy(ctx);
-
+	i915_gem_context_unreference(ctx);
 	return 0;
 }
 
@@ -512,8 +512,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
-	do_destroy(ctx);
-
+	idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	i915_gem_context_unreference(ctx);
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
-- 
1.7.9.5

[PATCH 2/2] drm/i915: unreference default context on module unload

[Mika Kuoppala]

Before module unload is called, gpu_idle() will switch
to default context. This will increment ref count of base
object as the default context is 'running' on module unload
time. Unreference the drm object so that when context
is freed, base object is freed as well.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9e8c685..30f7f4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -274,6 +274,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 	intel_gpu_reset(dev);
 
 	i915_gem_object_unpin(dctx->obj);
+	drm_gem_object_unreference(&dctx->obj->base);
 	i915_gem_context_unreference(dctx);
 }
 
-- 
1.7.9.5

Re: [PATCH 2/2] drm/i915: unreference default context on module unload

[Ben Widawsky]

On Tue, Apr 30, 2013 at 01:30:34PM +0300, Mika Kuoppala wrote:
> Before module unload is called, gpu_idle() will switch
> to default context. This will increment ref count of base
> object as the default context is 'running' on module unload
> time. Unreference the drm object so that when context
> is freed, base object is freed as well.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>


I would have liked the patch to be distinct from the context refcounting
since it really is a fix unrelated to the context refcounting. I don't
think you need to resend though.

I also think the commit message is false. The unload code will call
gpu_idle, yes, but it also will also idle the ring, wait for it to
complete and then immediately call i915_gem_retire_requests. Unless I'm
missing something that will handle the missing unref, and the only ref
possibly at that point is the one we have from when we created the
object.

The reset case can leave a ref over - but that's okay AFAICT.

I know we discussed this on IRC a few days ago, and then you convinced
me that we need this, but my brain reset. Can you explain where I've
gotten lost?

I still want to play around with patch 1 a bit more.

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9e8c685..30f7f4c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -274,6 +274,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	intel_gpu_reset(dev);
>  
>  	i915_gem_object_unpin(dctx->obj);
> +	drm_gem_object_unreference(&dctx->obj->base);
>  	i915_gem_context_unreference(dctx);
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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/2] drm/i915: unreference default context on module unload

[Mika Kuoppala]

Ben Widawsky <ben@bwidawsk.net> writes:

> On Tue, Apr 30, 2013 at 01:30:34PM +0300, Mika Kuoppala wrote:
>> Before module unload is called, gpu_idle() will switch
>> to default context. This will increment ref count of base
>> object as the default context is 'running' on module unload
>> time. Unreference the drm object so that when context
>> is freed, base object is freed as well.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
>
> I would have liked the patch to be distinct from the context refcounting
> since it really is a fix unrelated to the context refcounting. I don't
> think you need to resend though.
>
> I also think the commit message is false. The unload code will call
> gpu_idle, yes, but it also will also idle the ring, wait for it to
> complete and then immediately call i915_gem_retire_requests. Unless I'm
> missing something that will handle the missing unref, and the only ref
> possibly at that point is the one we have from when we created the
> object.
>
> The reset case can leave a ref over - but that's okay AFAICT.
>
> I know we discussed this on IRC a few days ago, and then you convinced
> me that we need this, but my brain reset. Can you explain where I've
> gotten lost?

This is how I see it:

When default context is created and switched to, base object refcount is
2. +1 from object creation, +1 from do_switch.

It is 2 also when _fini is called, as we are running in default context.

So if the i915_gem_context_unreference at the end of _fini 
will decrement the object creation part, we still need to offset the
do_switch part, to free the base object.

-Mika

> I still want to play around with patch 1 a bit more.
>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_context.c |    1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 9e8c685..30f7f4c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -274,6 +274,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>>  	intel_gpu_reset(dev);
>>  
>>  	i915_gem_object_unpin(dctx->obj);
>> +	drm_gem_object_unreference(&dctx->obj->base);
>>  	i915_gem_context_unreference(dctx);
>>  }
>>  
>> -- 
>> 1.7.9.5
>> 
>> _______________________________________________
>> 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/2] drm/i915: unreference default context on module unload

[Ben Widawsky]

On Thu, May 02, 2013 at 11:37:02AM +0300, Mika Kuoppala wrote:
> Ben Widawsky <ben@bwidawsk.net> writes:
> 
> > On Tue, Apr 30, 2013 at 01:30:34PM +0300, Mika Kuoppala wrote:
> >> Before module unload is called, gpu_idle() will switch
> >> to default context. This will increment ref count of base
> >> object as the default context is 'running' on module unload
> >> time. Unreference the drm object so that when context
> >> is freed, base object is freed as well.
> >> 
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> >
> > I would have liked the patch to be distinct from the context refcounting
> > since it really is a fix unrelated to the context refcounting. I don't
> > think you need to resend though.
> >
> > I also think the commit message is false. The unload code will call
> > gpu_idle, yes, but it also will also idle the ring, wait for it to
> > complete and then immediately call i915_gem_retire_requests. Unless I'm
> > missing something that will handle the missing unref, and the only ref
> > possibly at that point is the one we have from when we created the
> > object.
> >
> > The reset case can leave a ref over - but that's okay AFAICT.
> >
> > I know we discussed this on IRC a few days ago, and then you convinced
> > me that we need this, but my brain reset. Can you explain where I've
> > gotten lost?
> 
> This is how I see it:
> 
> When default context is created and switched to, base object refcount is
> 2. +1 from object creation, +1 from do_switch.
> 
> It is 2 also when _fini is called, as we are running in default context.
> 
> So if the i915_gem_context_unreference at the end of _fini 
> will decrement the object creation part, we still need to offset the
> do_switch part, to free the base object.
> 
> -Mika

Oh yeah, do_switch() that was the piece that I had forgotten/missed.
Thanks. Add what you just said  as a comment to the patch preferably in
fini() and it's:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> > I still want to play around with patch 1 a bit more.
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_context.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >> index 9e8c685..30f7f4c 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >> @@ -274,6 +274,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> >>  	intel_gpu_reset(dev);
> >>  
> >>  	i915_gem_object_unpin(dctx->obj);
> >> +	drm_gem_object_unreference(&dctx->obj->base);
> >>  	i915_gem_context_unreference(dctx);
> >>  }
> >>  
> >> -- 
> >> 1.7.9.5
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > -- 
> > Ben Widawsky, Intel Open Source Technology Center

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH v2] drm/i915: unreference default context on module unload

[Mika Kuoppala]

Before module unload is called, gpu_idle() will switch
to default context. This will increment ref count of base
object as the default context is 'running' on module unload
time. Unreference the drm object so that when context
is freed, base object is freed as well.

v2: added comment to explain the refcounts (Ben Widawsky)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9e8c685..280617e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -274,6 +274,14 @@ void i915_gem_context_fini(struct drm_device *dev)
 	intel_gpu_reset(dev);
 
 	i915_gem_object_unpin(dctx->obj);
+
+	/* When default context is created and switched to, base object refcount
+	 * will be 2 (+1 from object creation and +1 from do_switch()).
+	 * i915_gem_context_fini() will be called after gpu_idle() has switched
+	 * to default context. So we need to unreference the base object once
+	 * to offset the do_switch part, so that i915_gem_context_unreference()
+	 * can then free the base object correctly. */
+	drm_gem_object_unreference(&dctx->obj->base);
 	i915_gem_context_unreference(dctx);
 }
 
-- 
1.7.9.5

Re: [PATCH v2] drm/i915: unreference default context on module unload

[Ben Widawsky]

On Fri, May 03, 2013 at 04:29:08PM +0300, Mika Kuoppala wrote:
> Before module unload is called, gpu_idle() will switch
> to default context. This will increment ref count of base
> object as the default context is 'running' on module unload
> time. Unreference the drm object so that when context
> is freed, base object is freed as well.
> 
> v2: added comment to explain the refcounts (Ben Widawsky)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

I left an r-b on the other patch. You should feel free to add it if you
actually do what I say.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
[snip]
-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: unreference default context on module unload

[Daniel Vetter]

On Fri, May 03, 2013 at 08:53:20AM -0700, Ben Widawsky wrote:
> On Fri, May 03, 2013 at 04:29:08PM +0300, Mika Kuoppala wrote:
> > Before module unload is called, gpu_idle() will switch
> > to default context. This will increment ref count of base
> > object as the default context is 'running' on module unload
> > time. Unreference the drm object so that when context
> > is freed, base object is freed as well.
> > 
> > v2: added comment to explain the refcounts (Ben Widawsky)
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> I left an r-b on the other patch. You should feel free to add it if you
> actually do what I say.
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

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

Re: [PATCH 1/2] drm/i915: reference count for i915_hw_contexts

[Ben Widawsky]

On Tue, Apr 30, 2013 at 01:30:33PM +0300, Mika Kuoppala wrote:
> Enabling PPGTT and also the need to track which context was guilty of
> gpu hang (arb robustness enabling) have put pressure for struct i915_hw_context
> to be more than just a placeholder for hw context state.
> 
> In order to track object lifetime properly in a multi peer usage, add reference
> counting for i915_hw_context.
> 
> v2: track i915_hw_context pointers instead of using ctx_ids
> (from Chris Wilson)
> 
> v3 (Ben): Get rid of do_release() and handle refcounting more compactly.
> (recommended by Chis)
> 
> v4: kref_* put inside static inlines (Daniel Vetter)
> remove code duplication on freeing context (Chris Wilson)
> 
> v5: idr_remove and ctx->file_priv = NULL in destroy ioctl (Chris)
> This actually will cause a problem if one destroys a context and later
> refers to the idea of the context (multiple contexts may have the same
> id, but only 1 will exist in the idr).
> 
> v6: Strip out the request related stuff. Reworded commit message.
> Got rid of do_destroy and introduced i915_gem_context_release_handle,
> suggested by Chris Wilson.
> 
> v7: idr_remove can't be called inside idr_for_each (Chris Wilson)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v2)
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v3)
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v4)
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v5)
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v7)

I can't spot anything wrong, thought I'd argue this commit message
belongs with the actual reference counting to come later since a lot of
the history is with bugs found there.

This patch is simply introducing context refcounting interfaces and
replacing explicit destruction with unreferencing (and in all cases
there should be no functional impact).

So bring on the real request reference counting now!
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>


> ---
>  drivers/gpu/drm/i915/i915_drv.h         |   12 ++++++++++++
>  drivers/gpu/drm/i915/i915_gem_context.c |   28 ++++++++++++++--------------
>  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 14156f2..7f83be4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -452,6 +452,7 @@ 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;
>  	bool is_initialized;
>  	struct drm_i915_file_private *file_priv;
> @@ -1701,6 +1702,17 @@ void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
>  			struct drm_file *file, int to_id);
> +void i915_gem_context_free(struct kref *ctx_ref);
> +static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
> +{
> +	kref_get(&ctx->ref);
> +}
> +
> +static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
> +{
> +	kref_put(&ctx->ref, i915_gem_context_free);
> +}
> +
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file);
>  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a1e8ecb..9e8c685 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -124,10 +124,10 @@ static int get_context_size(struct drm_device *dev)
>  	return ret;
>  }
>  
> -static void do_destroy(struct i915_hw_context *ctx)
> +void i915_gem_context_free(struct kref *ctx_ref)
>  {
> -	if (ctx->file_priv)
> -		idr_remove(&ctx->file_priv->context_idr, ctx->id);
> +	struct i915_hw_context *ctx = container_of(ctx_ref,
> +						   typeof(*ctx), ref);
>  
>  	drm_gem_object_unreference(&ctx->obj->base);
>  	kfree(ctx);
> @@ -145,6 +145,7 @@ create_hw_context(struct drm_device *dev,
>  	if (ctx == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> +	kref_init(&ctx->ref);
>  	ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
>  	if (ctx->obj == NULL) {
>  		kfree(ctx);
> @@ -169,18 +170,18 @@ create_hw_context(struct drm_device *dev,
>  	if (file_priv == NULL)
>  		return ctx;
>  
> -	ctx->file_priv = file_priv;
> -
>  	ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID + 1, 0,
>  			GFP_KERNEL);
>  	if (ret < 0)
>  		goto err_out;
> +
> +	ctx->file_priv = file_priv;
>  	ctx->id = ret;
>  
>  	return ctx;
>  
>  err_out:
> -	do_destroy(ctx);
> +	i915_gem_context_unreference(ctx);
>  	return ERR_PTR(ret);
>  }
>  
> @@ -226,7 +227,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  err_unpin:
>  	i915_gem_object_unpin(ctx->obj);
>  err_destroy:
> -	do_destroy(ctx);
> +	i915_gem_context_unreference(ctx);
>  	return ret;
>  }
>  
> @@ -262,6 +263,7 @@ void 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 i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
>  
>  	if (dev_priv->hw_contexts_disabled)
>  		return;
> @@ -271,9 +273,8 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	 * other code, leading to spurious errors. */
>  	intel_gpu_reset(dev);
>  
> -	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
> -
> -	do_destroy(dev_priv->ring[RCS].default_context);
> +	i915_gem_object_unpin(dctx->obj);
> +	i915_gem_context_unreference(dctx);
>  }
>  
>  static int context_idr_cleanup(int id, void *p, void *data)
> @@ -282,8 +283,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
>  
>  	BUG_ON(id == DEFAULT_CONTEXT_ID);
>  
> -	do_destroy(ctx);
> -
> +	i915_gem_context_unreference(ctx);
>  	return 0;
>  }
>  
> @@ -512,8 +512,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> -	do_destroy(ctx);
> -
> +	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> +	i915_gem_context_unreference(ctx);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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/2] drm/i915: reference count for i915_hw_contexts

[Daniel Vetter]

On Tue, Apr 30, 2013 at 11:40:16AM -0700, Ben Widawsky wrote:
> On Tue, Apr 30, 2013 at 01:30:33PM +0300, Mika Kuoppala wrote:
> > Enabling PPGTT and also the need to track which context was guilty of
> > gpu hang (arb robustness enabling) have put pressure for struct i915_hw_context
> > to be more than just a placeholder for hw context state.
> > 
> > In order to track object lifetime properly in a multi peer usage, add reference
> > counting for i915_hw_context.
> > 
> > v2: track i915_hw_context pointers instead of using ctx_ids
> > (from Chris Wilson)
> > 
> > v3 (Ben): Get rid of do_release() and handle refcounting more compactly.
> > (recommended by Chis)
> > 
> > v4: kref_* put inside static inlines (Daniel Vetter)
> > remove code duplication on freeing context (Chris Wilson)
> > 
> > v5: idr_remove and ctx->file_priv = NULL in destroy ioctl (Chris)
> > This actually will cause a problem if one destroys a context and later
> > refers to the idea of the context (multiple contexts may have the same
> > id, but only 1 will exist in the idr).
> > 
> > v6: Strip out the request related stuff. Reworded commit message.
> > Got rid of do_destroy and introduced i915_gem_context_release_handle,
> > suggested by Chris Wilson.
> > 
> > v7: idr_remove can't be called inside idr_for_each (Chris Wilson)
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v2)
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v3)
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v4)
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v5)
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v7)
> 
> I can't spot anything wrong, thought I'd argue this commit message
> belongs with the actual reference counting to come later since a lot of
> the history is with bugs found there.
> 
> This patch is simply introducing context refcounting interfaces and
> replacing explicit destruction with unreferencing (and in all cases
> there should be no functional impact).
> 
> So bring on the real request reference counting now!
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch