On 06/01/2017 16:24, Chris Wilson wrote:
On Fri, Jan 06, 2017 at 04:04:44PM +0000, Tvrtko Ursulin wrote:
On 06/01/2017 15:22, Chris Wilson wrote:
Since commit fe115628d567 ("drm/i915: Implement pwrite without
struct-mutex") the lowlevel pwrite calls are now called without the
protection of struct_mutex, but pwrite_phys was still asserting that it
held the struct_mutex and later tried to drop and relock it.
Fixes: fe115628d567 ("drm/i915: Implement pwrite without struct-mutex")
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: <drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 34 ++++------------------------------
1 file changed, 4 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91072ebdbdcb..94958437252a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -597,47 +597,21 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
struct drm_i915_gem_pwrite *args,
struct drm_file *file)
{
- struct drm_device *dev = obj->base.dev;
void *vaddr = obj->phys_handle->vaddr + args->offset;
char __user *user_data = u64_to_user_ptr(args->data_ptr);
- int ret;
/* We manually control the domain here and pretend that it
* remains coherent i.e. in the GTT domain, like shmem_pwrite.
*/
- lockdep_assert_held(&obj->base.dev->struct_mutex);
- ret = i915_gem_object_wait(obj,
- I915_WAIT_INTERRUPTIBLE |
- I915_WAIT_LOCKED |
- I915_WAIT_ALL,
- MAX_SCHEDULE_TIMEOUT,
- to_rps_client(file));
- if (ret)
- return ret;
-
intel_fb_obj_invalidate(obj, ORIGIN_CPU);
- if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
- unsigned long unwritten;
-
- /* The physical object once assigned is fixed for the lifetime
- * of the obj, so we can safely drop the lock and continue
- * to access vaddr.
- */
- mutex_unlock(&dev->struct_mutex);
- unwritten = copy_from_user(vaddr, user_data, args->size);
- mutex_lock(&dev->struct_mutex);
- if (unwritten) {
- ret = -EFAULT;
- goto out;
- }
- }
+ if (copy_from_user(vaddr, user_data, args->size))
+ return -EFAULT;
drm_clflush_virt_range(vaddr, args->size);
- i915_gem_chipset_flush(to_i915(dev));
+ i915_gem_chipset_flush(to_i915(obj->base.dev));
-out:
intel_fb_obj_flush(obj, false, ORIGIN_CPU);
- return ret;
+ return 0;
}
void *i915_gem_object_alloc(struct drm_i915_private *dev_priv)
First time round I missed how obviously broken the copy was and that
wait is already done one level up. What I missed from the original
work is, now it is quite possible for userspace to see/create
corruption if it doesn't serialize rendering and pread/pwrite
itself. Is that OK?
Yes. We take the general view that we cannot prevent userspace from
racing with itself. Consider not just concurrent pwrites, but vs GTT,
CPU, wC mmaps and execbuf.
Starting from scratch, we would fence everything (including CPU access)
and have no implicit syncs and rely on userspace using the fences
correctly (if they wish to avoid seeing corruption).
Good, just double checking I got everything right.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
