[PATCH 1/2] drm/i915: Kill DRI1 cliprects

[PATCH 1/2] drm/i915: Kill DRI1 cliprects

[Chris Wilson]

Passing cliprects into the kernel for it to re-execute the batch buffer
with different CMD_DRAWRECT died out long ago. As DRI1 support has been
removed from the kernel, we can now simply reject any execbuf trying to
use this "feature".

To keep Daniel happy with the prospect of being able to reuse these
fields in the next decade, continue to ensure that current userspace is
not passing garbage in through the dead fields.

v2: Fix the cliprects_ptr check

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
 drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
 2 files changed, 31 insertions(+), 138 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 75a0c8b5305b..045a7631faa0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -947,7 +947,21 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
 	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
 		return false;
 
-	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
+	/* Kernel clipping was a DRI1 misfeature */
+	if (exec->num_cliprects || exec->cliprects_ptr)
+		return false;
+
+	if (exec->DR4 == 0xffffffff) {
+		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
+		exec->DR4 = 0;
+	}
+	if (exec->DR1 || exec->DR4)
+		return false;
+
+	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
+		return false;
+
+	return true;
 }
 
 static int
@@ -1111,47 +1125,6 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-i915_emit_box(struct drm_i915_gem_request *req,
-	      struct drm_clip_rect *box,
-	      int DR1, int DR4)
-{
-	struct intel_engine_cs *ring = req->ring;
-	int ret;
-
-	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
-	    box->y2 <= 0 || box->x2 <= 0) {
-		DRM_ERROR("Bad box %d,%d..%d,%d\n",
-			  box->x1, box->y1, box->x2, box->y2);
-		return -EINVAL;
-	}
-
-	if (INTEL_INFO(ring->dev)->gen >= 4) {
-		ret = intel_ring_begin(req, 4);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
-		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
-		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
-		intel_ring_emit(ring, DR4);
-	} else {
-		ret = intel_ring_begin(req, 6);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
-		intel_ring_emit(ring, DR1);
-		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
-		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
-		intel_ring_emit(ring, DR4);
-		intel_ring_emit(ring, 0);
-	}
-	intel_ring_advance(ring);
-
-	return 0;
-}
-
 static struct drm_i915_gem_object*
 i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
 			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
@@ -1210,65 +1183,21 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 			       struct drm_i915_gem_execbuffer2 *args,
 			       struct list_head *vmas)
 {
-	struct drm_clip_rect *cliprects = NULL;
 	struct drm_device *dev = params->dev;
 	struct intel_engine_cs *ring = params->ring;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u64 exec_start, exec_len;
 	int instp_mode;
 	u32 instp_mask;
-	int i, ret = 0;
-
-	if (args->num_cliprects != 0) {
-		if (ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
-			return -EINVAL;
-		}
-
-		if (INTEL_INFO(dev)->gen >= 5) {
-			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
-			return -EINVAL;
-		}
-
-		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
-			DRM_DEBUG("execbuf with %u cliprects\n",
-				  args->num_cliprects);
-			return -EINVAL;
-		}
-
-		cliprects = kcalloc(args->num_cliprects,
-				    sizeof(*cliprects),
-				    GFP_KERNEL);
-		if (cliprects == NULL) {
-			ret = -ENOMEM;
-			goto error;
-		}
-
-		if (copy_from_user(cliprects,
-				   to_user_ptr(args->cliprects_ptr),
-				   sizeof(*cliprects)*args->num_cliprects)) {
-			ret = -EFAULT;
-			goto error;
-		}
-	} else {
-		if (args->DR4 == 0xffffffff) {
-			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
-			args->DR4 = 0;
-		}
-
-		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
-			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
-			return -EINVAL;
-		}
-	}
+	int ret;
 
 	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
 	if (ret)
-		goto error;
+		return ret;
 
 	ret = i915_switch_context(params->request);
 	if (ret)
-		goto error;
+		return ret;
 
 	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
 	     "%s didn't clear reload\n", ring->name);
@@ -1281,22 +1210,19 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 	case I915_EXEC_CONSTANTS_REL_SURFACE:
 		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
 			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
-			ret = -EINVAL;
-			goto error;
+			return -EINVAL;
 		}
 
 		if (instp_mode != dev_priv->relative_constants_mode) {
 			if (INTEL_INFO(dev)->gen < 4) {
 				DRM_DEBUG("no rel constants on pre-gen4\n");
-				ret = -EINVAL;
-				goto error;
+				return -EINVAL;
 			}
 
 			if (INTEL_INFO(dev)->gen > 5 &&
 			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
 				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				ret = -EINVAL;
-				goto error;
+				return -EINVAL;
 			}
 
 			/* The HW changed the meaning on this bit on gen6 */
@@ -1306,15 +1232,14 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 		break;
 	default:
 		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	if (ring == &dev_priv->ring[RCS] &&
-			instp_mode != dev_priv->relative_constants_mode) {
+	    instp_mode != dev_priv->relative_constants_mode) {
 		ret = intel_ring_begin(params->request, 4);
 		if (ret)
-			goto error;
+			return ret;
 
 		intel_ring_emit(ring, MI_NOOP);
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
@@ -1328,42 +1253,25 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
 		ret = i915_reset_gen7_sol_offsets(dev, params->request);
 		if (ret)
-			goto error;
+			return ret;
 	}
 
 	exec_len   = args->batch_len;
 	exec_start = params->batch_obj_vm_offset +
 		     params->args_batch_start_offset;
 
-	if (cliprects) {
-		for (i = 0; i < args->num_cliprects; i++) {
-			ret = i915_emit_box(params->request, &cliprects[i],
-					    args->DR1, args->DR4);
-			if (ret)
-				goto error;
-
-			ret = ring->dispatch_execbuffer(params->request,
-							exec_start, exec_len,
-							params->dispatch_flags);
-			if (ret)
-				goto error;
-		}
-	} else {
-		ret = ring->dispatch_execbuffer(params->request,
-						exec_start, exec_len,
-						params->dispatch_flags);
-		if (ret)
-			return ret;
-	}
+	ret = ring->dispatch_execbuffer(params->request,
+					exec_start, exec_len,
+					params->dispatch_flags);
+	if (ret)
+		return ret;
 
 	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
 
 	i915_gem_execbuffer_move_to_active(vmas, params->request);
 	i915_gem_execbuffer_retire_commands(params);
 
-error:
-	kfree(cliprects);
-	return ret;
+	return 0;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 78aab946db04..d38746c5370d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -878,21 +878,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 		return -EINVAL;
 	}
 
