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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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