[Intel-gfx] [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson]

On Thu, Jul 02, 2015 at 12:27:42PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/02/2015 12:00 PM, Chris Wilson wrote:
> >>>+		/* This is a slow read/write as it tries to read from
> >>>+		 * and write to user memory which may result into page
> >>>+		 * faults
> >>>+		 */
> >>>+		ret = slow_user_access(dev_priv->gtt.mappable, page_base,
> >>>+				       page_offset, user_data,
> >>>+				       page_length, write);
> >>>+
> >>>+		if (ret) {
> >>>+			ret = -EINVAL;
> >
> >Would be better to return the right errno from slow_user_access or if it
> >is a bool, treat is a bool!
> 
> It returns a number of unwritten bytes.
> 
> Actually just spotted the return type for slow_user_access is also
> wrong, it is an int, to which it implicitly casts an unsigned long.

Ugh, our convention for those has to been to do the unwritten check in
the subroutine and convert it back to the errno. Saves all the guessing
about the failure mode in the caller.
 
> And it is customary to return -EFAULT on rather than -EINVAL so I
> suggest changing that as well.

Yup.
 
> >>>+			break;
> >>>+		}
> >>>+
> >>>+		remain -= page_length;
> >>>+		user_data += page_length;
> >>>+		offset += page_length;
> >>>+	}
> >>>+
> >>>+	mutex_lock(&dev->struct_mutex);
> >>
> >>Caller had it interruptible.
> >
> >On second access, it is preferrable not to risk interruption -
> >especially when catching up with state to return to userspace.
> 
> 
> Second access is only in pwrite case and more so, it makes no sense
> to make such decisions on behalf of the caller. If it is a real
> concern, caller should do it.

That's odd. We must have taken it on both pwrite/pread paths to have
prepared the object for reading/writing. We can only dropped it under
difficult circumstances (lots of dancing required if we do to make sure
that the object hasn't been modified in the meantime).
 
> Yes I see that there is a problem where this helper doesn't know
> what type of mutex caller had, but, it is still bad imho and could
> be fixed in a different way.

Smells fishy indeed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre