[Intel-gfx] [PATCH v4] drm/i915/execlists: Pull CSB reset under the timeline.lock

[Tvrtko Ursulin]

On 26/06/2018 12:50, Chris Wilson wrote:
> In the following patch, we will process the CSB events under the
> timeline.lock and not serialised by the tasklet. This also means that we
> will need to protect access to common variables such as
> execlists->csb_head with the timeline.lock during reset.
> 
> v2: Move sync_irq to avoid deadlocks between taking timeline.lock from
> our interrupt handler.
> v3: Kill off the synchronize_hardirq as it raises more questions than
> answered; now we use the timeline.lock entirely for CSB serialisation
> between the irq and elsewhere, we don't need to be so heavy handed with
> flushing
> v4: Treat request cancellation (wedging after failed reset) similarly
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 009db92b67d7..4b31e8f42aeb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -871,7 +871,6 @@ static void reset_irq(struct intel_engine_cs *engine)
>   {
>   	/* Mark all CS interrupts as complete */
>   	smp_store_mb(engine->execlists.active, 0);
> -	synchronize_hardirq(engine->i915->drm.irq);
>   
>   	clear_gtiir(engine);
>   
> @@ -908,14 +907,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   	 * submission's irq state, we also wish to remind ourselves that
>   	 * it is irq state.)
>   	 */
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&engine->timeline.lock, flags);
>   
>   	/* Cancel the requests on the HW and clear the ELSP tracker. */
>   	execlists_cancel_port_requests(execlists);
>   	reset_irq(engine);
>   
> -	spin_lock(&engine->timeline.lock);
> -
>   	/* Mark all executing requests as skipped. */
>   	list_for_each_entry(rq, &engine->timeline.requests, link) {
>   		GEM_BUG_ON(!rq->global_seqno);
> @@ -949,9 +946,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   	execlists->first = NULL;
>   	GEM_BUG_ON(port_isset(execlists->port));
>   
> -	spin_unlock(&engine->timeline.lock);
> -
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&engine->timeline.lock, flags);
>   }
>   
>   static void process_csb(struct intel_engine_cs *engine)
> @@ -1969,8 +1964,7 @@ static void execlists_reset(struct intel_engine_cs *engine,
>   		  engine->name, request ? request->global_seqno : 0,
>   		  intel_engine_get_seqno(engine));
>   
> -	/* See execlists_cancel_requests() for the irq/spinlock split. */
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&engine->timeline.lock, flags);
>   
>   	/*
>   	 * Catch up with any missed context-switch interrupts.
> @@ -1985,14 +1979,12 @@ static void execlists_reset(struct intel_engine_cs *engine,
>   	reset_irq(engine);
>   
>   	/* Push back any incomplete requests for replay after the reset. */
> -	spin_lock(&engine->timeline.lock);
>   	__unwind_incomplete_requests(engine);
> -	spin_unlock(&engine->timeline.lock);
>   
>   	/* Following the reset, we need to reload the CSB read/write pointers */
>   	engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
>   
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&engine->timeline.lock, flags);
>   
>   	/*
>   	 * If the request was innocent, we leave the request in the ELSP
> 

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

Regards,

Tvrtko