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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@chris-wilson.co.uk>
>>> Cc: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx