* [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer
@ 2014-05-23 9:45 Chris Wilson
2014-05-23 11:59 ` Daniel Vetter
0 siblings, 1 reply; 2+ messages in thread
From: Chris Wilson @ 2014-05-23 9:45 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
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>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer
2014-05-23 9:45 [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer Chris Wilson
@ 2014-05-23 11:59 ` Daniel Vetter
0 siblings, 0 replies; 2+ messages in thread
From: Daniel Vetter @ 2014-05-23 11:59 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-05-23 11:59 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-23 9:45 [PATCH] drm/i915: Only copy back the modified fields to userspace from execbuffer Chris Wilson
2014-05-23 11:59 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox