[Intel-gfx] [PATCH 2/2] drm/i915: unreference default context on module unload
Ben Widawsky
ben at bwidawsk.net
Thu May 2 18:10:16 CEST 2013
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
More information about the Intel-gfx
mailing list