-	if (args->num_cliprects != 0) {
-		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
-		return -EINVAL;
-	} else {
-		if (args->DR4 == 0xffffffff) {
-			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
-			args->DR4 = 0;
-		}
-
-		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
-			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
-			return -EINVAL;
-		}
-	}
-
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
 		DRM_DEBUG("sol reset is gen7 only\n");
 		return -EINVAL;
-- 
2.6.0

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

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

[Chris Wilson]

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.

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;
 
-		i915_gem_object_finish_gtt(obj);
-
 		/* 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.
@@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				return ret;
 		}
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (drm_mm_node_allocated(&vma->node)) {
-				ret = i915_vma_bind(vma, cache_level,
-						    PIN_UPDATE);
-				if (ret)
-					return ret;
-			}
+		/* Access to snoopable pages through the GTT is incoherent. */
+		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
+			i915_gem_release_mmap(obj);
+
+		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+			if (!drm_mm_node_allocated(&vma->node))
+				continue;
+
+			ret = i915_vma_bind(vma, cache_level, PIN_UPDATE);
+			if (ret)
+				return ret;
+		}
 	}
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
-- 
2.6.0

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

Re: [PATCH 1/2] drm/i915: Kill DRI1 cliprects

[Daniel Vetter]

On Tue, Oct 06, 2015 at 11:39:55AM +0100, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
> 
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
> 
> v2: Fix the cliprects_ptr check
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

