Re: [PATCH 09/16] drm/i915: Do a nonblocking wait first in pread/pwrite

Joonas Lahtinen
2016-08-04T07:53:34Z

On ma, 2016-08-01 at 19:22 +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
Upon --^
> 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).
>

<SNIP>

> @@ -953,25 +953,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 out_unlocked;

Again the sole exit path, could be just 'err'

> @@ -1368,27 +1372,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 out_unlocked;

Ditto in this func, just 'err'

>   }
>
> - trace_i915_gem_object_pwrite(obj, args->offset, args->size);
> + ret = __unsafe_wait_rendering(obj, to_rps_client(file), false);
> + if (ret)
> + goto out_unlocked;
> +
> + intel_runtime_pm_get(dev_priv);
>

As discussed in IRC, pread_ioctl does not take RPM for the fallback
path, it should.

> + ret = i915_mutex_lock_interruptible(dev);
> + if (ret)
> + goto out_rpm;
> +
> + trace_i915_gem_object_pwrite(obj, args->offset, args->size);

For tracing, I'm not quite sure if we should emit failed attempts too,
I guess it's a policy decision?

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation