From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer Date: Fri, 23 May 2014 13:59:13 +0200 Message-ID: <20140523115913.GS14357@phenom.ffwll.local> References: <1400838352-19420-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 5456D6E582 for ; Fri, 23 May 2014 04:59:18 -0700 (PDT) Received: by mail-ee0-f49.google.com with SMTP id e53so3492660eek.8 for ; Fri, 23 May 2014 04:59:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1400838352-19420-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > Cc: Daniel Vetter 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