| Message ID | 20150722143915.GO6166@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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); }