| Message ID | 1376442549-5087-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 13, 2013 at 06:09:06PM -0700, Ben Widawsky wrote: > VMAs can be created and not bound. One may think of it as lazy cleanup, > and safely gloss over the conditions which manufacture it. In either > case, when the object backing the i915 vma is destroyed, we must cleanup > the vma without stumbling into a bunch of pitfalls that assume the vma > is bound. > > NOTE: I was pretty certain the above condition could only happen when we > introduced the use of VMAs being looked up at execbuf, and already > existing. Paulo has hit this though, so I must be missing something. As > I believe the patch is correct anyway, therefore I won't scratch my head > too hard. If we end up calling evict_everything from i915_gem_object_bind_to_vm then we'll hit this. One more reason for a testcase here ;-) I'll amend the commit message and merge this. I've also applied a tiny bikeshed I've created while reviewing existing vma_create/destroy callsites. -Daniel > > v2: use goto destroy as a compromise (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 3d9e248b..4a58ead 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2606,6 +2606,9 @@ int i915_vma_unbind(struct i915_vma *vma) > if (list_empty(&vma->vma_link)) > return 0; > > + if (!drm_mm_node_allocated(&vma->node)) > + goto destroy; > + > if (obj->pin_count) > return -EBUSY; > > @@ -2643,6 +2646,8 @@ int i915_vma_unbind(struct i915_vma *vma) > obj->map_and_fenceable = true; > > drm_mm_remove_node(&vma->node); > + > +destroy: > i915_gem_vma_destroy(vma); > > /* Since the unbound list is global, only move to that list if > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 14, 2013 at 10:06:30AM +0200, Daniel Vetter wrote: > On Tue, Aug 13, 2013 at 06:09:06PM -0700, Ben Widawsky wrote: > > VMAs can be created and not bound. One may think of it as lazy cleanup, > > and safely gloss over the conditions which manufacture it. In either > > case, when the object backing the i915 vma is destroyed, we must cleanup > > the vma without stumbling into a bunch of pitfalls that assume the vma > > is bound. > > > > NOTE: I was pretty certain the above condition could only happen when we > > introduced the use of VMAs being looked up at execbuf, and already > > existing. Paulo has hit this though, so I must be missing something. As > > I believe the patch is correct anyway, therefore I won't scratch my head > > too hard. > > If we end up calling evict_everything from i915_gem_object_bind_to_vm then > we'll hit this. One more reason for a testcase here ;-) I'll amend the > commit message and merge this. I've also applied a tiny bikeshed I've > created while reviewing existing vma_create/destroy callsites. Actually evict_everything isn't in the callpath, and there's no memory allocation where the shrinker might play havoc. Furthermore the pages are pinned so the shrinker shouldn't be able to sneak in at all. This is a bit unsettling, I need to think more about this. I'll wait with merging this for now. -Daniel
On Tue, Aug 13, 2013 at 06:09:06PM -0700, Ben Widawsky wrote: > VMAs can be created and not bound. One may think of it as lazy cleanup, > and safely gloss over the conditions which manufacture it. In either > case, when the object backing the i915 vma is destroyed, we must cleanup > the vma without stumbling into a bunch of pitfalls that assume the vma > is bound. > > NOTE: I was pretty certain the above condition could only happen when we > introduced the use of VMAs being looked up at execbuf, and already > existing. Paulo has hit this though, so I must be missing something. As > I believe the patch is correct anyway, therefore I won't scratch my head > too hard. > > v2: use goto destroy as a compromise (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, Aug 14, 2013 at 10:15:50AM +0200, Daniel Vetter wrote: > On Wed, Aug 14, 2013 at 10:06:30AM +0200, Daniel Vetter wrote: > > On Tue, Aug 13, 2013 at 06:09:06PM -0700, Ben Widawsky wrote: > > > VMAs can be created and not bound. One may think of it as lazy cleanup, > > > and safely gloss over the conditions which manufacture it. In either > > > case, when the object backing the i915 vma is destroyed, we must cleanup > > > the vma without stumbling into a bunch of pitfalls that assume the vma > > > is bound. > > > > > > NOTE: I was pretty certain the above condition could only happen when we > > > introduced the use of VMAs being looked up at execbuf, and already > > > existing. Paulo has hit this though, so I must be missing something. As > > > I believe the patch is correct anyway, therefore I won't scratch my head > > > too hard. > > > > If we end up calling evict_everything from i915_gem_object_bind_to_vm then > > we'll hit this. One more reason for a testcase here ;-) I'll amend the > > commit message and merge this. I've also applied a tiny bikeshed I've > > created while reviewing existing vma_create/destroy callsites. > > Actually evict_everything isn't in the callpath, and there's no memory > allocation where the shrinker might play havoc. Furthermore the pages are > pinned so the shrinker shouldn't be able to sneak in at all. This is a bit > unsettling, I need to think more about this. > > I'll wait with merging this for now. Ok, I've merged the entire pile. I think now's the time to throw a bit of igt on top to exercise the corner cases here ... -Daniel
On Thu, Aug 15, 2013 at 04:05:56PM +0200, Daniel Vetter wrote: > On Wed, Aug 14, 2013 at 10:15:50AM +0200, Daniel Vetter wrote: > > On Wed, Aug 14, 2013 at 10:06:30AM +0200, Daniel Vetter wrote: > > > On Tue, Aug 13, 2013 at 06:09:06PM -0700, Ben Widawsky wrote: > > > > VMAs can be created and not bound. One may think of it as lazy cleanup, > > > > and safely gloss over the conditions which manufacture it. In either > > > > case, when the object backing the i915 vma is destroyed, we must cleanup > > > > the vma without stumbling into a bunch of pitfalls that assume the vma > > > > is bound. > > > > > > > > NOTE: I was pretty certain the above condition could only happen when we > > > > introduced the use of VMAs being looked up at execbuf, and already > > > > existing. Paulo has hit this though, so I must be missing something. As > > > > I believe the patch is correct anyway, therefore I won't scratch my head > > > > too hard. > > > > > > If we end up calling evict_everything from i915_gem_object_bind_to_vm then > > > we'll hit this. One more reason for a testcase here ;-) I'll amend the > > > commit message and merge this. I've also applied a tiny bikeshed I've > > > created while reviewing existing vma_create/destroy callsites. > > > > Actually evict_everything isn't in the callpath, and there's no memory > > allocation where the shrinker might play havoc. Furthermore the pages are > > pinned so the shrinker shouldn't be able to sneak in at all. This is a bit > > unsettling, I need to think more about this. > > > > I'll wait with merging this for now. > > Ok, I've merged the entire pile. I think now's the time to throw a bit of > igt on top to exercise the corner cases here ... > -Daniel Was that a specific request for me to do something, I missed the message if so? When should I rework the remaining patches and resend?
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3d9e248b..4a58ead 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2606,6 +2606,9 @@ int i915_vma_unbind(struct i915_vma *vma) if (list_empty(&vma->vma_link)) return 0; + if (!drm_mm_node_allocated(&vma->node)) + goto destroy; + if (obj->pin_count) return -EBUSY; @@ -2643,6 +2646,8 @@ int i915_vma_unbind(struct i915_vma *vma) obj->map_and_fenceable = true; drm_mm_remove_node(&vma->node); + +destroy: i915_gem_vma_destroy(vma); /* Since the unbound list is global, only move to that list if
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound. NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard. v2: use goto destroy as a compromise (Chris) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 5 +++++ 1 file changed, 5 insertions(+)