[Intel-gfx] [PATCH] drm/i915: pwrite/pread do not require obj->base.filp, just pages

[Chris Wilson]

On Mon, Jun 13, 2016 at 02:12:46PM +0100, Tvrtko Ursulin wrote:
> 
> On 13/06/16 13:52, Chris Wilson wrote:
> >On Mon, Jun 13, 2016 at 01:45:56PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 13/06/16 13:40, Chris Wilson wrote:
> >>>The idea behind relaxing the restriction for pread/pwrite was to handle
> >>>!obj->base.flip, i.e. non-shmemfs backed objects, which only requires
> >>>that the object provide struct pages.
> >>>
> >>>v2: Remove excess (). Note enough editing after copy'n'paste.
> >>>
> >>>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>Cc: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> >>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>---
> >>>  drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
> >>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>>index 21d0dea57312..6f950c37efab 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>@@ -760,7 +760,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
> >>>  	int needs_clflush = 0;
> >>>  	struct sg_page_iter sg_iter;
> >>>
> >>>-	if (!obj->base.filp)
> >>>+	if ((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0)
> >>>  		return -ENODEV;
> >>>
> >>>  	user_data = u64_to_user_ptr(args->data_ptr);
> >>>@@ -1298,7 +1298,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >>>  	 * pread/pwrite currently are reading and writing from the CPU
> >>>  	 * perspective, requiring manual detiling by the client.
> >>>  	 */
> >>>-	if (!obj->base.filp || cpu_write_needs_clflush(obj)) {
> >>>+	if ((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0 ||
> >>>+	    cpu_write_needs_clflush(obj)) {
> >>>  		ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
> >>>  		/* Note that the gtt paths might fail with non-page-backed user
> >>>  		 * pointers (e.g. gtt mappings when moving data between
> >>>@@ -1308,7 +1309,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >>>  	if (ret == -EFAULT) {
> >>>  		if (obj->phys_handle)
> >>>  			ret = i915_gem_phys_pwrite(obj, args, file);
> >>>-		else if (obj->base.filp)
> >>>+		else if (obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE)
> >>>  			ret = i915_gem_shmem_pwrite(dev, obj, args, file);
> >>>  		else
> >>>  			ret = -ENODEV;
> >>>
> >>
> >>To enable on userptr or there is more to it? Would it even make more
> >>sense to keep rejecting it on userptr to discourage suboptimal
> >>userspace?
> >
> >And prime objects, and everything in future not backed by a filp.
> 
> Hmm.. I sense a hole in the IGT coverage. :)
> 
> You definitely do not mind allowing it with userptr?

Don't mind. People shouldn't use it, but I'd rather have fewer special
cases in the ABI.
 
> Actually, we don't need to go through the aperture for anything but
> stolen, right? A third, more optimal path could be added for page
> backed objects which are not shmem, not userptr and not stolen.

Hence s/obj->base.filp/obj->op->flags & HAS_STRUCT_PAGE/. The shmem path
is for direct access to struct page, everything else requires
indirect access through the GTT.

In the future, we may want indirect access through another set of pfn,
but that is likely overkill.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre