| Message ID | 1367317834-11294-2-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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
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
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); }
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(+)