On Thu, 22 Apr 2010 22:38:41 +0100, "Owain G. Ainsworth"
<zer...@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 <o...@openbsd.org>
Minor comment inline.
Reviewed-by: Chris Wilson <ch...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
