[Intel-gfx] [PATCH v3] drm/i915: Add to timeline requires the timeline mutex

[Tvrtko Ursulin]

On 08/07/2019 12:39, Chris Wilson wrote:
> Modifying a remote context requires careful serialisation with requests
> on that context, and that serialisation requires us to take their
> timeline->mutex. Make it so.
> 
> Note that while struct_mutex rules, we can't create more than one
> request in parallel, but that age is soon coming to an end.
> 
> v2: Though it doesn't affect the current users, contexts may share
> timelines so check if we already hold the right mutex.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 16 ++-------
>   drivers/gpu/drm/i915/gt/intel_context.c     | 38 +++++++++++++++++++++
>   drivers/gpu/drm/i915/gt/intel_context.h     |  3 ++
>   drivers/gpu/drm/i915/i915_perf.c            |  7 +---
>   4 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 1f0d10bb88c1..6000177472ee 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1192,20 +1192,8 @@ gen8_modify_rpcs(struct intel_context *ce, struct intel_sseu sseu)
>   	if (IS_ERR(rq))
>   		return PTR_ERR(rq);
>   
> -	/* Queue this switch after all other activity by this context. */
> -	ret = i915_active_request_set(&ce->ring->timeline->last_request, rq);
> -	if (ret)
> -		goto out_add;
> -
> -	/*
> -	 * Guarantee context image and the timeline remains pinned until the
> -	 * modifying request is retired by setting the ce activity tracker.
> -	 *
> -	 * But we only need to take one pin on the account of it. Or in other
> -	 * words transfer the pinned ce object to tracked active request.
> -	 */
> -	GEM_BUG_ON(i915_active_is_idle(&ce->active));
> -	ret = i915_active_ref(&ce->active, rq->fence.context, rq);
> +	/* Serialise with the remote context */
> +	ret = intel_context_prepare_remote_request(ce, rq);
>   	if (ret)
>   		goto out_add;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 1110fc8f657a..b1e2e4b60027 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -239,6 +239,44 @@ void intel_context_exit_engine(struct intel_context *ce)
>   	intel_engine_pm_put(ce->engine);
>   }
>   
> +int intel_context_prepare_remote_request(struct intel_context *ce,
> +					 struct i915_request *rq)
> +{
> +	struct intel_timeline *tl = ce->ring->timeline;
> +	int err;
> +
> +	/* Only suitable for use in remotely modifying this context */
> +	GEM_BUG_ON(rq->hw_context == ce);
> +
> +	if (rq->timeline != tl) { /* beware timeline sharing */
> +		err = mutex_lock_interruptible_nested(&tl->mutex,
> +						      SINGLE_DEPTH_NESTING);

Which caller is holding tl->mutex?

Regards,

Tvrtko

> +		if (err)
> +			return err;
> +	}
> +	lockdep_assert_held(&tl->mutex);
> +
> +	/* Queue this switch after all other activity by this context. */
> +	err = i915_active_request_set(&tl->last_request, rq);
> +	if (err)
> +		goto unlock;
> +
> +	/*
> +	 * Guarantee context image and the timeline remains pinned until the
> +	 * modifying request is retired by setting the ce activity tracker.
> +	 *
> +	 * But we only need to take one pin on the account of it. Or in other
> +	 * words transfer the pinned ce object to tracked active request.
> +	 */
> +	GEM_BUG_ON(i915_active_is_idle(&ce->active));
> +	err = i915_active_ref(&ce->active, rq->fence.context, rq);
> +
> +unlock:
> +	if (rq->timeline != tl)
> +		mutex_unlock(&tl->mutex);
> +	return err;
> +}
> +
>   struct i915_request *intel_context_create_request(struct intel_context *ce)
>   {
>   	struct i915_request *rq;
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 40cd8320fcc3..b41c610c2ce6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -139,6 +139,9 @@ static inline void intel_context_timeline_unlock(struct intel_context *ce)
>   	mutex_unlock(&ce->ring->timeline->mutex);
>   }
>   
> +int intel_context_prepare_remote_request(struct intel_context *ce,
> +					 struct i915_request *rq);
> +
>   struct i915_request *intel_context_create_request(struct intel_context *ce);
>   
>   #endif /* __INTEL_CONTEXT_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 25c34a0b521e..6f77014640b8 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1762,12 +1762,7 @@ static int gen8_modify_context(struct intel_context *ce,
>   		return PTR_ERR(rq);
>   
>   	/* Serialise with the remote context */
> -	err = i915_active_request_set(&ce->ring->timeline->last_request, rq);
> -	if (err)
> -		goto out_add;
> -
> -	/* Keep the remote context alive until after we finish editing */
> -	err = i915_active_ref(&ce->active, rq->fence.context, rq);
> +	err = intel_context_prepare_remote_request(ce, rq);
>   	if (err)
>   		goto out_add;
>   
>