On Thu, May 02, 2013 at 11:37:02AM +0300, Mika Kuoppala wrote: > Ben Widawsky <ben at 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 at 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 at 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 at 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
