| Message ID | 1370036780-13482-2-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 31, 2013 at 02:46:20PM -0700, Ben Widawsky wrote: > The way the stolen handling works is we take a pin on the backing pages, > but we never actually get a reference to the bo. On freeing objects > allocated with stolen memory, the final unref will end up freeing the > object with pinned pages count left. To enable an assertion to catch > bugs in this code path, this patch cleans up that remaining pin. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> That neatly explains the WARN. Not too happy about accumulating lots of backing storage specific processing into free_object, but that can be fixed up later (there is an obj->ops->release() pending). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, May 31, 2013 at 02:46:20PM -0700, Ben Widawsky wrote: >> The way the stolen handling works is we take a pin on the backing pages, >> but we never actually get a reference to the bo. On freeing objects >> allocated with stolen memory, the final unref will end up freeing the >> object with pinned pages count left. To enable an assertion to catch >> bugs in this code path, this patch cleans up that remaining pin. >> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > That neatly explains the WARN. Not too happy about accumulating lots of > backing storage specific processing into free_object, but that can be > fixed up later (there is an obj->ops->release() pending). I'm more irked with the semantic overloading of object pinning. Might be cleaner to otherwise mark stolen obejcts as not shrinkable instead of pinning them for their entire lifetime. But we can bikeshed that later on ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote: > On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > That neatly explains the WARN. Not too happy about accumulating lots of > > backing storage specific processing into free_object, but that can be > > fixed up later (there is an obj->ops->release() pending). > > I'm more irked with the semantic overloading of object pinning. Might > be cleaner to otherwise mark stolen obejcts as not shrinkable instead > of pinning them for their entire lifetime. But we can bikeshed that > later on ;-) Some merit to that argument, but it still feels correct to say that the stolen pages are pinned for their lifetime. Given obj->ops->release(), it does actually become simpler to not mess around with pin_count. So later it is. -Chris
On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote: >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > That neatly explains the WARN. Not too happy about accumulating lots of >> > backing storage specific processing into free_object, but that can be >> > fixed up later (there is an obj->ops->release() pending). >> >> I'm more irked with the semantic overloading of object pinning. Might >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead >> of pinning them for their entire lifetime. But we can bikeshed that >> later on ;-) > > Some merit to that argument, but it still feels correct to say that the > stolen pages are pinned for their lifetime. Given obj->ops->release(), > it does actually become simpler to not mess around with pin_count. So > later it is. I was more unhappy that pin_count has different meanings, until I've noticed that we've fixed that up already with the introduction of ->pages_pin_count. Shouldn't stolen mem just hold a reference on that one? After all unbinding from the gtt is ok with stolen memory, but dropping the backing storage in the shrinker won't work. Not that we currently use stolen for anything else than permanently pinned bos. Reading through the code we have a bit an unclear relationship between pages_pin_count and the (gtt) pin_count, so this approach would need some clarification (especially around the shrinker). And it would still leave us with a special case in free, so your ->release callback still has uses ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote: > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote: > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > That neatly explains the WARN. Not too happy about accumulating lots of > >> > backing storage specific processing into free_object, but that can be > >> > fixed up later (there is an obj->ops->release() pending). > >> > >> I'm more irked with the semantic overloading of object pinning. Might > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead > >> of pinning them for their entire lifetime. But we can bikeshed that > >> later on ;-) > > > > Some merit to that argument, but it still feels correct to say that the > > stolen pages are pinned for their lifetime. Given obj->ops->release(), > > it does actually become simpler to not mess around with pin_count. So > > later it is. > > I was more unhappy that pin_count has different meanings, until I've > noticed that we've fixed that up already with the introduction of > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that > one? After all unbinding from the gtt is ok with stolen memory, but > dropping the backing storage in the shrinker won't work. Not that we > currently use stolen for anything else than permanently pinned bos. As mentioned on irc, stolen does use the pages_pin_count for its purposes. The purpose of this patch is purely to allow sanity checking the pages_pin_count with a WARN_ON during free which seems sensible but not strictly required. -Chris
On Mon, Jun 03, 2013 at 09:36:48AM +0100, Chris Wilson wrote: > On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote: > > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote: > > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > >> > That neatly explains the WARN. Not too happy about accumulating lots of > > >> > backing storage specific processing into free_object, but that can be > > >> > fixed up later (there is an obj->ops->release() pending). > > >> > > >> I'm more irked with the semantic overloading of object pinning. Might > > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead > > >> of pinning them for their entire lifetime. But we can bikeshed that > > >> later on ;-) > > > > > > Some merit to that argument, but it still feels correct to say that the > > > stolen pages are pinned for their lifetime. Given obj->ops->release(), > > > it does actually become simpler to not mess around with pin_count. So > > > later it is. > > > > I was more unhappy that pin_count has different meanings, until I've > > noticed that we've fixed that up already with the introduction of > > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that > > one? After all unbinding from the gtt is ok with stolen memory, but > > dropping the backing storage in the shrinker won't work. Not that we > > currently use stolen for anything else than permanently pinned bos. > > As mentioned on irc, stolen does use the pages_pin_count for its > purposes. The purpose of this patch is purely to allow sanity checking > the pages_pin_count with a WARN_ON during free which seems sensible but > not strictly required. Yeah, silly me didn't read the code before sending a knee-jerk mail ;-) Series merged (hopefully in the right order and all, please check), thanks for the patches and review. -Daniel
On Mon, Jun 03, 2013 at 10:52:12AM +0200, Daniel Vetter wrote: > On Mon, Jun 03, 2013 at 09:36:48AM +0100, Chris Wilson wrote: > > On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote: > > > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote: > > > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > >> > That neatly explains the WARN. Not too happy about accumulating lots of > > > >> > backing storage specific processing into free_object, but that can be > > > >> > fixed up later (there is an obj->ops->release() pending). > > > >> > > > >> I'm more irked with the semantic overloading of object pinning. Might > > > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead > > > >> of pinning them for their entire lifetime. But we can bikeshed that > > > >> later on ;-) > > > > > > > > Some merit to that argument, but it still feels correct to say that the > > > > stolen pages are pinned for their lifetime. Given obj->ops->release(), > > > > it does actually become simpler to not mess around with pin_count. So > > > > later it is. > > > > > > I was more unhappy that pin_count has different meanings, until I've > > > noticed that we've fixed that up already with the introduction of > > > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that > > > one? After all unbinding from the gtt is ok with stolen memory, but > > > dropping the backing storage in the shrinker won't work. Not that we > > > currently use stolen for anything else than permanently pinned bos. > > > > As mentioned on irc, stolen does use the pages_pin_count for its > > purposes. The purpose of this patch is purely to allow sanity checking > > the pages_pin_count with a WARN_ON during free which seems sensible but > > not strictly required. > > Yeah, silly me didn't read the code before sending a knee-jerk mail ;-) > > Series merged (hopefully in the right order and all, please check), thanks > for the patches and review. > -Daniel > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch Order looks good. Thanks.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e5b6a92..a18a485 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3857,6 +3857,11 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) dev_priv->mm.interruptible = was_interruptible; } + /* Stolen objects don't hold a ref, but do hold pin count. Fix that up + * before progressing. */ + if (obj->stolen) + i915_gem_object_unpin_pages(obj); + obj->pages_pin_count = 0; i915_gem_object_put_pages(obj); i915_gem_object_free_mmap_offset(obj);
The way the stolen handling works is we take a pin on the backing pages, but we never actually get a reference to the bo. On freeing objects allocated with stolen memory, the final unref will end up freeing the object with pinned pages count left. To enable an assertion to catch bugs in this code path, this patch cleans up that remaining pin. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 5 +++++ 1 file changed, 5 insertions(+)