igt subtest seems to be missing to ensure we enforce this. Yay otherwise!
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
>  drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
>  2 files changed, 31 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 75a0c8b5305b..045a7631faa0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -947,7 +947,21 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
>  	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
>  		return false;
>  
> -	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
> +	/* Kernel clipping was a DRI1 misfeature */
> +	if (exec->num_cliprects || exec->cliprects_ptr)
> +		return false;
> +
> +	if (exec->DR4 == 0xffffffff) {
> +		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> +		exec->DR4 = 0;
> +	}
> +	if (exec->DR1 || exec->DR4)
> +		return false;
> +
> +	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
> +		return false;
> +
> +	return true;
>  }
>  
>  static int
> @@ -1111,47 +1125,6 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static int
> -i915_emit_box(struct drm_i915_gem_request *req,
> -	      struct drm_clip_rect *box,
> -	      int DR1, int DR4)
> -{
> -	struct intel_engine_cs *ring = req->ring;
> -	int ret;
> -
> -	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
> -	    box->y2 <= 0 || box->x2 <= 0) {
> -		DRM_ERROR("Bad box %d,%d..%d,%d\n",
> -			  box->x1, box->y1, box->x2, box->y2);
> -		return -EINVAL;
> -	}
> -
> -	if (INTEL_INFO(ring->dev)->gen >= 4) {
> -		ret = intel_ring_begin(req, 4);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -	} else {
> -		ret = intel_ring_begin(req, 6);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
> -		intel_ring_emit(ring, DR1);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -		intel_ring_emit(ring, 0);
> -	}
> -	intel_ring_advance(ring);
> -
> -	return 0;
> -}
> -
>  static struct drm_i915_gem_object*
>  i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>  			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
> @@ -1210,65 +1183,21 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  			       struct drm_i915_gem_execbuffer2 *args,
>  			       struct list_head *vmas)
>  {
> -	struct drm_clip_rect *cliprects = NULL;
>  	struct drm_device *dev = params->dev;
>  	struct intel_engine_cs *ring = params->ring;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u64 exec_start, exec_len;
>  	int instp_mode;
>  	u32 instp_mask;
> -	int i, ret = 0;
> -
> -	if (args->num_cliprects != 0) {
> -		if (ring != &dev_priv->ring[RCS]) {
> -			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
> -			return -EINVAL;
> -		}
> -
> -		if (INTEL_INFO(dev)->gen >= 5) {
> -			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -			return -EINVAL;
> -		}
> -
> -		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
> -			DRM_DEBUG("execbuf with %u cliprects\n",
> -				  args->num_cliprects);
> -			return -EINVAL;
> -		}
> -
> -		cliprects = kcalloc(args->num_cliprects,
> -				    sizeof(*cliprects),
> -				    GFP_KERNEL);
> -		if (cliprects == NULL) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
> -		if (copy_from_user(cliprects,
> -				   to_user_ptr(args->cliprects_ptr),
> -				   sizeof(*cliprects)*args->num_cliprects)) {
> -			ret = -EFAULT;
> -			goto error;
> -		}
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> +	int ret;
>  
>  	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	ret = i915_switch_context(params->request);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
>  	     "%s didn't clear reload\n", ring->name);
> @@ -1281,22 +1210,19 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  	case I915_EXEC_CONSTANTS_REL_SURFACE:
>  		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
>  			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> -			ret = -EINVAL;
> -			goto error;
> +			return -EINVAL;
>  		}
>  
>  		if (instp_mode != dev_priv->relative_constants_mode) {
>  			if (INTEL_INFO(dev)->gen < 4) {
>  				DRM_DEBUG("no rel constants on pre-gen4\n");
> -				ret = -EINVAL;
> -				goto error;
> +				return -EINVAL;
>  			}
>  
>  			if (INTEL_INFO(dev)->gen > 5 &&
>  			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
>  				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
> -				ret = -EINVAL;
> -				goto error;
> +				return -EINVAL;
>  			}
>  
>  			/* The HW changed the meaning on this bit on gen6 */
> @@ -1306,15 +1232,14 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  		break;
>  	default:
>  		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> -		ret = -EINVAL;
> -		goto error;
> +		return -EINVAL;
>  	}
>  
>  	if (ring == &dev_priv->ring[RCS] &&
> -			instp_mode != dev_priv->relative_constants_mode) {
> +	    instp_mode != dev_priv->relative_constants_mode) {
>  		ret = intel_ring_begin(params->request, 4);
>  		if (ret)
> -			goto error;
> +			return ret;
>  
>  		intel_ring_emit(ring, MI_NOOP);
>  		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> @@ -1328,42 +1253,25 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>  		ret = i915_reset_gen7_sol_offsets(dev, params->request);
>  		if (ret)
> -			goto error;
> +			return ret;
>  	}
>  
>  	exec_len   = args->batch_len;
>  	exec_start = params->batch_obj_vm_offset +
>  		     params->args_batch_start_offset;
>  
> -	if (cliprects) {
> -		for (i = 0; i < args->num_cliprects; i++) {
> -			ret = i915_emit_box(params->request, &cliprects[i],
> -					    args->DR1, args->DR4);
> -			if (ret)
> -				goto error;
> -
> -			ret = ring->dispatch_execbuffer(params->request,
> -							exec_start, exec_len,
> -							params->dispatch_flags);
> -			if (ret)
> -				goto error;
> -		}
> -	} else {
> -		ret = ring->dispatch_execbuffer(params->request,
> -						exec_start, exec_len,
> -						params->dispatch_flags);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ring->dispatch_execbuffer(params->request,
> +					exec_start, exec_len,
> +					params->dispatch_flags);
> +	if (ret)
> +		return ret;
>  
>  	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>  
>  	i915_gem_execbuffer_move_to_active(vmas, params->request);
>  	i915_gem_execbuffer_retire_commands(params);
>  
> -error:
> -	kfree(cliprects);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 78aab946db04..d38746c5370d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -878,21 +878,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  		return -EINVAL;
>  	}
>  
> -	if (args->num_cliprects != 0) {
> -		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -		return -EINVAL;
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>  		DRM_DEBUG("sol reset is gen7 only\n");
>  		return -EINVAL;
> -- 
> 2.6.0
> 

-- 
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 2/2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Daniel Vetter]

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.
-Daniel

>  
> -		i915_gem_object_finish_gtt(obj);
> -
>  		/* 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.
> @@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				return ret;
>  		}
>  
> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (drm_mm_node_allocated(&vma->node)) {
> -				ret = i915_vma_bind(vma, cache_level,
> -						    PIN_UPDATE);
> -				if (ret)
> -					return ret;
> -			}
> +		/* Access to snoopable pages through the GTT is incoherent. */
> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> +			i915_gem_release_mmap(obj);
> +
> +		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +			if (!drm_mm_node_allocated(&vma->node))
> +				continue;
> +
> +			ret = i915_vma_bind(vma, cache_level, PIN_UPDATE);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -- 
> 2.6.0
> 
> _______________________________________________
> 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 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: Move the mb() following release-mmap into release-mmap

[Chris Wilson]

As paranoia, we want to ensure that the CPU's PTEs have been revoked for
the object before we return from i915_gem_release_mmap(). This allows us
to rely on there being no outstanding memory accesses and guarantees
serialisation of the code against concurrent access just by calling
i915_gem_release_mmap().

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2b8ed7a2faab..642644f12295 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1877,11 +1877,21 @@ out:
 void
 i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 {
+	/* Serialisation between user GTT access and our code depends upon
+	 * revoking the CPU's PTE whilst the mutex is held. The next user
+	 * pagefault then has to wait until we release the mutex.
+	 */
+	lockdep_assert_held(&obj->base.dev->struct_mutex);
+
 	if (!obj->fault_mappable)
 		return;
 
 	drm_vma_node_unmap(&obj->base.vma_node,
 			   obj->base.dev->anon_inode->i_mapping);
+
+	/* Ensure that the CPU's PTE are revoked before we return */
+	mb();
+
 	obj->fault_mappable = false;
 }
 
@@ -3168,9 +3178,6 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
 	if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)
 		return;
 
-	/* Wait for any direct GTT access to complete */
-	mb();
-
 	old_read_domains = obj->base.read_domains;
 	old_write_domain = obj->base.write_domain;
 
-- 
2.6.1

_______________________________________________
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 1/2] drm/i915: Kill DRI1 cliprects

[Chris Wilson]

