Re: [PATCH 02/16] drm/i915: Convert non-blocking waits for requests over to using RCU

Joonas Lahtinen
2016-08-03T13:23:40Z

On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> - if (!obj) {
> - ret = -ENOENT;
> - goto unlock;
> - }
> + if (!obj)
> + return -ENOENT;
>
>   /* Try to flush the object off the GPU without holding the lock.
>    * We will repeat the flush holding the lock in the normal manner
>    * to catch cases where we are gazumped.
>    */
> - ret = i915_gem_object_wait_rendering__nonblocking(obj,
> -   to_rps_client(file),
> -   !write_domain);
> + ret = __unsafe_wait_rendering(obj, to_rps_client(file), !write_domain);
>   if (ret)
> - goto unref;
> + goto out_unlocked;
> +
> + ret = i915_mutex_lock_interruptible(dev);
> + if (ret)
> + goto out_unlocked;
>
>   if (read_domains & I915_GEM_DOMAIN_GTT)
>   ret = i915_gem_object_set_to_gtt_domain(obj, write_domain != 0);
> @@ -1501,11 +1481,13 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>   if (write_domain != 0)
>   intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
>
> -unref:
>   i915_gem_object_put(obj);
> -unlock:
>   mutex_unlock(&dev->struct_mutex);
>   return ret;
> +
> +out_unlocked:

This is the sole label, you could call 'err' too.

> + i915_gem_object_put_unlocked(obj);
> + return ret;
>  }
>
>  /**
> @@ -1647,6 +1629,15 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>   int ret = 0;
>   bool write = !!(vmf->flags & FAULT_FLAG_WRITE);
>
> + /* Try to flush the object off the GPU first without holding the lock.
> +  * Upon acquiring the lock, we will perform our sanity checks and then
> +  * repeat the flush holding the lock in the normal manner to catch cases
> +  * where we are gazumped.
> +  */
> + ret = __unsafe_wait_rendering(obj, NULL, !write);
> + if (ret)
> + goto err;
> +

Why do you lift this call super early, tracing will be affected at
least.

>   intel_runtime_pm_get(dev_priv);
>
>   /* We don't use vmf->pgoff since that has the fake offset */
> @@ -1655,23 +1646,14 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>
>   ret = i915_mutex_lock_interruptible(dev);
>   if (ret)
> - goto out;
> + goto err_rpm;
>
>   trace_i915_gem_object_fault(obj, page_offset, true, write);
>
> - /* Try to flush the object off the GPU first without holding the lock.
> -  * Upon reacquiring the lock, we will perform our sanity checks and then
> -  * repeat the flush holding the lock in the normal manner to catch cases
> -  * where we are gazumped.
> -  */
> - ret = i915_gem_object_wait_rendering__nonblocking(obj, NULL, !write);
> - if (ret)
> - goto unlock;
> -
>   /* Access to snoopable pages through the GTT is incoherent. */
>   if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(dev)) {
>   ret = -EFAULT;
> - goto unlock;
> + goto err_unlock;
>   }
>
>   /* Use a partial view if the object is bigger than the aperture. */
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation