Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

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

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

> Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> context-switch when idle") we noticed the presence of late
> context-switch interrupts. We were able to filter those out by looking
> at whether the ELSP remained active, but in commit beecec901790
> ("drm/i915/execlists: Preemption!") that became problematic as we now
> anticipate receiving a context-switch event for preemption while ELSP
> may be empty. To restore the spurious interrupt suppression, add a

This confuses me, how can we preempt something that was never submitted.
Could you elaborate?

Thanks,
-Mika

> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.
>
> Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
>  5 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114b739d..c22d0a07467c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
>  	GEM_BUG_ON(rq == port_request(port));
> +	GEM_BUG_ON(port_isset(port));
>  
> -	if (port_isset(port))
> -		i915_gem_request_put(port_request(port));
> -
> -	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +	port_set(port, i915_gem_request_get(rq));
>  	nested_enable_signaling(rq);
>  }
>  
> @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  					goto done;
>  				}
>  
> -				if (submit)
> +				if (submit) {
>  					port_assign(port, last);
> +					execlists->active++;
> +				}
>  				port++;
>  			}
>  
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  	execlists->first = rb;
>  	if (submit) {
>  		port_assign(port, last);
> +		execlists->active++;
>  		i915_guc_submit(engine);
>  	}
>  	spin_unlock_irq(&engine->timeline->lock);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a55c1e4..f8205841868b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
>  	bool tasklet = false;
>  
>  	if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
> -		__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> -		tasklet = true;
> +		if (READ_ONCE(engine->execlists.active)) {
> +			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> +			tasklet = true;
> +		}
>  	}
>  
>  	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a47a9c6bea52..8aecbc321786 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
>  	if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
>  		return false;
>  
> -	/* Both ports drained, no more ELSP submission? */
> -	if (port_request(&engine->execlists.port[0]))
> +	/* Waiting to drain ELSP? */
> +	if (READ_ONCE(engine->execlists.active))
>  		return false;
>  
>  	/* ELSP is empty, but there are ready requests? */
> @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
>  					   idx);
>  			}
>  		}
> +		drm_printf(m, "\t\tELSP active %d\n", execlists->active);
>  		rcu_read_unlock();
>  	} else if (INTEL_GEN(dev_priv) > 6) {
>  		drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 7f45dd7dc3e5..8a47b8211c74 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev,
>  	return true;
>  }
>  
> -static void port_assign(struct execlist_port *port,
> +static bool port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
> +	bool was_idle;
> +
>  	GEM_BUG_ON(rq == port_request(port));
>  
> -	if (port_isset(port))
> +	if (port_isset(port)) {
>  		i915_gem_request_put(port_request(port));
> +		was_idle = false;
> +	} else {
> +		was_idle = true;
> +	}
>  
>  	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +
> +	return was_idle;
>  }
>  
>  static void inject_preempt_context(struct intel_engine_cs *engine)
> @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  				GEM_BUG_ON(last->ctx == rq->ctx);
>  
>  				if (submit)
> -					port_assign(port, last);
> +					execlists->active += port_assign(port, last);
>  				port++;
>  
>  				GEM_BUG_ON(port_isset(port));
> @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  done:
>  	execlists->first = rb;
>  	if (submit)
> -		port_assign(port, last);
> +		execlists->active += port_assign(port, last);
>  unlock:
>  	spin_unlock_irq(&engine->timeline->lock);
>  
> @@ -696,10 +704,12 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
>  	while (num_ports-- && port_isset(port)) {
>  		struct drm_i915_gem_request *rq = port_request(port);
>  
> +		GEM_BUG_ON(!execlists->active);
>  		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
>  		i915_gem_request_put(rq);
>  
>  		memset(port, 0, sizeof(*port));
> +		execlists->active--;
>  		port++;
>  	}
>  }
> @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data)
>  
>  				GEM_BUG_ON(!execlists->preempt);
>  				execlists->preempt = false;
> +				execlists->active--;
> +				GEM_BUG_ON(execlists->active);
>  				continue;
>  			}
>  
> @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>  	execlists->csb_head = -1;
>  	execlists->preempt = false;
> +	execlists->active = 0;
>  
>  	/* After a GPU reset, we may have requests to replay */
>  	if (!i915_modparams.enable_guc_submission && execlists->first)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 17186f067408..588f5fe9d2b7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -245,6 +245,11 @@ struct intel_engine_execlists {
>  	 */
>  	bool preempt;
>  
> +	/**
> +	 * @active: count of the number of outstanding CSB events
> +	 */
> +	unsigned int active;
> +
>  	/**
>  	 * @port_mask: number of execlist ports - 1
>  	 */
> @@ -538,9 +543,11 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
>  	const unsigned int m = execlists->port_mask;
>  
>  	GEM_BUG_ON(port_index(port, execlists) != 0);
> +	GEM_BUG_ON(!execlists->active);
>  
>  	memmove(port, port + 1, m * sizeof(struct execlist_port));
>  	memset(port + m, 0, sizeof(struct execlist_port));
> +	execlists->active--;
>  }
>  
>  static inline unsigned int
> -- 
> 2.15.0.rc1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx