[Intel-gfx] [PATCH] drm/i915: Spin until breadcrumb threads are complete

[Tvrtko Ursulin]

On 08/11/2016 14:37, Chris Wilson wrote:
> When we need to reset the global seqno on wraparound, we have to wait
> until the current rbtrees are drained (or otherwise the next waiter will
> be out of sequence). The current mechanism to kick and spin until
> complete, may exit too early as it would break if the target thread was
> currently running. Instead, we must wake up the threads, but keep
> spinning until the trees have been deleted.
>
> In order to appease Tvrtko, busy spin rather than yield().
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c  |  5 ++---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 31 ++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  3 +--
>  3 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index d866069c1767..6c71637d4be1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -319,9 +319,8 @@ static int i915_gem_init_global_seqno(struct drm_i915_private *i915, u32 seqno)
>
>  	/* If the seqno wraps around, we need to clear the breadcrumb rbtree */
>  	if (!i915_seqno_passed(seqno, atomic_read(&timeline->next_seqno))) {
> -		while (intel_kick_waiters(i915) || intel_kick_signalers(i915))
> -			yield();
> -		yield();
> +		while (intel_breadcrumbs_busy(i915))
> +			cond_resched(); /* spin until threads are complete */
>  	}
>  	atomic_set(&timeline->next_seqno, seqno);
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index c410d3d6465f..c9c46a538edb 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -629,35 +629,28 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>  	cancel_fake_irq(engine);
>  }
>
> -unsigned int intel_kick_waiters(struct drm_i915_private *i915)
> +unsigned int intel_breadcrumbs_busy(struct drm_i915_private *i915)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
>  	unsigned int mask = 0;
>
> -	/* To avoid the task_struct disappearing beneath us as we wake up
> -	 * the process, we must first inspect the task_struct->state under the
> -	 * RCU lock, i.e. as we call wake_up_process() we must be holding the
> -	 * rcu_read_lock().
> -	 */
> -	for_each_engine(engine, i915, id)
> -		if (unlikely(intel_engine_wakeup(engine)))
> -			mask |= intel_engine_flag(engine);
> +	for_each_engine(engine, i915, id) {
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
>
> -	return mask;
> -}
> +		spin_lock_irq(&b->lock);
>
> -unsigned int intel_kick_signalers(struct drm_i915_private *i915)
> -{
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -	unsigned int mask = 0;
> +		if (b->first_wait) {
> +			wake_up_process(b->first_wait->tsk);
> +			mask |= intel_engine_flag(engine);
> +		}
>
> -	for_each_engine(engine, i915, id) {
> -		if (unlikely(READ_ONCE(engine->breadcrumbs.first_signal))) {
> -			wake_up_process(engine->breadcrumbs.signaler);
> +		if (b->first_signal) {
> +			wake_up_process(b->signaler);
>  			mask |= intel_engine_flag(engine);
>  		}
> +
> +		spin_unlock_irq(&b->lock);
>  	}
>
>  	return mask;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index cbc148863a03..3466b4e77e7c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -587,7 +587,6 @@ static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
>
>  void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
> -unsigned int intel_kick_waiters(struct drm_i915_private *i915);
> -unsigned int intel_kick_signalers(struct drm_i915_private *i915);
> +unsigned int intel_breadcrumbs_busy(struct drm_i915_private *i915);
>
>  #endif /* _INTEL_RINGBUFFER_H_ */
>

Looks OK. Side note to myself - catch up on the rcu waiter business.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko