Re: [PATCH 01/16] drm/i915: Introduce i915_gem_active_wait_unlocked()

Joonas Lahtinen
2016-08-03T11:41:08Z

On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> It is useful to be able to wait on pending rendering without grabbing
> the struct_mutex. We can do this by using the i915_gem_active_get_rcu()
> primitive to acquire a reference to the pending request without
> requiring struct_mutex, just the RCU read lock, and then call
> i915_wait_request().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.h | 36 +++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 48382ac401fd..d077b023a89f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -546,6 +546,42 @@ i915_gem_active_wait(const struct i915_gem_active *active, struct mutex *mutex)
>  }
>
>  /**
> + * i915_gem_active_wait_unlocked - waits until the request is completed
> + * @active - the active request on which to wait
> + * @interruptible - whether the wait can be woken by a userspace signal
> + * @timeout - how long to wait at most
> + * @rps - userspace client to charge for a waitboost
> + *
> + * i915_gem_active_wait_unlocked() waits until the request is completed before
> + * returning, without requiring any locks to be held. Note that it does not
> + * retire any requests before returning.
> + *
> + * This function wraps i915_wait_request(), see it for the full details.
> + *
> + * Returns 0 if successful, or a negative error code.
> + */
> +static inline int
> +i915_gem_active_wait_unlocked(const struct i915_gem_active *active,
> +       bool interruptible,
> +       s64 *timeout,
> +       struct intel_rps_client *rps)
> +{
> + struct drm_i915_gem_request *request;
> + int ret = 0;
> +
> + rcu_read_lock();
> + request = i915_gem_active_get_rcu(active);
> + rcu_read_unlock();

This looks weird compared to the usual way RCU is used, documentation
explicitly specifies that stuff obtained under rcu_read_lock() can not
be referenced after rcu_read_unlock(). I'd put the rcu_read_lock()
section inside i915_gem_active_get_rcu() to make this less confusing.

Regards, Joonas

> +
> + if (request) {
> + ret = i915_wait_request(request, interruptible, timeout, rps);
> + i915_gem_request_put(request);
> + }
> +
> + return ret;
> +}
> +
> +/**
>   * i915_gem_active_retire - waits until the request is retired
>   * @active - the active request on which to wait
>   *
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation