[PATCH 11/87] drm/i915: Fix phys pwrite for struct_mutex-less operation

[Chris Wilson]

On Fri, Jan 06, 2017 at 11:49:54AM +0000, Tvrtko Ursulin wrote:
> 
> On 05/01/2017 23:19, 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 at chris-wilson.co.uk>
> >Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >Cc: <drm-intel-fixes at lists.freedesktop.org>
> >---
> > 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 65373b66a953..c8bc2cd9d479 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)
> >
> 
> Could you please explain the rest of the change in the commit -
> rationale behind dropping the wait and change of copying strategy?

We have already waited. Claiming to wait under the lock hits a BUG_ON.
We don't hold the lock, so the copy path also hits a (lockdep) BUG_ON.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre