diff mbox

[12/13] drm/i915: Prevent mixing of snooped and tiling modes for old chipsets

Message ID 1302771827-26112-13-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson April 14, 2011, 9:03 a.m. UTC
Older chipsets do not support snooping (i.e. cache sharing between the
CPU and GPU) on tiled surfaces. So prevent userspace from being silly
should we one day expose the ability to change cache levels from
userspace.

It does enforce a strict ordering for mode changing though. So in order
to change a buffer to snooped, the driver has to clear any tiling first
and then change the cache level. This is consistent with how we flush
and update the PTEs and seems a reasonable imposition on the driver.
Deferring the check until use, whilst providing flexibilty here, implies
forcing extra unbinds and a more complicated error message from, for
example, execbuffer.

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

Comments

Daniel Vetter April 14, 2011, 5:43 p.m. UTC | #1
On Thu, Apr 14, 2011 at 10:03:46AM +0100, Chris Wilson wrote:
> Older chipsets do not support snooping (i.e. cache sharing between the
> CPU and GPU) on tiled surfaces. So prevent userspace from being silly
> should we one day expose the ability to change cache levels from
> userspace.
> 
> It does enforce a strict ordering for mode changing though. So in order
> to change a buffer to snooped, the driver has to clear any tiling first
> and then change the cache level. This is consistent with how we flush
> and update the PTEs and seems a reasonable imposition on the driver.
> Deferring the check until use, whilst providing flexibilty here, implies
> forcing extra unbinds and a more complicated error message from, for
> example, execbuffer.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Small error in the debug output below.

> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 281ad3d..ca69fd4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -331,6 +331,14 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  	}
>  
>  	mutex_lock(&dev->struct_mutex);
> +	if (INTEL_INFO(dev)->gen < 6 &&
> +	    args->tiling_mode != I915_TILING_NONE &&
> +	    obj->cache_level != I915_CACHE_NONE) {
> +		DRM_DEBUG("can't not set a tiling mode on snooped memory,"

One negation too much.

> +			  "it must be linear for pre-SandyBridge chipsets\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
>  	if (args->tiling_mode != obj->tiling_mode ||
>  	    args->stride != obj->stride) {
>  		/* We need to rebind the object if its current allocation
Chris Wilson April 14, 2011, 8:26 p.m. UTC | #2
On Thu, 14 Apr 2011 19:43:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 14, 2011 at 10:03:46AM +0100, Chris Wilson wrote:
> > +	if (INTEL_INFO(dev)->gen < 6 &&
> > +	    args->tiling_mode != I915_TILING_NONE &&
> > +	    obj->cache_level != I915_CACHE_NONE) {
> > +		DRM_DEBUG("can't not set a tiling mode on snooped memory,"
> 
> One negation too much.

D'oh. That was me trying to be extra careful and not succumb to my usual
habit of over-using contractions. Thanks,
-Chris
Keith Packard May 4, 2011, 5:32 p.m. UTC | #3
On Thu, 14 Apr 2011 10:03:46 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Older chipsets do not support snooping (i.e. cache sharing between the
> CPU and GPU) on tiled surfaces. So prevent userspace from being silly
> should we one day expose the ability to change cache levels from
> userspace.

We will not need this as there is no plan to have user-mode use snooped
access on older chipsets.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f57f99..46b63c3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3053,6 +3053,14 @@  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	if (obj->cache_level == cache_level)
 		return 0;
 
+	if (INTEL_INFO(obj->base.dev)->gen < 6 &&
+	    obj->tiling_mode != I915_TILING_NONE &&
+	    cache_level != I915_CACHE_NONE) {
+		DRM_DEBUG("can not enable snooping on a tiled surface, "
+			  "it must be linear for pre-SandyBridge chipsets\n");
+		return -EINVAL;
+	}
+
 	if (obj->pin_count) {
 		DRM_DEBUG("can not change the cache level of pinned objects\n");
 		return -EBUSY;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 281ad3d..ca69fd4 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -331,6 +331,14 @@  i915_gem_set_tiling(struct drm_device *dev, void *data,
 	}
 
 	mutex_lock(&dev->struct_mutex);
+	if (INTEL_INFO(dev)->gen < 6 &&
+	    args->tiling_mode != I915_TILING_NONE &&
+	    obj->cache_level != I915_CACHE_NONE) {
+		DRM_DEBUG("can't not set a tiling mode on snooped memory,"
+			  "it must be linear for pre-SandyBridge chipsets\n");
+		ret = -EINVAL;
+		goto err;
+	}
 	if (args->tiling_mode != obj->tiling_mode ||
 	    args->stride != obj->stride) {
 		/* We need to rebind the object if its current allocation
@@ -360,6 +368,7 @@  i915_gem_set_tiling(struct drm_device *dev, void *data,
 		}
 	}
 	/* we have to maintain this existing ABI... */
+err:
 	args->stride = obj->stride;
 	args->tiling_mode = obj->tiling_mode;
 	drm_gem_object_unreference(&obj->base);