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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx