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

2010-04-22 Thread Owain G. Ainsworth
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
---
 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;
 
if (args-buffer_count  1) {
DRM_ERROR(execbuf with %d buffers\n, args-buffer_count);
@@ -3878,15 +3870,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void 
*data,
ret = -EINVAL;
goto err;
}
-   batch_obj-pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
-
-   /* Sanity check the batch buffer, prior to moving objects */
-   exec_offset = exec_list[args-buffer_count - 1].offset;
-   ret = i915_gem_check_execbuffer (args, exec_offset);
-   if (ret != 0) {
-   DRM_ERROR(execbuf with invalid offset/length\n);
+   if (args-batch_start_offset + args-batch_len  batch_obj-size) {
+   DRM_ERROR(batchbuffer shorter than program length\n);
+   ret = EINVAL;
goto err;
}
+   batch_obj-pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
i915_verify_inactive(dev, __FILE__, __LINE__);
 
@@ -3955,7 +3944,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 #endif
 
/* Exec the batchbuffer */
-   ret = i915_dispatch_gem_execbuffer(dev, args, cliprects, exec_offset);
+   ret = i915_dispatch_gem_execbuffer(dev, args, cliprects,
+   exec_list[args-buffer_count - 1].offset);
if (ret) {
DRM_ERROR(dispatch failed %d\n, ret);
goto err;
-- 
1.6.5.7

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


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

2010-04-22 Thread Chris Wilson
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