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

[Joonas Lahtinen]

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