On Tue, Oct 06, 2015 at 01:21:25PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 11:39:55AM +0100, Chris Wilson wrote:
> > Passing cliprects into the kernel for it to re-execute the batch buffer
> > with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> > removed from the kernel, we can now simply reject any execbuf trying to
> > use this "feature".
> > 
> > To keep Daniel happy with the prospect of being able to reuse these
> > fields in the next decade, continue to ensure that current userspace is
> > not passing garbage in through the dead fields.
> > 
> > v2: Fix the cliprects_ptr check
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> igt subtest seems to be missing to ensure we enforce this. Yay otherwise!

Looking at gem_exec_params/cliprects-invalid, presumably all you want to
do is to remove the igt_require(). To be more precise you would need to
a flag to tell when the wicked witch is dead and you could expect the
execbuf to fail with any cliprect garbage.
-Chris
> -Da 

-- 
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

[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

Re: [PATCH 1/2] drm/i915: Kill DRI1 cliprects

[Tvrtko Ursulin]

On 06/10/15 11:39, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
>
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
>
> v2: Fix the cliprects_ptr check
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Don't know anything about the DRI1 history but the removal looks fine to 
me, so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Would it also make sense to rename the related fields in 
drm_i915_gem_execbuffer2 to reserved?

Regards,

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

Re: [PATCH 1/2] drm/i915: Kill DRI1 cliprects

[Dave Gordon]

On 06/10/15 11:39, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
>
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
>
> v2: Fix the cliprects_ptr check
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
>   drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
>   2 files changed, 31 insertions(+), 138 deletions(-)

This will cause John Harrison & myself a certain amount of pain (because 
the changes collide with the scheduler's reorganisation of the 
execbuffer path), but I'm all in favour of getting rid of the legacy 
crud cluttering up this code, so ...

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Perhaps we could get rid of relative-constants-mode next :)

> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 75a0c8b5305b..045a7631faa0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -947,7 +947,21 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
>   	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
>   		return false;
>
> -	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
> +	/* Kernel clipping was a DRI1 misfeature */
> +	if (exec->num_cliprects || exec->cliprects_ptr)
> +		return false;
> +
> +	if (exec->DR4 == 0xffffffff) {
> +		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> +		exec->DR4 = 0;
> +	}
> +	if (exec->DR1 || exec->DR4)
> +		return false;
> +
> +	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
> +		return false;
> +
> +	return true;
>   }
>
>   static int
> @@ -1111,47 +1125,6 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>   	return 0;
>   }
>
> -static int
> -i915_emit_box(struct drm_i915_gem_request *req,
> -	      struct drm_clip_rect *box,
> -	      int DR1, int DR4)
> -{
> -	struct intel_engine_cs *ring = req->ring;
> -	int ret;
> -
> -	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
> -	    box->y2 <= 0 || box->x2 <= 0) {
> -		DRM_ERROR("Bad box %d,%d..%d,%d\n",
> -			  box->x1, box->y1, box->x2, box->y2);
> -		return -EINVAL;
> -	}
> -
> -	if (INTEL_INFO(ring->dev)->gen >= 4) {
> -		ret = intel_ring_begin(req, 4);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -	} else {
> -		ret = intel_ring_begin(req, 6);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
> -		intel_ring_emit(ring, DR1);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -		intel_ring_emit(ring, 0);
> -	}
> -	intel_ring_advance(ring);
> -
> -	return 0;
> -}
> -
>   static struct drm_i915_gem_object*
>   i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>   			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
> @@ -1210,65 +1183,21 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   			       struct drm_i915_gem_execbuffer2 *args,
>   			       struct list_head *vmas)
>   {
> -	struct drm_clip_rect *cliprects = NULL;
>   	struct drm_device *dev = params->dev;
>   	struct intel_engine_cs *ring = params->ring;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	u64 exec_start, exec_len;
>   	int instp_mode;
>   	u32 instp_mask;
> -	int i, ret = 0;
> -
> -	if (args->num_cliprects != 0) {
> -		if (ring != &dev_priv->ring[RCS]) {
> -			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
> -			return -EINVAL;
> -		}
> -
> -		if (INTEL_INFO(dev)->gen >= 5) {
> -			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -			return -EINVAL;
> -		}
> -
> -		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
> -			DRM_DEBUG("execbuf with %u cliprects\n",
> -				  args->num_cliprects);
> -			return -EINVAL;
> -		}
> -
> -		cliprects = kcalloc(args->num_cliprects,
> -				    sizeof(*cliprects),
> -				    GFP_KERNEL);
> -		if (cliprects == NULL) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
> -		if (copy_from_user(cliprects,
> -				   to_user_ptr(args->cliprects_ptr),
> -				   sizeof(*cliprects)*args->num_cliprects)) {
> -			ret = -EFAULT;
> -			goto error;
> -		}
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> +	int ret;
>
>   	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
>   	if (ret)
> -		goto error;
> +		return ret;
>
>   	ret = i915_switch_context(params->request);
>   	if (ret)
> -		goto error;
> +		return ret;
>
>   	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
>   	     "%s didn't clear reload\n", ring->name);
> @@ -1281,22 +1210,19 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   	case I915_EXEC_CONSTANTS_REL_SURFACE:
>   		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
>   			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> -			ret = -EINVAL;
> -			goto error;
> +			return -EINVAL;
>   		}
>
>   		if (instp_mode != dev_priv->relative_constants_mode) {
>   			if (INTEL_INFO(dev)->gen < 4) {
>   				DRM_DEBUG("no rel constants on pre-gen4\n");
> -				ret = -EINVAL;
> -				goto error;
> +				return -EINVAL;
>   			}
>
>   			if (INTEL_INFO(dev)->gen > 5 &&
>   			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
>   				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
> -				ret = -EINVAL;
> -				goto error;
> +				return -EINVAL;
>   			}
>
>   			/* The HW changed the meaning on this bit on gen6 */
> @@ -1306,15 +1232,14 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   		break;
>   	default:
>   		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> -		ret = -EINVAL;
> -		goto error;
> +		return -EINVAL;
>   	}
>
>   	if (ring == &dev_priv->ring[RCS] &&
> -			instp_mode != dev_priv->relative_constants_mode) {
> +	    instp_mode != dev_priv->relative_constants_mode) {
>   		ret = intel_ring_begin(params->request, 4);
>   		if (ret)
> -			goto error;
> +			return ret;
>
>   		intel_ring_emit(ring, MI_NOOP);
>   		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> @@ -1328,42 +1253,25 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>   		ret = i915_reset_gen7_sol_offsets(dev, params->request);
>   		if (ret)
> -			goto error;
> +			return ret;
>   	}
>
>   	exec_len   = args->batch_len;
>   	exec_start = params->batch_obj_vm_offset +
>   		     params->args_batch_start_offset;
>
> -	if (cliprects) {
> -		for (i = 0; i < args->num_cliprects; i++) {
> -			ret = i915_emit_box(params->request, &cliprects[i],
> -					    args->DR1, args->DR4);
> -			if (ret)
> -				goto error;
> -
> -			ret = ring->dispatch_execbuffer(params->request,
> -							exec_start, exec_len,
> -							params->dispatch_flags);
> -			if (ret)
> -				goto error;
> -		}
> -	} else {
> -		ret = ring->dispatch_execbuffer(params->request,
> -						exec_start, exec_len,
> -						params->dispatch_flags);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ring->dispatch_execbuffer(params->request,
> +					exec_start, exec_len,
> +					params->dispatch_flags);
> +	if (ret)
> +		return ret;
>
>   	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>
>   	i915_gem_execbuffer_move_to_active(vmas, params->request);
>   	i915_gem_execbuffer_retire_commands(params);
>
> -error:
> -	kfree(cliprects);
> -	return ret;
> +	return 0;
>   }
>
>   /**
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 78aab946db04..d38746c5370d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -878,21 +878,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>   		return -EINVAL;
>   	}
>
> -	if (args->num_cliprects != 0) {
> -		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -		return -EINVAL;
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>   	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>   		DRM_DEBUG("sol reset is gen7 only\n");
>   		return -EINVAL;
>

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

Re: [PATCH] drm/i915: Move the mb() following release-mmap into release-mmap

[Tvrtko Ursulin]

Hi,

On 06/10/15 12:58, Chris Wilson wrote:
> As paranoia, we want to ensure that the CPU's PTEs have been revoked for
> the object before we return from i915_gem_release_mmap(). This allows us
> to rely on there being no outstanding memory accesses and guarantees
> serialisation of the code against concurrent access just by calling
> i915_gem_release_mmap().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2b8ed7a2faab..642644f12295 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1877,11 +1877,21 @@ out:
>   void
>   i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>   {
> +	/* Serialisation between user GTT access and our code depends upon
> +	 * revoking the CPU's PTE whilst the mutex is held. The next user
> +	 * pagefault then has to wait until we release the mutex.
> +	 */
> +	lockdep_assert_held(&obj->base.dev->struct_mutex);
> +
>   	if (!obj->fault_mappable)
>   		return;
>
>   	drm_vma_node_unmap(&obj->base.vma_node,
>   			   obj->base.dev->anon_inode->i_mapping);
> +
> +	/* Ensure that the CPU's PTE are revoked before we return */
> +	mb();
> +

smp_mb() or smp_wmb() would not suffice? Is it needed on uniprocessor?

Regards,

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

Re: [PATCH 1/2] drm/i915: Kill DRI1 cliprects

[Chris Wilson]

On Tue, Oct 06, 2015 at 03:29:20PM +0100, Dave Gordon wrote:
> On 06/10/15 11:39, Chris Wilson wrote:
> >Passing cliprects into the kernel for it to re-execute the batch buffer
> >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> >removed from the kernel, we can now simply reject any execbuf trying to
> >use this "feature".
> >
> >To keep Daniel happy with the prospect of being able to reuse these
> >fields in the next decade, continue to ensure that current userspace is
> >not passing garbage in through the dead fields.
> >
> >v2: Fix the cliprects_ptr check
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
> >  drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
> >  2 files changed, 31 insertions(+), 138 deletions(-)
> 
> This will cause John Harrison & myself a certain amount of pain
> (because the changes collide with the scheduler's reorganisation of
> the execbuffer path), but I'm all in favour of getting rid of the
> legacy crud cluttering up this code, so ...

Oh, don't worry I'm completely rewriting it and will nak anything that
adds a cycle of latency to execbuffer.
-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 1/2] drm/i915: Kill DRI1 cliprects

[Chris Wilson]

On Tue, Oct 06, 2015 at 03:19:36PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/10/15 11:39, Chris Wilson wrote:
> >Passing cliprects into the kernel for it to re-execute the batch buffer
> >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> >removed from the kernel, we can now simply reject any execbuf trying to
> >use this "feature".
> >
> >To keep Daniel happy with the prospect of being able to reuse these
> >fields in the next decade, continue to ensure that current userspace is
> >not passing garbage in through the dead fields.
> >
> >v2: Fix the cliprects_ptr check
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Don't know anything about the DRI1 history but the removal looks
> fine to me, so:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Would it also make sense to rename the related fields in
> drm_i915_gem_execbuffer2 to reserved?

