| Message ID | 1444132942-2025-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 06, 2015 at 01:02:22PM +0100, Chris Wilson wrote: > Since > > commit 43566dedde54f9729113f5f9fde77d53e75e61e9 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Jan 2 16:29:29 2015 +0530 > > drm/i915: Broaden application of set-domain(GTT) > > we allowed objects to be in the GTT domain, but unbound. Therefore > removing the GTT cache domain when removing the GGTT vma is no longer > semantically correct. > > An unfortunate side-effect is we lose the wondrously named > i915_gem_object_finish_gtt(), not to be confused with > i915_gem_gtt_finish_object()! > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Akash Goel <akash.goel@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 20 +++----------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8f498d4d874d..682af2ae3681 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3183,20 +3183,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > return 0; > } > > -static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) > -{ > - /* Force a pagefault for domain tracking on next user access */ > - i915_gem_release_mmap(obj); > - > - if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) > - return; > - > - obj->base.read_domains &= ~I915_GEM_DOMAIN_GTT; > - obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT; > - > - trace_i915_gem_object_change_domain(obj); > -} > - > int i915_vma_unbind(struct i915_vma *vma) > { > struct drm_i915_gem_object *obj = vma->obj; > @@ -3228,12 +3214,12 @@ int i915_vma_unbind(struct i915_vma *vma) > */ > > if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > - i915_gem_object_finish_gtt(obj); > - > - /* release the fence reg _after_ flushing */ > ret = i915_gem_object_put_fence(obj); > if (ret) > return ret; > + > + /* Force a pagefault for domain tracking on next user access */ > + i915_gem_release_mmap(obj); Can't put_fence before release_mmap ... I guess we should have a testcase for this somewhere? Hard to provoke probably ... -Daniel > } > > if (!vma->vm->closed) { > -- > 2.6.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 06, 2015 at 02:40:26PM +0200, Daniel Vetter wrote: > On Tue, Oct 06, 2015 at 01:02:22PM +0100, Chris Wilson wrote: > > Since > > > > commit 43566dedde54f9729113f5f9fde77d53e75e61e9 > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Fri Jan 2 16:29:29 2015 +0530 > > > > drm/i915: Broaden application of set-domain(GTT) > > > > we allowed objects to be in the GTT domain, but unbound. Therefore > > removing the GTT cache domain when removing the GGTT vma is no longer > > semantically correct. > > > > An unfortunate side-effect is we lose the wondrously named > > i915_gem_object_finish_gtt(), not to be confused with > > i915_gem_gtt_finish_object()! > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Akash Goel <akash.goel@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 20 +++----------------- > > 1 file changed, 3 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 8f498d4d874d..682af2ae3681 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3183,20 +3183,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > > return 0; > > } > > > > -static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) > > -{ > > - /* Force a pagefault for domain tracking on next user access */ > > - i915_gem_release_mmap(obj); > > - > > - if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) > > - return; > > - > > - obj->base.read_domains &= ~I915_GEM_DOMAIN_GTT; > > - obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT; > > - > > - trace_i915_gem_object_change_domain(obj); > > -} > > - > > int i915_vma_unbind(struct i915_vma *vma) > > { > > struct drm_i915_gem_object *obj = vma->obj; > > @@ -3228,12 +3214,12 @@ int i915_vma_unbind(struct i915_vma *vma) > > */ > > > > if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > > - i915_gem_object_finish_gtt(obj); > > - > > - /* release the fence reg _after_ flushing */ > > ret = i915_gem_object_put_fence(obj); > > if (ret) > > return ret; > > + > > + /* Force a pagefault for domain tracking on next user access */ > > + i915_gem_release_mmap(obj); > > Can't put_fence before release_mmap ... I guess we should have a testcase > for this somewhere? Hard to provoke probably ... Why not? i915_gem_object_put_fence() has to release the mmaps itself if it has a fence register assigned for the object. -Chris
On Tue, Oct 06, 2015 at 01:46:25PM +0100, Chris Wilson wrote: > On Tue, Oct 06, 2015 at 02:40:26PM +0200, Daniel Vetter wrote: > > On Tue, Oct 06, 2015 at 01:02:22PM +0100, Chris Wilson wrote: > > > Since > > > > > > commit 43566dedde54f9729113f5f9fde77d53e75e61e9 > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Fri Jan 2 16:29:29 2015 +0530 > > > > > > drm/i915: Broaden application of set-domain(GTT) > > > > > > we allowed objects to be in the GTT domain, but unbound. Therefore > > > removing the GTT cache domain when removing the GGTT vma is no longer > > > semantically correct. > > > > > > An unfortunate side-effect is we lose the wondrously named > > > i915_gem_object_finish_gtt(), not to be confused with > > > i915_gem_gtt_finish_object()! > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Akash Goel <akash.goel@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 20 +++----------------- > > > 1 file changed, 3 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > index 8f498d4d874d..682af2ae3681 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -3183,20 +3183,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > > > return 0; > > > } > > > > > > -static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) > > > -{ > > > - /* Force a pagefault for domain tracking on next user access */ > > > - i915_gem_release_mmap(obj); > > > - > > > - if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) > > > - return; > > > - > > > - obj->base.read_domains &= ~I915_GEM_DOMAIN_GTT; > > > - obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT; > > > - > > > - trace_i915_gem_object_change_domain(obj); > > > -} > > > - > > > int i915_vma_unbind(struct i915_vma *vma) > > > { > > > struct drm_i915_gem_object *obj = vma->obj; > > > @@ -3228,12 +3214,12 @@ int i915_vma_unbind(struct i915_vma *vma) > > > */ > > > > > > if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > > > - i915_gem_object_finish_gtt(obj); > > > - > > > - /* release the fence reg _after_ flushing */ > > > ret = i915_gem_object_put_fence(obj); > > > if (ret) > > > return ret; > > > + > > > + /* Force a pagefault for domain tracking on next user access */ > > > + i915_gem_release_mmap(obj); > > > > Can't put_fence before release_mmap ... I guess we should have a testcase > > for this somewhere? Hard to provoke probably ... > > Why not? i915_gem_object_put_fence() has to release the mmaps itself if > it has a fence register assigned for the object. Oh right, forgotten that put_fence is robust. Looking at this simply brought up bad memories since I fixed this kind of bug 5 years ago once ;-) Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8f498d4d874d..682af2ae3681 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3183,20 +3183,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, return 0; } -static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) -{ - /* Force a pagefault for domain tracking on next user access */ - i915_gem_release_mmap(obj); - - if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) - return; - - obj->base.read_domains &= ~I915_GEM_DOMAIN_GTT; - obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT; - - trace_i915_gem_object_change_domain(obj); -} - int i915_vma_unbind(struct i915_vma *vma) { struct drm_i915_gem_object *obj = vma->obj; @@ -3228,12 +3214,12 @@ int i915_vma_unbind(struct i915_vma *vma) */ if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { - i915_gem_object_finish_gtt(obj); - - /* release the fence reg _after_ flushing */ ret = i915_gem_object_put_fence(obj); if (ret) return ret; + + /* Force a pagefault for domain tracking on next user access */ + i915_gem_release_mmap(obj); } if (!vma->vm->closed) {
Since commit 43566dedde54f9729113f5f9fde77d53e75e61e9 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Jan 2 16:29:29 2015 +0530 drm/i915: Broaden application of set-domain(GTT) we allowed objects to be in the GTT domain, but unbound. Therefore removing the GTT cache domain when removing the GGTT vma is no longer semantically correct. An unfortunate side-effect is we lose the wondrously named i915_gem_object_finish_gtt(), not to be confused with i915_gem_gtt_finish_object()! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Akash Goel <akash.goel@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)