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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