In our heads, yes. Renaming structs in uapi.h is harder than it looks.
-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 1/2] drm/i915: Kill DRI1 cliprects

[Daniel Vetter]

On Tue, Oct 06, 2015 at 04:37:13PM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 03:19:36PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 06/10/15 11:39, Chris Wilson wrote:
> > >Passing cliprects into the kernel for it to re-execute the batch buffer
> > >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> > >removed from the kernel, we can now simply reject any execbuf trying to
> > >use this "feature".
> > >
> > >To keep Daniel happy with the prospect of being able to reuse these
> > >fields in the next decade, continue to ensure that current userspace is
> > >not passing garbage in through the dead fields.
> > >
> > >v2: Fix the cliprects_ptr check
> > >
> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Don't know anything about the DRI1 history but the removal looks
> > fine to me, so:
> > 
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Queued for -next, thanks for the patch. Assuming the igt materializes ;-)
-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

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

[Tvrtko Ursulin]

Hi,

On 06/10/15 11:39, 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()

i915_gem_obj_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.

"do sanity checks" or something.

> At the same time, we can micro-optimise GTT mmap() behaviour since we
> only need to relinquish the mmaps before Sandybridge.
>
> 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;
> +		}
> +

But this is the same as i915_gem_obj_is_pinned, where is the 
obj->pin_display change commit message talks about?

>   		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;
>
> -		i915_gem_object_finish_gtt(obj);
> -
>   		/* 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.
> @@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>   				return ret;
>   		}
>
> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (drm_mm_node_allocated(&vma->node)) {
> -				ret = i915_vma_bind(vma, cache_level,
> -						    PIN_UPDATE);
> -				if (ret)
> -					return ret;
> -			}
> +		/* Access to snoopable pages through the GTT is incoherent. */
> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> +			i915_gem_release_mmap(obj);

Don't fully understand this one - but my question is this. Previously 
userspace would lose mappings on cache level changes any time, after 
this only on !LLC when turning on caching mode. So this means userspace 
needs to know about this change and modify it's behavior? Or what 
exactly would happen in practice?

Regards,

Tvrtko
_______________________________________________
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 Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 06/10/15 11:39, 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()
> 
> i915_gem_obj_is_pinned

What? That's not the normal prefix, who named that monstrosity!

> 
> >here with a plain obj->pin_display check. During rebinding, we will check
> >sanity checks in case vma->pin_count is erroneously set.
> 
> "do sanity checks" or something.
> 
> >At the same time, we can micro-optimise GTT mmap() behaviour since we
> >only need to relinquish the mmaps before Sandybridge.
> >
> >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;
> >+		}
> >+
> 
> But this is the same as i915_gem_obj_is_pinned, where is the
> obj->pin_display change commit message talks about?

Right here. The difference is that we are only iterating the vma list
once rather than 3x.

> >  		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;
> >
> >-		i915_gem_object_finish_gtt(obj);
> >-
> >  		/* 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.
> >@@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  				return ret;
> >  		}
> >
> >-		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >-			if (drm_mm_node_allocated(&vma->node)) {
> >-				ret = i915_vma_bind(vma, cache_level,
> >-						    PIN_UPDATE);
> >-				if (ret)
> >-					return ret;
> >-			}
> >+		/* Access to snoopable pages through the GTT is incoherent. */
> >+		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> >+			i915_gem_release_mmap(obj);
> 
> Don't fully understand this one - but my question is this.
> Previously userspace would lose mappings on cache level changes any
> time, after this only on !LLC when turning on caching mode. So this
> means userspace needs to know about this change and modify it's
> behavior? Or what exactly would happen in practice?

No. Userspace has no knowledge of the kernel handling the PTEs, its
mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
Otoh, we are improving the situation so even if userspace tries to avoid
set-cache-level nothing is lost.
-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

[Tvrtko Ursulin]

On 07/10/15 17:19, Chris Wilson wrote:
> On Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 06/10/15 11:39, 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()

[snip]

>>> 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;
>>> +		}
>>> +
>>
>> But this is the same as i915_gem_obj_is_pinned, where is the
>> obj->pin_display change commit message talks about?
>
> Right here. The difference is that we are only iterating the vma list
> once rather than 3x.

Thats true, but the commit says it is going to use obj->pin_display for 
something and then doesn't use it at all. Riddles in patches are not 
that hot. :)

>>>   		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;
>>>
>>> -		i915_gem_object_finish_gtt(obj);
>>> -
>>>   		/* 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.
>>> @@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>>>   				return ret;
>>>   		}
>>>
>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
>>> -			if (drm_mm_node_allocated(&vma->node)) {
>>> -				ret = i915_vma_bind(vma, cache_level,
>>> -						    PIN_UPDATE);
>>> -				if (ret)
>>> -					return ret;
>>> -			}
>>> +		/* Access to snoopable pages through the GTT is incoherent. */
>>> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
>>> +			i915_gem_release_mmap(obj);
>>
>> Don't fully understand this one - but my question is this.
>> Previously userspace would lose mappings on cache level changes any
>> time, after this only on !LLC when turning on caching mode. So this
>> means userspace needs to know about this change and modify it's
>> behavior? Or what exactly would happen in practice?
>
> No. Userspace has no knowledge of the kernel handling the PTEs, its
> mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
> Otoh, we are improving the situation so even if userspace tries to avoid
> set-cache-level nothing is lost.

Hm so if a VMA is re-bound in this process and it could have gotten a 
new GGTT address, why it is not necessary to always release mmaps and so 
to update CPU PTEs?

Also what about Sandy Bridge? Commit message mentions it and the code 
doesn't?

Regards,

