Re: [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer

[Daniel Vetter <daniel@ffwll.ch>]

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@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

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