[PATCH] drm/i915: Relinquish any fence when changing cache levels

[PATCH] drm/i915: Relinquish any fence when changing cache levels

[Chris Wilson]

This is vital to maintain our contract with the hw for not using fences
on snooped memory for older chipsets. It should have no impact
other than clearing the fence register (and updating the fence
bookkeeping) as any future IO access (page faults or pwrite/pread) will
go through the cached CPU domain for older chipsets. On SandyBridge, we
incur an extra get_fence() on the rare path that we need to perform
detiling through a pagefault (i.e. texture transfers).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ca026d..2d16a23 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3355,6 +3355,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		if (ret)
 			return ret;
 
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			return ret;
+
 		ret = i915_gem_gtt_bind_object(obj, cache_level);
 		if (ret)
 			return ret;
-- 
1.7.4.1

Re: [PATCH] drm/i915: Relinquish any fence when changing cache levels

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1577 bytes --]

On Wed, 13 Apr 2011 13:25:40 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This is vital to maintain our contract with the hw for not using fences
> on snooped memory for older chipsets. It should have no impact
> other than clearing the fence register (and updating the fence
> bookkeeping) as any future IO access (page faults or pwrite/pread) will
> go through the cached CPU domain for older chipsets. On SandyBridge, we
> incur an extra get_fence() on the rare path that we need to perform
> detiling through a pagefault (i.e. texture transfers).

Surely you could just update this to do that for the hardware that
requires it.  With a comment so someone doesn't delete it later :)

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ca026d..2d16a23 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3355,6 +3355,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		if (ret)
>  			return ret;
>  
> +		ret = i915_gem_object_put_fence(obj);
> +		if (ret)
> +			return ret;
> +
>  		ret = i915_gem_gtt_bind_object(obj, cache_level);
>  		if (ret)
>  			return ret;
> -- 
> 1.7.4.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Relinquish any fence when changing cache levels

[Chris Wilson]

On Wed, 13 Apr 2011 09:36:08 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Wed, 13 Apr 2011 13:25:40 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > This is vital to maintain our contract with the hw for not using fences
> > on snooped memory for older chipsets. It should have no impact
> > other than clearing the fence register (and updating the fence
> > bookkeeping) as any future IO access (page faults or pwrite/pread) will
> > go through the cached CPU domain for older chipsets. On SandyBridge, we
> > incur an extra get_fence() on the rare path that we need to perform
> > detiling through a pagefault (i.e. texture transfers).
> 
> Surely you could just update this to do that for the hardware that
> requires it.  With a comment so someone doesn't delete it later :)

The comment is surely lacking, yes. But the test here for the right
generations is just ugly since losing a CPU fence register is not that
big an issue -- the largest overhead will be in reinstating the vma, and
we need to do that anyway if we mix CPU / GTT reads through the pagefault
handler.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Relinquish any fence when changing cache levels

[Chris Wilson]

This is vital to maintain our contract with the hw for not using fences
on snooped memory for older chipsets. It should have no impact
other than clearing the fence register (and updating the fence
bookkeeping) as any future IO access (page faults or pwrite/pread) will
be linear and go through the cached CPU domain.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c38b011..39c50eb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3358,6 +3358,16 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		if (ret)
 			return ret;
 
+		/* Before SandyBridge, you could not use tiling or fence
+		 * registers with snooped memory, so relinquish any fences
+		 * currently pointing to our region in the aperture.
+		 */
+		if (INTEL_INFO(obj->base.dev)->gen < 6) {
+			ret = i915_gem_object_put_fence(obj);
+			if (ret)
+				return ret;
+		}
+
 		ret = i915_gem_gtt_bind_object(obj, cache_level);
 		if (ret)
 			return ret;
-- 
1.7.4.1

Re: [PATCH] drm/i915: Relinquish any fence when changing cache levels

[Daniel Vetter]

On Wed, Apr 13, 2011 at 07:01:11PM +0100, Chris Wilson wrote:
> This is vital to maintain our contract with the hw for not using fences
> on snooped memory for older chipsets. It should have no impact
> other than clearing the fence register (and updating the fence
> bookkeeping) as any future IO access (page faults or pwrite/pread) will
> be linear and go through the cached CPU domain.

set_cache_level is rather unused on pre-snb. Can't we just hold off with
such complexity before we actually use it in e.g. the vmap code? Maybe add
a WARN_ON(gen < 6) instead?

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Relinquish any fence when changing cache levels

[Chris Wilson]

On Wed, 13 Apr 2011 22:46:47 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> set_cache_level is rather unused on pre-snb. Can't we just hold off with
> such complexity before we actually use it in e.g. the vmap code? Maybe add
> a WARN_ON(gen < 6) instead?

You might argue that set_cache_level is not used even on SNB at the point at
which we are writing the interface for it...

Once you've finished reviewing this code, I have another batch... ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre