Re: [PATCH 13/31] drm/i915: Move the irq_counter inside the spinlock

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Rather than have multiple locked instructions inside the notify_ring()
> irq handler, move them inside the spinlock and reduce their intrinsic
> locking.
>

Less is better, could note in commit message that we omit the non
wait ones.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c          |  4 ++--
>  drivers/gpu/drm/i915/i915_request.c      |  4 ++--
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 11 +++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>  4 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 53dad48f92ce..6730c1a7f135 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1153,8 +1153,6 @@ static void notify_ring(struct intel_engine_cs *engine)
>  	if (unlikely(!engine->breadcrumbs.irq_armed))
>  		return;
>  
> -	atomic_inc(&engine->irq_count);
> -
>  	rcu_read_lock();
>  
>  	spin_lock(&engine->breadcrumbs.irq_lock);
> @@ -1189,6 +1187,8 @@ static void notify_ring(struct intel_engine_cs *engine)
>  				tsk = wait->tsk;
>  			}
>  		}
> +
> +		engine->breadcrumbs.irq_count++;
>  	} else {
>  		if (engine->breadcrumbs.irq_armed)
>  			__intel_engine_disarm_breadcrumbs(engine);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 11f175554da8..696125663105 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1161,7 +1161,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
>  	 * takes to sleep on a request, on the order of a microsecond.
>  	 */
>  
> -	irq = atomic_read(&engine->irq_count);
> +	irq = READ_ONCE(engine->breadcrumbs.irq_count);
>  	timeout_us += local_clock_us(&cpu);
>  	do {
>  		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
> @@ -1173,7 +1173,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
>  		 * assume we won't see one in the near future but require
>  		 * the engine->seqno_barrier() to fixup coherency.
>  		 */
> -		if (atomic_read(&engine->irq_count) != irq)
> +		if (READ_ONCE(engine->breadcrumbs.irq_count) != irq)
>  			break;
>  
>  		if (signal_pending_state(state, current))
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 86a987b8ac66..1db6ba7d926e 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -98,12 +98,14 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
>  	struct intel_engine_cs *engine =
>  		from_timer(engine, t, breadcrumbs.hangcheck);
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +	unsigned int irq_count;
>  
>  	if (!b->irq_armed)
>  		return;
>  
> -	if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
> -		b->hangcheck_interrupts = atomic_read(&engine->irq_count);
> +	irq_count = READ_ONCE(b->irq_count);
> +	if (b->hangcheck_interrupts != irq_count) {
> +		b->hangcheck_interrupts = irq_count;
>  		mod_timer(&b->hangcheck, wait_timeout());
>  		return;
>  	}
> @@ -272,13 +274,14 @@ static bool use_fake_irq(const struct intel_breadcrumbs *b)
>  	if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
>  		return false;
>  
> -	/* Only start with the heavy weight fake irq timer if we have not
> +	/*
> +	 * Only start with the heavy weight fake irq timer if we have not
>  	 * seen any interrupts since enabling it the first time. If the
>  	 * interrupts are still arriving, it means we made a mistake in our
>  	 * engine->seqno_barrier(), a timing error that should be transient
>  	 * and unlikely to reoccur.
>  	 */
> -	return atomic_read(&engine->irq_count) == b->hangcheck_interrupts;
> +	return READ_ONCE(b->irq_count) == b->hangcheck_interrupts;
>  }
>  
>  static void enable_fake_irq(struct intel_breadcrumbs *b)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8dd34b9dc18a..33602eb1c77f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -356,7 +356,6 @@ struct intel_engine_cs {
>  	struct drm_i915_gem_object *default_state;
>  	void *pinned_default_state;
>  
> -	atomic_t irq_count;
>  	unsigned long irq_posted;
>  #define ENGINE_IRQ_BREADCRUMB 0
>  
> @@ -390,6 +389,7 @@ struct intel_engine_cs {
>  
>  		unsigned int hangcheck_interrupts;
>  		unsigned int irq_enabled;
> +		unsigned int irq_count;
>  
>  		bool irq_armed : 1;
>  		I915_SELFTEST_DECLARE(bool mock : 1);
> -- 
> 2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx