[PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Chris Wilson]

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 | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ffc4a619849..1ea94aec41d1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3171,23 +3171,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;
-
-	/* Wait for any direct GTT access to complete */
-	mb();
-
-	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;
@@ -3219,12 +3202,13 @@ 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);
 	}
 
 	trace_i915_vma_unbind(vma);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Daniel Vetter]

On Thu, Apr 23, 2015 at 12:54:37PM +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()!

What are we going to do without that bit of hilarity ...
> 
> 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>

Queued for -next (with the rebase conflict resolved because I don't have
your tree), thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ffc4a619849..1ea94aec41d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3171,23 +3171,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;
> -
> -	/* Wait for any direct GTT access to complete */
> -	mb();
> -
> -	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;
> @@ -3219,12 +3202,13 @@ 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);
>  	}
>  
>  	trace_i915_vma_unbind(vma);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Daniel Vetter]

On Mon, May 04, 2015 at 04:14:41PM +0200, Daniel Vetter wrote:
> On Thu, Apr 23, 2015 at 12:54:37PM +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()!
> 
> What are we going to do without that bit of hilarity ...
> > 
> > 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>
> 
> Queued for -next (with the rebase conflict resolved because I don't have
> your tree), thanks for the patch.

Well not since there's another finish_gtt in my tree in set_cache_level.
And tbh I'm not sure what to do with that one, but it seems rather
redundnant. Care to respin/rebase?
-Daniel

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6ffc4a619849..1ea94aec41d1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3171,23 +3171,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;
> > -
> > -	/* Wait for any direct GTT access to complete */
> > -	mb();
> > -
> > -	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;
> > @@ -3219,12 +3202,13 @@ 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);
> >  	}
> >  
> >  	trace_i915_vma_unbind(vma);
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Chris Wilson]

On Mon, May 04, 2015 at 04:21:20PM +0200, Daniel Vetter wrote:
> On Mon, May 04, 2015 at 04:14:41PM +0200, Daniel Vetter wrote:
> > On Thu, Apr 23, 2015 at 12:54:37PM +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()!
> > 
> > What are we going to do without that bit of hilarity ...
> > > 
> > > 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>
> > 
> > Queued for -next (with the rebase conflict resolved because I don't have
> > your tree), thanks for the patch.
> 
> Well not since there's another finish_gtt in my tree in set_cache_level.
> And tbh I'm not sure what to do with that one, but it seems rather
> redundnant. Care to respin/rebase?

There's a patch on list to remove it, I think I am about 90% of the way
through convincing Tvrtko that that patch is golden.

Between Akash any myself we have a had a little debate over whether we
want to keep the mb() here or not. The consensus, at least how I have the
patch in my tree, is to use:

   if (vma->map_and_fenceable) {
                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);

                /* Wait for any direct GTT access to complete */
                if (obj->base.read_domains & I915_GEM_DOMAIN_GTT)
                        mb();
        }

as the replacement. The worry is that we don't have sufficient barriers
between the user doing their GTT writes through one BAR and updating the
PTE through another.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Chris Wilson]

On Tue, Oct 06, 2015 at 01:28:07PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 11:39:56AM +0100, Chris Wilson wrote:
> > Since the remove of the pin-ioctl, we only care about not changing the
> > cache level on buffers pinned to the hardware as indicated by
> > obj->pin_display. So we can safely replace i915_gem_object_is_pinned()
> > here with a plain obj->pin_display check. During rebinding, we will check
> > sanity checks in case vma->pin_count is erroneously set.
> > 
> > At the same time, we can micro-optimise GTT mmap() behaviour since we
> > only need to relinquish the mmaps before Sandybridge.
> 
> Actual condition is !LLC so would need to be updated (and split out imo).
>  
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d4a3bdf0c5b6..2b8ed7a2faab 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3629,31 +3629,34 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct i915_vma *vma, *next;
> > +	bool bound = false;
> >  	int ret = 0;
> >  
> >  	if (obj->cache_level == cache_level)
> >  		goto out;
> >  
> > -	if (i915_gem_obj_is_pinned(obj)) {
> > -		DRM_DEBUG("can not change the cache level of pinned objects\n");
> > -		return -EBUSY;
> > -	}
> > -
> >  	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> > +		if (!drm_mm_node_allocated(&vma->node))
> > +			continue;
> > +
> > +		if (vma->pin_count) {
> > +			DRM_DEBUG("can not change the cache level of pinned objects\n");
> > +			return -EBUSY;
> > +		}
> > +
> >  		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> >  			ret = i915_vma_unbind(vma);
> >  			if (ret)
> >  				return ret;
> > -		}
> > +		} else
> > +			bound = true;
> >  	}
> >  
> > -	if (i915_gem_obj_bound_any(obj)) {
> > +	if (bound) {
> >  		ret = i915_gem_object_wait_rendering(obj, false);
> >  		if (ret)
> >  			return ret;
> 
> Shouldn't the below be split out into a separate patch? And maybe for
> paranoia keep calling finish_gtt but restrict it to !LLC && snooped like
> you do below.

Hmm, I don't have a finish-gtt. The serialisation is based on
release-mmaps (we have to be sure that any concurrent access is
prohibited). So the question is: is i915_gem_release_mmap() a sufficient
barrier and if not, why not. In release-mmap we are revoking the CPU's PTE,
but that can be ordered with the memory accesses, but before we continue
we should be sure that they have been revoked. Paranoia says we should
be moving the mb() we have from outside of release-mmaps into
release-mmaps.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Chris Wilson]

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);
 	}
 
 	if (!vma->vm->closed) {
-- 
2.6.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop discarding GTT cache-domain on unbind vma

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx