On Fri, May 23, 2014 at 10:45:52AM +0100, Chris Wilson wrote:
> We only want to modifiy a single field in the userspace view of the
> execbuffer command buffer, so explicitly change that rather than copy
> everything back again.
>
> This serves two purposes:
>
> 1. The single fields are much cheaper to copy (constant size so the
> copy uses special case code) and much smaller than the whole array.
>
> 2. We modify the array for internal use that need to be masked from
> the user.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Yeah, makes sense, both as a bugfix for the batch bias EINVAL confusion
and in general. Picked up for -fixes, thanks for the patch. And I've
applied the batch bias patch again, too.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 54 ++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index b4020b9503fe..3a30133f93e8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -797,9 +797,9 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> * relocations were valid.
> */
> for (j = 0; j < exec[i].relocation_count; j++) {
> - if (copy_to_user(&user_relocs[j].presumed_offset,
> - &invalid_offset,
> - sizeof(invalid_offset))) {
> + if (__copy_to_user(&user_relocs[j].presumed_offset,
> + &invalid_offset,
> + sizeof(invalid_offset))) {
> ret = -EFAULT;
> mutex_lock(&dev->struct_mutex);
> goto err;
> @@ -1460,18 +1460,21 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
>
> ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
> if (!ret) {
> + struct drm_i915_gem_exec_object __user *user_exec_list =
> + to_user_ptr(args->buffers_ptr);
> +
> /* Copy the new buffer offsets back to the user's exec list. */
> - for (i = 0; i < args->buffer_count; i++)
> - exec_list[i].offset = exec2_list[i].offset;
> - /* ... and back out to userspace */
> - ret = copy_to_user(to_user_ptr(args->buffers_ptr),
> - exec_list,
> - sizeof(*exec_list) * args->buffer_count);
> - if (ret) {
> - ret = -EFAULT;
> - DRM_DEBUG("failed to copy %d exec entries "
> - "back to user (%d)\n",
> - args->buffer_count, ret);
> + for (i = 0; i < args->buffer_count; i++) {
> + ret = __copy_to_user(&user_exec_list[i].offset,
> + &exec2_list[i].offset,
> + sizeof(user_exec_list[i].offset));
> + if (ret) {
> + ret = -EFAULT;
> + DRM_DEBUG("failed to copy %d exec entries "
> + "back to user (%d)\n",
> + args->buffer_count, ret);
> + break;
> + }
> }
> }
>
> @@ -1522,14 +1525,21 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list);
> if (!ret) {
> /* Copy the new buffer offsets back to the user's exec list. */
> - ret = copy_to_user(to_user_ptr(args->buffers_ptr),
> - exec2_list,
> - sizeof(*exec2_list) * args->buffer_count);
> - if (ret) {
> - ret = -EFAULT;
> - DRM_DEBUG("failed to copy %d exec entries "
> - "back to user (%d)\n",
> - args->buffer_count, ret);
> + struct drm_i915_gem_exec_object2 *user_exec_list =
> + to_user_ptr(args->buffers_ptr);
> + int i;
> +
> + for (i = 0; i < args->buffer_count; i++) {
> + ret = __copy_to_user(&user_exec_list[i].offset,
> + &exec2_list[i].offset,
> + sizeof(user_exec_list[i].offset));
> + if (ret) {
> + ret = -EFAULT;
> + DRM_DEBUG("failed to copy %d exec entries "
> + "back to user\n",
> + args->buffer_count);
> + break;
> + }
> }
> }
>
> --
> 2.0.0.rc2
>
--
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
