[Intel-gfx] [PATCH 1/2] drm/i915: check execbuffer for validity earlier to save on work.

[Chris Wilson]

On Thu, 22 Apr 2010 22:38:41 +0100, "Owain G. Ainsworth" <zerooa at googlemail.com> wrote:
> Before, we were waiting until we knew the batch object's gtt offset
> before we checked for validity. However, since this is purely an
> alignment check (it is impossible for the batch object to get to
> execbuffer stage without being pinned and bound) we can check alignment
> before we do any other expensive work.
> 
> Additionally, check that batch_start_offset +  batch_len is <= the size
> of the batchbuffer object. And that batch_len and batch_start_offset do
> not overflow a u_int32_t.
> 
> Signed-off-by: Owain G. Ainsworth <oga at openbsd.org>

Minor comment inline.
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   42 ++++++++++++++------------------------
>  1 files changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b85727c..a9da182 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3656,23 +3656,6 @@ err:
>  	return ret;
>  }
>  
> -static int
> -i915_gem_check_execbuffer (struct drm_i915_gem_execbuffer2 *exec,
> -			   uint64_t exec_offset)
> -{
> -	uint32_t exec_start, exec_len;
> -
> -	exec_start = (uint32_t) exec_offset + exec->batch_start_offset;
> -	exec_len = (uint32_t) exec->batch_len;
> -
> -	if ((exec_start | exec_len) & 0x7)
> -		return -EINVAL;
> -
> -	if (!exec_start)
> -		return -EINVAL;
> -
> -	return 0;
> -}
>  
>  static int
>  i915_gem_wait_for_pending_flip(struct drm_device *dev,
> @@ -3722,7 +3705,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	struct drm_clip_rect *cliprects = NULL;
>  	struct drm_i915_gem_relocation_entry *relocs = NULL;
>  	int ret = 0, ret2, i, pinned = 0;
> -	uint64_t exec_offset;
>  	uint32_t seqno, flush_domains, reloc_index;
>  	int pin_tries, flips;
>  
> @@ -3730,6 +3712,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
>  		  (int) args->buffers_ptr, args->buffer_count, args->batch_len);
>  #endif
> +	/*
> +	 * check for valid execbuffer offset. We can do this early because
> +	 * bound objects are always page aligned, so only the start offset
> +	 * matters. Also check for overflow.
> +	 */
> +	if ((args->batch_start_offset | args->batch_len) & 0x7 ||
> +	    args->batch_start_offset + args->batch_len < args->batch_len ||
> +	    args->batch_start_offset + args->batch_len <
> +	    args->batch_start_offset)
> +		return -EINVAL;

A minor critique, can we move this predicate into an inline for the sake
of the reader? i915_gem_check_execbuffer_offset(args) perhaps?
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre