[PATCH] drm/i915: Pre-validate the NEED_GTTS flag for execbuffer

[PATCH] drm/i915: Pre-validate the NEED_GTTS flag for execbuffer

[Chris Wilson]

We have an implementation requirement that precludes the user from
requesting a ggtt entry when the device is operating in ppgtt mode. Move
the current check from inside the execbuffer object collation to the
prevalidation phase.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5c5fa48115fb..e15759f0679a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -131,12 +131,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
 		struct i915_vma *vma;
 		struct i915_address_space *bind_vm = vm;
 
-		if (exec[i].flags & EXEC_OBJECT_NEEDS_GTT &&
-		    USES_FULL_PPGTT(vm->dev)) {
-			ret = -EINVAL;
-			goto err;
-		}
-
 		/* If we have secure dispatch, or the userspace assures us that
 		 * they know what they're doing, use the GGTT VM.
 		 */
@@ -889,12 +883,14 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
 }
 
 static int
-validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
+validate_exec_list(struct drm_device *dev,
+		   struct drm_i915_gem_exec_object2 *exec,
 		   int count)
 {
-	int i;
+	const unsigned uses_full_ppgtt = USES_FULL_PPGTT(dev) ? EXEC_OBJECT_NEEDS_GTT : 0;
 	unsigned relocs_total = 0;
 	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
+	int i;
 
 	for (i = 0; i < count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
@@ -903,6 +899,9 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 		if (exec[i].flags & __EXEC_OBJECT_UNKNOWN_FLAGS)
 			return -EINVAL;
 
+		if (exec[i].flags & uses_full_ppgtt)
+			return -EINVAL;
+
 		/* First check for malicious input causing overflow in
 		 * the worst case where we need to allocate the entire
 		 * relocation tree as a single array.
@@ -1243,7 +1242,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (!i915_gem_check_execbuffer(args))
 		return -EINVAL;
 
-	ret = validate_exec_list(exec, args->buffer_count);
+	ret = validate_exec_list(dev, exec, args->buffer_count);
 	if (ret)
 		return ret;
 
-- 
2.1.0.rc1

[PATCH 1/4] drm/i915: Pre-validate the NEED_GTTS flag for execbuffer

[Chris Wilson]

We have an implementation requirement that precludes the user from
requesting a ggtt entry when the device is operating in ppgtt mode. Move
the current check from inside the execbuffer object collation to the
prevalidation phase.

v2: Roll both invalid flags checks into one

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index fbac7b51736c..4304d8b9e17c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -132,12 +132,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
 		struct i915_vma *vma;
 		struct i915_address_space *bind_vm = vm;
 
-		if (exec[i].flags & EXEC_OBJECT_NEEDS_GTT &&
-		    USES_FULL_PPGTT(vm->dev)) {
-			ret = -EINVAL;
-			goto err;
-		}
-
 		/* If we have secure dispatch, or the userspace assures us that
 		 * they know what they're doing, use the GGTT VM.
 		 */
@@ -886,18 +880,24 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
 }
 
 static int
-validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
+validate_exec_list(struct drm_device *dev,
+		   struct drm_i915_gem_exec_object2 *exec,
 		   int count)
 {
-	int i;
 	unsigned relocs_total = 0;
 	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
+	unsigned invalid_flags;
+	int i;
+
+	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
+	if (USES_FULL_PPGTT(dev))
+		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
 
 	for (i = 0; i < count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
 		int length; /* limited by fault_in_pages_readable() */
 
-		if (exec[i].flags & __EXEC_OBJECT_UNKNOWN_FLAGS)
+		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
 		/* First check for malicious input causing overflow in
@@ -1259,7 +1259,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (!i915_gem_check_execbuffer(args))
 		return -EINVAL;
 
-	ret = validate_exec_list(exec, args->buffer_count);
+	ret = validate_exec_list(dev, exec, args->buffer_count);
 	if (ret)
 		return ret;
 
-- 
2.1.0.rc1

[PATCH 2/4] drm/i915: Mark the execbuffer validation failures as unlikely

[Chris Wilson]

This just allows the compiler to pessimise callers who try to abuse the
ioctl in the hope of making the correct users faster.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 +++++++++++++-----------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4304d8b9e17c..0ba1e7bbd09d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -870,41 +870,38 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
 	return intel_ring_invalidate_all_caches(ring);
 }
 
-static bool
-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;
-}
-
 static int
 validate_exec_list(struct drm_device *dev,
-		   struct drm_i915_gem_exec_object2 *exec,
-		   int count)
+		   const struct drm_i915_gem_execbuffer2 *args,
+		   const struct drm_i915_gem_exec_object2 *exec)
 {
 	unsigned relocs_total = 0;
 	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
 	unsigned invalid_flags;
 	int i;
 
+	if (unlikely(args->flags & __I915_EXEC_UNKNOWN_FLAGS))
+		return -EINVAL;
+
+	if (unlikely((args->batch_start_offset | args->batch_len) & 0x7))
+		return -EINVAL;
+
 	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
 	if (USES_FULL_PPGTT(dev))
 		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < args->buffer_count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
 		int length; /* limited by fault_in_pages_readable() */
 
-		if (exec[i].flags & invalid_flags)
+		if (unlikely(exec[i].flags & invalid_flags))
 			return -EINVAL;
 
 		/* First check for malicious input causing overflow in
 		 * the worst case where we need to allocate the entire
 		 * relocation tree as a single array.
 		 */
-		if (exec[i].relocation_count > relocs_max - relocs_total)
+		if (unlikely(exec[i].relocation_count > relocs_max - relocs_total))
 			return -EINVAL;
 		relocs_total += exec[i].relocation_count;
 
@@ -915,11 +912,11 @@ validate_exec_list(struct drm_device *dev,
 		 * to read, but since we may need to update the presumed
 		 * offsets during execution, check for full write access.
 		 */
-		if (!access_ok(VERIFY_WRITE, ptr, length))
+		if (unlikely(!access_ok(VERIFY_WRITE, ptr, length)))
 			return -EFAULT;
 
 		if (likely(!i915.prefault_disable)) {
-			if (fault_in_multipages_readable(ptr, length))
+			if (unlikely(fault_in_multipages_readable(ptr, length)))
 				return -EFAULT;
 		}
 	}
@@ -1256,11 +1253,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	int ret;
 	bool need_relocs;
 
-	if (!i915_gem_check_execbuffer(args))
-		return -EINVAL;
-
-	ret = validate_exec_list(dev, exec, args->buffer_count);
-	if (ret)
+	ret = validate_exec_list(dev, args, exec);
+	if (unlikely(ret))
 		return ret;
 
 	flags = 0;
-- 
2.1.0.rc1

[PATCH 3/4] drm/i915: Remove redundant list_empty(eb->vmas) tests in execbuffer

[Chris Wilson]

Part of the pre-validation for an execbuffer call is that there is at
least one object in the execlist. As we bail if we fail to lookup any
object, we can be sure that after the eb_lookup_vma() there is at least
one object in the vma list and so we do not need to assert.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ba1e7bbd09d..e6fcd0594bd5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -631,9 +631,6 @@ i915_gem_execbuffer_reserve(struct intel_engine_cs *ring,
 	bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4;
 	int retry;
 
-	if (list_empty(vmas))
-		return 0;
-
 	i915_gem_retire_requests_ring(ring);
 
 	vm = list_first_entry(vmas, struct i915_vma, exec_list)->vm;
@@ -734,9 +731,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	int i, total, ret;
 	unsigned count = args->buffer_count;
 
-	if (WARN_ON(list_empty(&eb->vmas)))
-		return 0;
-
 	vm = list_first_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
 
 	/* We may process another execbuffer during the unlock... */
-- 
2.1.0.rc1

[PATCH 4/4] drm/i915: Simplify relocate_entry_gtt() and make 64-bit safe

[Chris Wilson]

Even though we should not try to use 4+GiB GTTs on 32-bit systems, by
using a local variable we can future proof the code whilst making it
easier to read.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index e6fcd0594bd5..5c04f77f8a66 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -302,7 +302,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint64_t delta = reloc->delta + target_offset;
-	uint32_t __iomem *reloc_entry;
+	uint64_t offset;
 	void __iomem *reloc_page;
 	int ret;
 
@@ -315,25 +315,22 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
 		return ret;
 
 	/* Map the page containing the relocation we're going to perform.  */
-	reloc->offset += i915_gem_obj_ggtt_offset(obj);
+	offset = i915_gem_obj_ggtt_offset(obj);
+	offset += reloc->offset;
 	reloc_page = io_mapping_map_atomic_wc(dev_priv->gtt.mappable,
-			reloc->offset & PAGE_MASK);
-	reloc_entry = (uint32_t __iomem *)
-		(reloc_page + offset_in_page(reloc->offset));
-	iowrite32(lower_32_bits(delta), reloc_entry);
+					      offset & PAGE_MASK);
+	iowrite32(lower_32_bits(delta), reloc_page + offset_in_page(offset));
 
 	if (INTEL_INFO(dev)->gen >= 8) {
-		reloc_entry += 1;
+		offset += sizeof(uint32_t);
 
-		if (offset_in_page(reloc->offset + sizeof(uint32_t)) == 0) {
+		if (offset_in_page(offset) == 0) {
 			io_mapping_unmap_atomic(reloc_page);
-			reloc_page = io_mapping_map_atomic_wc(
-					dev_priv->gtt.mappable,
-					reloc->offset + sizeof(uint32_t));
-			reloc_entry = reloc_page;
+			reloc_page = io_mapping_map_atomic_wc(dev_priv->gtt.mappable,
+							      offset);
 		}
 
-		iowrite32(upper_32_bits(delta), reloc_entry);
+		iowrite32(upper_32_bits(delta), reloc_page + offset_in_page(offset));
 	}
 
 	io_mapping_unmap_atomic(reloc_page);
-- 
2.1.0.rc1

Re: [PATCH 2/4] drm/i915: Mark the execbuffer validation failures as unlikely

[Daniel Vetter]

On Sun, Aug 10, 2014 at 06:29:09AM +0100, Chris Wilson wrote:
> This just allows the compiler to pessimise callers who try to abuse the
> ioctl in the hope of making the correct users faster.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I'm not that much of a fan of likely/unlikely really. If it helps with
documenting code then I'm ok, but mass-sprinkling looks like too much.

For this case here I think an unlikely on the return value of
validate_exec_list is about all I want to stomach. gcc should reach all
the other conclusions itself.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 +++++++++++++-----------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4304d8b9e17c..0ba1e7bbd09d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -870,41 +870,38 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
>  	return intel_ring_invalidate_all_caches(ring);
>  }
>  
> -static bool
> -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;
> -}
> -
>  static int
>  validate_exec_list(struct drm_device *dev,
> -		   struct drm_i915_gem_exec_object2 *exec,
> -		   int count)
> +		   const struct drm_i915_gem_execbuffer2 *args,
> +		   const struct drm_i915_gem_exec_object2 *exec)
>  {
>  	unsigned relocs_total = 0;
>  	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
>  	unsigned invalid_flags;
>  	int i;
>  
> +	if (unlikely(args->flags & __I915_EXEC_UNKNOWN_FLAGS))
> +		return -EINVAL;
> +
> +	if (unlikely((args->batch_start_offset | args->batch_len) & 0x7))
> +		return -EINVAL;
> +
>  	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
>  	if (USES_FULL_PPGTT(dev))
>  		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < args->buffer_count; i++) {
>  		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
>  		int length; /* limited by fault_in_pages_readable() */
>  
> -		if (exec[i].flags & invalid_flags)
> +		if (unlikely(exec[i].flags & invalid_flags))
>  			return -EINVAL;
>  
>  		/* First check for malicious input causing overflow in
>  		 * the worst case where we need to allocate the entire
>  		 * relocation tree as a single array.
>  		 */
> -		if (exec[i].relocation_count > relocs_max - relocs_total)
> +		if (unlikely(exec[i].relocation_count > relocs_max - relocs_total))
>  			return -EINVAL;
>  		relocs_total += exec[i].relocation_count;
>  
> @@ -915,11 +912,11 @@ validate_exec_list(struct drm_device *dev,
>  		 * to read, but since we may need to update the presumed
>  		 * offsets during execution, check for full write access.
>  		 */
> -		if (!access_ok(VERIFY_WRITE, ptr, length))
> +		if (unlikely(!access_ok(VERIFY_WRITE, ptr, length)))
>  			return -EFAULT;
>  
>  		if (likely(!i915.prefault_disable)) {
> -			if (fault_in_multipages_readable(ptr, length))
> +			if (unlikely(fault_in_multipages_readable(ptr, length)))
>  				return -EFAULT;
>  		}
>  	}
> @@ -1256,11 +1253,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	int ret;
>  	bool need_relocs;
>  
> -	if (!i915_gem_check_execbuffer(args))
> -		return -EINVAL;
> -
> -	ret = validate_exec_list(dev, exec, args->buffer_count);
> -	if (ret)
> +	ret = validate_exec_list(dev, args, exec);
> +	if (unlikely(ret))
>  		return ret;
>  
>  	flags = 0;
> -- 
> 2.1.0.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Simplify relocate_entry_gtt() and make 64-bit safe

[Daniel Vetter]

On Sun, Aug 10, 2014 at 06:29:11AM +0100, Chris Wilson wrote:
> Even though we should not try to use 4+GiB GTTs on 32-bit systems, by
> using a local variable we can future proof the code whilst making it
> easier to read.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Appeased checkpatch a bit for the long lines and pulled this in with the
other patches from this series, except the likely/unlikely one.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index e6fcd0594bd5..5c04f77f8a66 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -302,7 +302,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint64_t delta = reloc->delta + target_offset;
> -	uint32_t __iomem *reloc_entry;
> +	uint64_t offset;
>  	void __iomem *reloc_page;
>  	int ret;
>  
> @@ -315,25 +315,22 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
>  		return ret;
>  
>  	/* Map the page containing the relocation we're going to perform.  */
> -	reloc->offset += i915_gem_obj_ggtt_offset(obj);
> +	offset = i915_gem_obj_ggtt_offset(obj);
> +	offset += reloc->offset;
>  	reloc_page = io_mapping_map_atomic_wc(dev_priv->gtt.mappable,
> -			reloc->offset & PAGE_MASK);
> -	reloc_entry = (uint32_t __iomem *)
> -		(reloc_page + offset_in_page(reloc->offset));
> -	iowrite32(lower_32_bits(delta), reloc_entry);
> +					      offset & PAGE_MASK);
> +	iowrite32(lower_32_bits(delta), reloc_page + offset_in_page(offset));
>  
>  	if (INTEL_INFO(dev)->gen >= 8) {
> -		reloc_entry += 1;
> +		offset += sizeof(uint32_t);
>  
> -		if (offset_in_page(reloc->offset + sizeof(uint32_t)) == 0) {
> +		if (offset_in_page(offset) == 0) {
>  			io_mapping_unmap_atomic(reloc_page);
> -			reloc_page = io_mapping_map_atomic_wc(
> -					dev_priv->gtt.mappable,
> -					reloc->offset + sizeof(uint32_t));
> -			reloc_entry = reloc_page;
> +			reloc_page = io_mapping_map_atomic_wc(dev_priv->gtt.mappable,
> +							      offset);
>  		}
>  
> -		iowrite32(upper_32_bits(delta), reloc_entry);
> +		iowrite32(upper_32_bits(delta), reloc_page + offset_in_page(offset));
>  	}
>  
>  	io_mapping_unmap_atomic(reloc_page);
> -- 
> 2.1.0.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch