| Message ID | 1470997701-988-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On pe, 2016-08-12 at 11:28 +0100, Chris Wilson wrote: > Keep any error reported by the gup_worker until we are notified that the > arena has changed (via the mmu-notifier). This has the importance of > making two consecutive calls to i915_gem_object_get_pages() reporting > the same error, and curtailing an loop of detecting a fault and requeueing > a gup_worker. > I think this is for Mika to review. Regards, Joonas > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 57218cca7e05..be54825ef3e8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -542,8 +542,6 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > } > obj->userptr.work = ERR_PTR(ret); > - if (ret) > - __i915_gem_userptr_set_active(obj, false); > } > > obj->userptr.workers--; > @@ -628,15 +626,14 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > * to the vma (discard or cloning) which should prevent the more > * egregious cases from causing harm. > */ > - if (IS_ERR(obj->userptr.work)) { > - /* active flag will have been dropped already by the worker */ > - ret = PTR_ERR(obj->userptr.work); > - obj->userptr.work = NULL; > - return ret; > - } > - if (obj->userptr.work) > + > + if (obj->userptr.work) { > /* active flag should still be held for the pending work */ > - return -EAGAIN; > + if (IS_ERR(obj->userptr.work)) > + return PTR_ERR(obj->userptr.work); > + else > + return -EAGAIN; > + } > > /* Let the mmu-notifier know that we have begun and need cancellation */ > ret = __i915_gem_userptr_set_active(obj, true);
Chris Wilson <chris@chris-wilson.co.uk> writes: > Keep any error reported by the gup_worker until we are notified that the > arena has changed (via the mmu-notifier). This has the importance of > making two consecutive calls to i915_gem_object_get_pages() reporting > the same error, and curtailing an loop of detecting a fault and requeueing > a gup_worker. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 57218cca7e05..be54825ef3e8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -542,8 +542,6 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > } > obj->userptr.work = ERR_PTR(ret); > - if (ret) > - __i915_gem_userptr_set_active(obj, false); > } > > obj->userptr.workers--; > @@ -628,15 +626,14 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > * to the vma (discard or cloning) which should prevent the more > * egregious cases from causing harm. > */ > - if (IS_ERR(obj->userptr.work)) { > - /* active flag will have been dropped already by the worker */ > - ret = PTR_ERR(obj->userptr.work); > - obj->userptr.work = NULL; > - return ret; > - } > - if (obj->userptr.work) > + > + if (obj->userptr.work) { > /* active flag should still be held for the pending work */ > - return -EAGAIN; > + if (IS_ERR(obj->userptr.work)) > + return PTR_ERR(obj->userptr.work); Previously you did set the work to null before returning error, now you dont. Is it the responsibility of cancel_userptr now, through mm notifier, that clears the pointer? -Mika > + else > + return -EAGAIN; > + } > > /* Let the mmu-notifier know that we have begun and need cancellation */ > ret = __i915_gem_userptr_set_active(obj, true); > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Aug 15, 2016 at 06:08:21PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > + if (obj->userptr.work) { > > /* active flag should still be held for the pending work */ > > - return -EAGAIN; > > + if (IS_ERR(obj->userptr.work)) > > + return PTR_ERR(obj->userptr.work); > > Previously you did set the work to null before returning error, > now you dont. > > Is it the responsibility of cancel_userptr now, through mm notifier, > that clears the pointer? Yes. > > Keep any error reported by the gup_worker until we are notified that the > > arena has changed (via the mmu-notifier). This has the importance of > > making two consecutive calls to i915_gem_object_get_pages() reporting > > the same error, and curtailing a loop of detecting a fault and requeueing > > a gup_worker. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Keep any error reported by the gup_worker until we are notified that the > arena has changed (via the mmu-notifier). This has the importance of > making two consecutive calls to i915_gem_object_get_pages() reporting > the same error, and curtailing an loop of detecting a fault and requeueing > a gup_worker. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 57218cca7e05..be54825ef3e8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -542,8 +542,6 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > } > obj->userptr.work = ERR_PTR(ret); > - if (ret) > - __i915_gem_userptr_set_active(obj, false); > } > > obj->userptr.workers--; > @@ -628,15 +626,14 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > * to the vma (discard or cloning) which should prevent the more > * egregious cases from causing harm. > */ > - if (IS_ERR(obj->userptr.work)) { > - /* active flag will have been dropped already by the worker */ > - ret = PTR_ERR(obj->userptr.work); > - obj->userptr.work = NULL; > - return ret; > - } > - if (obj->userptr.work) > + > + if (obj->userptr.work) { > /* active flag should still be held for the pending work */ > - return -EAGAIN; > + if (IS_ERR(obj->userptr.work)) > + return PTR_ERR(obj->userptr.work); > + else > + return -EAGAIN; > + } > > /* Let the mmu-notifier know that we have begun and need cancellation */ > ret = __i915_gem_userptr_set_active(obj, true); > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index 57218cca7e05..be54825ef3e8 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -542,8 +542,6 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) } } obj->userptr.work = ERR_PTR(ret); - if (ret) - __i915_gem_userptr_set_active(obj, false); } obj->userptr.workers--; @@ -628,15 +626,14 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) * to the vma (discard or cloning) which should prevent the more * egregious cases from causing harm. */ - if (IS_ERR(obj->userptr.work)) { - /* active flag will have been dropped already by the worker */ - ret = PTR_ERR(obj->userptr.work); - obj->userptr.work = NULL; - return ret; - } - if (obj->userptr.work) + + if (obj->userptr.work) { /* active flag should still be held for the pending work */ - return -EAGAIN; + if (IS_ERR(obj->userptr.work)) + return PTR_ERR(obj->userptr.work); + else + return -EAGAIN; + } /* Let the mmu-notifier know that we have begun and need cancellation */ ret = __i915_gem_userptr_set_active(obj, true);
Keep any error reported by the gup_worker until we are notified that the arena has changed (via the mmu-notifier). This has the importance of making two consecutive calls to i915_gem_object_get_pages() reporting the same error, and curtailing an loop of detecting a fault and requeueing a gup_worker. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_userptr.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)