[4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects
diff mbox

Message ID 20150722143915.GO6166@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson July 22, 2015, 2:39 p.m. UTC
On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
>  static int
>  i915_gem_shmem_pread(struct drm_device *dev,
>  		     struct drm_i915_gem_object *obj,
> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	/* prime objects have no backing filp to GEM pread/pwrite
> -	 * pages from.
> -	 */
> -	if (!obj->base.filp) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	trace_i915_gem_object_pread(obj, args->offset, args->size);
>  
> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
> +	/* pread for non shmem backed objects */
> +	if (!obj->base.filp) {
> +		if (obj->tiling_mode == I915_TILING_NONE)

pread/pwrite is defined as a cpu linear, the restriction upon tiling is
a simplification of handling swizzling.

> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> +							args->offset,
> +							args->data_ptr,
> +							false);
> +		else
> +			ret = -EINVAL;
> +	} else {
> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
> +	}
>  
>  out:
>  	drm_gem_object_unreference(&obj->base);
> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	/* prime objects have no backing filp to GEM pread/pwrite
> -	 * pages from.
> -	 */
> -	if (!obj->base.filp) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>  
>  	ret = -EFAULT;
> +
> +	/* pwrite for non shmem backed objects */
> +	if (!obj->base.filp) {
> +		if (obj->tiling_mode == I915_TILING_NONE)
> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> +							args->offset,
> +							args->data_ptr,
> +							true);
> +		else
> +			ret = -EINVAL;
> +
> +		goto out;

The fast pwrite code always works for obj->base.filp == NULL. To easily
make it generic and handle the cases where we cannot fallback to shem,
undo the PIN_NONBLOCK.

Then you just want something like

to enable pwrite access to stolen.
-Chris

Comments

akash.goel@intel.com July 31, 2015, 1:16 p.m. UTC | #1
On 7/22/2015 8:09 PM, Chris Wilson wrote:
> On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
>>   static int
>>   i915_gem_shmem_pread(struct drm_device *dev,
>>   		     struct drm_i915_gem_object *obj,
>> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>>   		goto out;
>>   	}
>>
>> -	/* prime objects have no backing filp to GEM pread/pwrite
>> -	 * pages from.
>> -	 */
>> -	if (!obj->base.filp) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>>   	trace_i915_gem_object_pread(obj, args->offset, args->size);
>>
>> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
>> +	/* pread for non shmem backed objects */
>> +	if (!obj->base.filp) {
>> +		if (obj->tiling_mode == I915_TILING_NONE)
>
> pread/pwrite is defined as a cpu linear, the restriction upon tiling is
> a simplification of handling swizzling.
>
>> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
>> +							args->offset,
>> +							args->data_ptr,
>> +							false);
>> +		else
>> +			ret = -EINVAL;
>> +	} else {
>> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
>> +	}
>>
>>   out:
>>   	drm_gem_object_unreference(&obj->base);
>> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>>   		goto out;
>>   	}
>>
>> -	/* prime objects have no backing filp to GEM pread/pwrite
>> -	 * pages from.
>> -	 */
>> -	if (!obj->base.filp) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>>   	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>>
>>   	ret = -EFAULT;
>> +
>> +	/* pwrite for non shmem backed objects */
>> +	if (!obj->base.filp) {
>> +		if (obj->tiling_mode == I915_TILING_NONE)
>> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
>> +							args->offset,
>> +							args->data_ptr,
>> +							true);
>> +		else
>> +			ret = -EINVAL;
>> +
>> +		goto out;
>
> The fast pwrite code always works for obj->base.filp == NULL. To easily
> make it generic and handle the cases where we cannot fallback to shem,
> undo the PIN_NONBLOCK.
>
> Then you just want something like
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d3016f37cd4d..f2284a27dd6d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1114,8 +1114,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>           * perspective, requiring manual detiling by the client.
>           */
>          if (obj->tiling_mode == I915_TILING_NONE &&

Since the suggestion is to reuse the gtt_pwrite_fast function only for 
non-shmem backed objects, can we also relax the Tiling constraint here, 
apart from removing the PIN_NONBLOCK flag. As anyways there is a 
put_fence already being done in gtt_pwrite_fast function.

Also currently the gtt_pwrite_fast function is non-tolerant to faults, 
incurred while accessing the User space buffer, so can that also be 
relaxed (at least for the non-shmem backed objects) ??

Best regards
Akash

> -           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> -           cpu_write_needs_clflush(obj)) {
> +           (obj->base.filp == NULL ||
> +            (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> +             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
> @@ -1125,7 +1126,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>          if (ret == -EFAULT || ret == -ENOSPC) {
>                  if (obj->phys_handle)
>                          ret = i915_gem_phys_pwrite(obj, args, file);
> -               else
> +               else if (obj->filp)
>                          ret = i915_gem_shmem_pwrite(dev, obj, args, file);
>          }
>
>
> to enable pwrite access to stolen.
> -Chris
>
ankitprasad.r.sharma@intel.com Sept. 10, 2015, 5:50 p.m. UTC | #2
On Fri, 2015-07-31 at 18:46 +0530, Goel, Akash wrote:
> 
> On 7/22/2015 8:09 PM, Chris Wilson wrote:
> > On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
> >>   static int
> >>   i915_gem_shmem_pread(struct drm_device *dev,
> >>   		     struct drm_i915_gem_object *obj,
> >> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
> >>   		goto out;
> >>   	}
> >>
> >> -	/* prime objects have no backing filp to GEM pread/pwrite
> >> -	 * pages from.
> >> -	 */
> >> -	if (!obj->base.filp) {
> >> -		ret = -EINVAL;
> >> -		goto out;
> >> -	}
> >> -
> >>   	trace_i915_gem_object_pread(obj, args->offset, args->size);
> >>
> >> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
> >> +	/* pread for non shmem backed objects */
> >> +	if (!obj->base.filp) {
> >> +		if (obj->tiling_mode == I915_TILING_NONE)
> >
> > pread/pwrite is defined as a cpu linear, the restriction upon tiling is
> > a simplification of handling swizzling.
> >
> >> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> >> +							args->offset,
> >> +							args->data_ptr,
> >> +							false);
> >> +		else
> >> +			ret = -EINVAL;
> >> +	} else {
> >> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
> >> +	}
> >>
> >>   out:
> >>   	drm_gem_object_unreference(&obj->base);
> >> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >>   		goto out;
> >>   	}
> >>
> >> -	/* prime objects have no backing filp to GEM pread/pwrite
> >> -	 * pages from.
> >> -	 */
> >> -	if (!obj->base.filp) {
> >> -		ret = -EINVAL;
> >> -		goto out;
> >> -	}
> >> -
> >>   	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
> >>
> >>   	ret = -EFAULT;
> >> +
> >> +	/* pwrite for non shmem backed objects */
> >> +	if (!obj->base.filp) {
> >> +		if (obj->tiling_mode == I915_TILING_NONE)
> >> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> >> +							args->offset,
> >> +							args->data_ptr,
> >> +							true);
> >> +		else
> >> +			ret = -EINVAL;
> >> +
> >> +		goto out;
> >
> > The fast pwrite code always works for obj->base.filp == NULL. To easily
> > make it generic and handle the cases where we cannot fallback to shem,
> > undo the PIN_NONBLOCK.
> >
> > Then you just want something like
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d3016f37cd4d..f2284a27dd6d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1114,8 +1114,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >           * perspective, requiring manual detiling by the client.
> >           */
> >          if (obj->tiling_mode == I915_TILING_NONE &&
> 
> Since the suggestion is to reuse the gtt_pwrite_fast function only for 
> non-shmem backed objects, can we also relax the Tiling constraint here, 
> apart from removing the PIN_NONBLOCK flag. As anyways there is a 
> put_fence already being done in gtt_pwrite_fast function.
> 
> Also currently the gtt_pwrite_fast function is non-tolerant to faults, 
> incurred while accessing the User space buffer, so can that also be 
> relaxed (at least for the non-shmem backed objects) ??

Even if we reuse the gtt_pwrite_fast we will have to handle page-faults
for non-shmem backed objects, that will contradict the purpose of
gtt_pwrite_fast. The gtt_pread_pwrite will still be around for pread
purpose.

Also, I think it should be ok to relax the tiling constraint as well, as
we put_fence everytime we try to pread/pwrite from/to a non-shmem-backed
object here.

Thanks,
Ankit
Chris Wilson Sept. 15, 2015, 9:58 a.m. UTC | #3
On Fri, Jul 31, 2015 at 06:46:20PM +0530, Goel, Akash wrote:
> Since the suggestion is to reuse the gtt_pwrite_fast function only
> for non-shmem backed objects, can we also relax the Tiling
> constraint here, apart from removing the PIN_NONBLOCK flag. As
> anyways there is a put_fence already being done in gtt_pwrite_fast
> function.

The tiling restraint is due to a hardware limitation (the effect of
swizzling), you cannot simply drop it.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d3016f37cd4d..f2284a27dd6d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1114,8 +1114,9 @@  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
         * perspective, requiring manual detiling by the client.
         */
        if (obj->tiling_mode == I915_TILING_NONE &&
-           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-           cpu_write_needs_clflush(obj)) {
+           (obj->base.filp == NULL ||
+            (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
+             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
@@ -1125,7 +1126,7 @@  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
        if (ret == -EFAULT || ret == -ENOSPC) {
                if (obj->phys_handle)
                        ret = i915_gem_phys_pwrite(obj, args, file);
-               else
+               else if (obj->filp)
                        ret = i915_gem_shmem_pwrite(dev, obj, args, file);
        }