| Message ID | 1470340352-16118-12-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2016-08-04 at 20:52 +0100, Chris Wilson wrote: > If we try and read or write to an active request, we first must wait > upon the GPU completing that request. Let's do that without holding the > mutex (and so allow someone else to access the GPU whilst we wait). Upn STILL TYPO Upon ---^ > /* Bounds check destination. */ > if (args->offset > obj->base.size || > args->size > obj->base.size - args->offset) { > ret = -EINVAL; > - goto out; > + goto err; > } > > - trace_i915_gem_object_pwrite(obj, args->offset, args->size); > + ret = __unsafe_wait_rendering(obj, to_rps_client(file), false); > + if (ret) > + goto err; > > + intel_runtime_pm_get(dev_priv); > + > + ret = i915_mutex_lock_interruptible(dev); > + if (ret) > + goto err_rpm; > + > + trace_i915_gem_object_pwrite(obj, args->offset, args->size); This trace is still moved, maybe add your reasoning to commit message. With those addressed (and the RPM fix in separate patch); Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Fri, Aug 05, 2016 at 10:08:30AM +0300, Joonas Lahtinen wrote: > On to, 2016-08-04 at 20:52 +0100, Chris Wilson wrote: > > If we try and read or write to an active request, we first must wait > > upon the GPU completing that request. Let's do that without holding the > > mutex (and so allow someone else to access the GPU whilst we wait). Upn > > STILL TYPO Upon ---^ > > > /* Bounds check destination. */ > > if (args->offset > obj->base.size || > > args->size > obj->base.size - args->offset) { > > ret = -EINVAL; > > - goto out; > > + goto err; > > } > > > > - trace_i915_gem_object_pwrite(obj, args->offset, args->size); > > + ret = __unsafe_wait_rendering(obj, to_rps_client(file), false); > > + if (ret) > > + goto err; > > > > + intel_runtime_pm_get(dev_priv); > > + > > + ret = i915_mutex_lock_interruptible(dev); > > + if (ret) > > + goto err_rpm; > > + > > + trace_i915_gem_object_pwrite(obj, args->offset, args->size); > > This trace is still moved, maybe add your reasoning to commit message. I think the trace is not good enough. We know we entered the ioctl, we can trace that itself. So the question is what information is valuable at this point, and that would be which path we take (gtt, shmem, phys). Anyway, I'll bump it back to the start and we can add a review of tracepoints to the wishlist. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ee222961c62a..fa0936a787a7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -956,25 +956,26 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, args->size)) return -EFAULT; - ret = i915_mutex_lock_interruptible(dev); - if (ret) - return ret; - obj = i915_gem_object_lookup(file, args->handle); - if (!obj) { - ret = -ENOENT; - goto unlock; - } + if (!obj) + return -ENOENT; /* Bounds check source. */ if (args->offset > obj->base.size || args->size > obj->base.size - args->offset) { ret = -EINVAL; - goto out; + goto err; } - trace_i915_gem_object_pread(obj, args->offset, args->size); + ret = __unsafe_wait_rendering(obj, to_rps_client(file), true); + if (ret) + goto err; + ret = i915_mutex_lock_interruptible(dev); + if (ret) + goto err; + + 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 */ @@ -985,10 +986,13 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, intel_runtime_pm_put(to_i915(dev)); } -out: i915_gem_object_put(obj); -unlock: mutex_unlock(&dev->struct_mutex); + + return ret; + +err: + i915_gem_object_put_unlocked(obj); return ret; } @@ -1374,27 +1378,28 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, return -EFAULT; } - intel_runtime_pm_get(dev_priv); - - ret = i915_mutex_lock_interruptible(dev); - if (ret) - goto put_rpm; - obj = i915_gem_object_lookup(file, args->handle); - if (!obj) { - ret = -ENOENT; - goto unlock; - } + if (!obj) + return -ENOENT; /* Bounds check destination. */ if (args->offset > obj->base.size || args->size > obj->base.size - args->offset) { ret = -EINVAL; - goto out; + goto err; } - trace_i915_gem_object_pwrite(obj, args->offset, args->size); + ret = __unsafe_wait_rendering(obj, to_rps_client(file), false); + if (ret) + goto err; + intel_runtime_pm_get(dev_priv); + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + goto err_rpm; + + trace_i915_gem_object_pwrite(obj, args->offset, args->size); ret = -EFAULT; /* We can only do the GTT pwrite on untiled buffers, as otherwise * it would end up going through the fenced access, and we'll get @@ -1419,14 +1424,17 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, ret = -ENODEV; } -out: i915_gem_object_put(obj); -unlock: mutex_unlock(&dev->struct_mutex); -put_rpm: intel_runtime_pm_put(dev_priv); return ret; + +err_rpm: + intel_runtime_pm_put(dev_priv); +err: + i915_gem_object_put_unlocked(obj); + return ret; } static enum fb_op_origin
If we try and read or write to an active request, we first must wait upon the GPU completing that request. Let's do that without holding the mutex (and so allow someone else to access the GPU whilst we wait). Upn completion, we will reacquire the mutex and only then start the operation (i.e. we do not rely on state from before dropping the mutex). v2: Repaint the goto labels Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 62 +++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 27 deletions(-)