Tvrtko
_______________________________________________
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 Thu, Oct 08, 2015 at 10:32:39AM +0100, Tvrtko Ursulin wrote:
> 
> On 07/10/15 17:19, Chris Wilson wrote:
> >On Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
> >>
> >>Hi,
> >>
> >>On 06/10/15 11:39, 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()
> 
> [snip]
> 
> >>>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;
> >>>+		}
> >>>+
> >>
> >>But this is the same as i915_gem_obj_is_pinned, where is the
> >>obj->pin_display change commit message talks about?
> >
> >Right here. The difference is that we are only iterating the vma list
> >once rather than 3x.
> 
> Thats true, but the commit says it is going to use obj->pin_display
> for something and then doesn't use it at all. Riddles in patches are
> not that hot. :)

I was trying to explain what the actual rules pertaining to the
rebinding the vma was. We can rebind anything that isn't pinned and the
only thing pinned here can be obj->pin_display.
 
> >>>  		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;
> >>>
> >>>-		i915_gem_object_finish_gtt(obj);
> >>>-
> >>>  		/* 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.
> >>>@@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >>>  				return ret;
> >>>  		}
> >>>
> >>>-		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >>>-			if (drm_mm_node_allocated(&vma->node)) {
> >>>-				ret = i915_vma_bind(vma, cache_level,
> >>>-						    PIN_UPDATE);
> >>>-				if (ret)
> >>>-					return ret;
> >>>-			}
> >>>+		/* Access to snoopable pages through the GTT is incoherent. */
> >>>+		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> >>>+			i915_gem_release_mmap(obj);
> >>
> >>Don't fully understand this one - but my question is this.
> >>Previously userspace would lose mappings on cache level changes any
> >>time, after this only on !LLC when turning on caching mode. So this
> >>means userspace needs to know about this change and modify it's
> >>behavior? Or what exactly would happen in practice?
> >
> >No. Userspace has no knowledge of the kernel handling the PTEs, its
> >mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
> >Otoh, we are improving the situation so even if userspace tries to avoid
> >set-cache-level nothing is lost.
> 
> Hm so if a VMA is re-bound in this process and it could have gotten
> a new GGTT address, why it is not necessary to always release mmaps
> and so to update CPU PTEs?

The VMA are not moved by this function, only the PTEs are rewritten. The
GTT ignores the bits we are changing on llc architectures. On !llc using
the GTT to access snoopable PTE is verboten and does cause machine hangs.
 
> Also what about Sandy Bridge? Commit message mentions it and the
> code doesn't?

Age of patch and lack of !llc snb+ at the time, and the state of my mind
when I think about llc.
-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

[Tvrtko Ursulin]

On 08/10/15 10:46, Chris Wilson wrote:
> On Thu, Oct 08, 2015 at 10:32:39AM +0100, Tvrtko Ursulin wrote:
>>
>> On 07/10/15 17:19, Chris Wilson wrote:
>>> On Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 06/10/15 11:39, 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()
>>
>> [snip]
>>
>>>>> 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;
>>>>> +		}
>>>>> +
>>>>
>>>> But this is the same as i915_gem_obj_is_pinned, where is the
>>>> obj->pin_display change commit message talks about?
>>>
>>> Right here. The difference is that we are only iterating the vma list
>>> once rather than 3x.
>>
>> Thats true, but the commit says it is going to use obj->pin_display
>> for something and then doesn't use it at all. Riddles in patches are
>> not that hot. :)
>
> I was trying to explain what the actual rules pertaining to the
> rebinding the vma was. We can rebind anything that isn't pinned and the
> only thing pinned here can be obj->pin_display.

Okay but the commit message says the is going to use obj->pin_display.

>>>>>   		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;
>>>>>
>>>>> -		i915_gem_object_finish_gtt(obj);
>>>>> -
>>>>>   		/* 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.
>>>>> @@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>>>>>   				return ret;
>>>>>   		}
>>>>>
>>>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>>> -			if (drm_mm_node_allocated(&vma->node)) {
>>>>> -				ret = i915_vma_bind(vma, cache_level,
>>>>> -						    PIN_UPDATE);
>>>>> -				if (ret)
>>>>> -					return ret;
>>>>> -			}
>>>>> +		/* Access to snoopable pages through the GTT is incoherent. */
>>>>> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
>>>>> +			i915_gem_release_mmap(obj);
>>>>
>>>> Don't fully understand this one - but my question is this.
>>>> Previously userspace would lose mappings on cache level changes any
>>>> time, after this only on !LLC when turning on caching mode. So this
>>>> means userspace needs to know about this change and modify it's
>>>> behavior? Or what exactly would happen in practice?
>>>
>>> No. Userspace has no knowledge of the kernel handling the PTEs, its
>>> mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
>>> Otoh, we are improving the situation so even if userspace tries to avoid
>>> set-cache-level nothing is lost.
>>
>> Hm so if a VMA is re-bound in this process and it could have gotten
>> a new GGTT address, why it is not necessary to always release mmaps
>> and so to update CPU PTEs?
>
> The VMA are not moved by this function, only the PTEs are rewritten. The
> GTT ignores the bits we are changing on llc architectures. On !llc using
> the GTT to access snoopable PTE is verboten and does cause machine hangs.

How come they are not moved when they can be unbound and then bound again?

Regards,

Tvrtko
_______________________________________________
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 Fri, Oct 09, 2015 at 11:17:19AM +0100, Tvrtko Ursulin wrote:
> >>>>>-		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >>>>>-			if (drm_mm_node_allocated(&vma->node)) {
> >>>>>-				ret = i915_vma_bind(vma, cache_level,
> >>>>>-						    PIN_UPDATE);
> >>>>>-				if (ret)
> >>>>>-					return ret;
> >>>>>-			}
> >>>>>+		/* Access to snoopable pages through the GTT is incoherent. */
> >>>>>+		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> >>>>>+			i915_gem_release_mmap(obj);
> >>>>
> >>>>Don't fully understand this one - but my question is this.
> >>>>Previously userspace would lose mappings on cache level changes any
> >>>>time, after this only on !LLC when turning on caching mode. So this
> >>>>means userspace needs to know about this change and modify it's
> >>>>behavior? Or what exactly would happen in practice?
> >>>
> >>>No. Userspace has no knowledge of the kernel handling the PTEs, its
> >>>mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
> >>>Otoh, we are improving the situation so even if userspace tries to avoid
> >>>set-cache-level nothing is lost.
> >>
> >>Hm so if a VMA is re-bound in this process and it could have gotten
> >>a new GGTT address, why it is not necessary to always release mmaps
> >>and so to update CPU PTEs?
> >
> >The VMA are not moved by this function, only the PTEs are rewritten. The
> >GTT ignores the bits we are changing on llc architectures. On !llc using
> >the GTT to access snoopable PTE is verboten and does cause machine hangs.
> 
> How come they are not moved when they can be unbound and then bound again?

The only relevant vma here are rebound with PIN_UPDATE. If we have to
unbind any due to subsequent placement errors, the behaviour doesn't
change in this patch. So I'm not understanding your concern and can't
address it adequately. :(
-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

[Tvrtko Ursulin]

On 09/10/15 11:34, Chris Wilson wrote:
> On Fri, Oct 09, 2015 at 11:17:19AM +0100, Tvrtko Ursulin wrote:
>>>>>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>>>>> -			if (drm_mm_node_allocated(&vma->node)) {
>>>>>>> -				ret = i915_vma_bind(vma, cache_level,
>>>>>>> -						    PIN_UPDATE);
>>>>>>> -				if (ret)
>>>>>>> -					return ret;
>>>>>>> -			}
>>>>>>> +		/* Access to snoopable pages through the GTT is incoherent. */
>>>>>>> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
>>>>>>> +			i915_gem_release_mmap(obj);
>>>>>>
>>>>>> Don't fully understand this one - but my question is this.
>>>>>> Previously userspace would lose mappings on cache level changes any
>>>>>> time, after this only on !LLC when turning on caching mode. So this
>>>>>> means userspace needs to know about this change and modify it's
>>>>>> behavior? Or what exactly would happen in practice?
>>>>>
>>>>> No. Userspace has no knowledge of the kernel handling the PTEs, its
>>>>> mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
>>>>> Otoh, we are improving the situation so even if userspace tries to avoid
>>>>> set-cache-level nothing is lost.
>>>>
>>>> Hm so if a VMA is re-bound in this process and it could have gotten
>>>> a new GGTT address, why it is not necessary to always release mmaps
>>>> and so to update CPU PTEs?
>>>
>>> The VMA are not moved by this function, only the PTEs are rewritten. The
>>> GTT ignores the bits we are changing on llc architectures. On !llc using
>>> the GTT to access snoopable PTE is verboten and does cause machine hangs.
>>
>> How come they are not moved when they can be unbound and then bound again?
>
> The only relevant vma here are rebound with PIN_UPDATE. If we have to
> unbind any due to subsequent placement errors, the behaviour doesn't
> change in this patch. So I'm not understanding your concern and can't
> address it adequately. :(

I started to understand how this works after a chat on IRC. Before I had 
a completely wrong assumptions.

(This also demonstrates this code should really have a good high level 
comment.)

Unless I missed something it really looks the behaviour is unchanged, 
just a trip to the fault handler is avoided if not needed.

But I still think you need to improve the commit message to be clearer 
on pin_display (un)usage and SandyBridge referencing.

Regards,

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

Re: [PATCH] drm/i915: Move the mb() following release-mmap into release-mmap

[Chris Wilson]

On Tue, Oct 06, 2015 at 03:40:02PM +0100, Tvrtko Ursulin wrote:
> 
> 
> Hi,
> 
> On 06/10/15 12:58, Chris Wilson wrote:
> >As paranoia, we want to ensure that the CPU's PTEs have been revoked for
> >the object before we return from i915_gem_release_mmap(). This allows us
> >to rely on there being no outstanding memory accesses and guarantees
> >serialisation of the code against concurrent access just by calling
> >i915_gem_release_mmap().
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 2b8ed7a2faab..642644f12295 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -1877,11 +1877,21 @@ out:
> >  void
> >  i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> >  {
> >+	/* Serialisation between user GTT access and our code depends upon
> >+	 * revoking the CPU's PTE whilst the mutex is held. The next user
> >+	 * pagefault then has to wait until we release the mutex.
> >+	 */
> >+	lockdep_assert_held(&obj->base.dev->struct_mutex);
> >+
> >  	if (!obj->fault_mappable)
> >  		return;
> >
> >  	drm_vma_node_unmap(&obj->base.vma_node,
> >  			   obj->base.dev->anon_inode->i_mapping);
> >+
> >+	/* Ensure that the CPU's PTE are revoked before we return */
> >+	mb();
> >+
> 
> smp_mb() or smp_wmb() would not suffice? Is it needed on uniprocessor?

Correct, smp_mb() would not suffice as we are serialised accessing
through a mmio channel with the PTE writes.

A wmb() may suffice though, but that actually changed code :)
-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