Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 08-09-2021 om 20:57 schreef Sebastian Andrzej Siewior:
> execlists_dequeue() is invoked from a function which uses
> local_irq_disable() to disable interrupts so the spin_lock() behaves
> like spin_lock_irq().
> This breaks PREEMPT_RT because local_irq_disable() + spin_lock() is not
> the same as spin_lock_irq().
>
> execlists_dequeue_irq() and execlists_dequeue() has each one caller
> only. If intel_engine_cs::active::lock is acquired and released with the
> _irq suffix then it behaves almost as if execlists_dequeue() would be
> invoked with disabled interrupts. The difference is the last part of the
> function which is then invoked with enabled interrupts.
> I can't tell if this makes a difference. From looking at it, it might
> work to move the last unlock at the end of the function as I didn't find
> anything that would acquire the lock again.
>
> Reported-by: Clark Williams <williams@redhat.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  .../drm/i915/gt/intel_execlists_submission.c    | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index fc77592d88a96..2ec1dd352960b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -1265,7 +1265,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	 * and context switches) submission.
>  	 */
>  
> -	spin_lock(&engine->active.lock);
> +	spin_lock_irq(&engine->active.lock);
>  
>  	/*
>  	 * If the queue is higher priority than the last
> @@ -1365,7 +1365,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  				 * Even if ELSP[1] is occupied and not worthy
>  				 * of timeslices, our queue might be.
>  				 */
> -				spin_unlock(&engine->active.lock);
> +				spin_unlock_irq(&engine->active.lock);
>  				return;
>  			}
>  		}
> @@ -1391,7 +1391,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  
>  		if (last && !can_merge_rq(last, rq)) {
>  			spin_unlock(&ve->base.active.lock);
> -			spin_unlock(&engine->active.lock);
> +			spin_unlock_irq(&engine->active.lock);
>  			return; /* leave this for another sibling */
>  		}
>  
> @@ -1552,7 +1552,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	 * interrupt for secondary ports).
>  	 */
>  	execlists->queue_priority_hint = queue_prio(execlists);
> -	spin_unlock(&engine->active.lock);
> +	spin_unlock_irq(&engine->active.lock);
>  
>  	/*
>  	 * We can skip poking the HW if we ended up with exactly the same set
> @@ -1578,13 +1578,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	}
>  }
>  
> -static void execlists_dequeue_irq(struct intel_engine_cs *engine)
> -{
> -	local_irq_disable(); /* Suspend interrupts across request submission */
> -	execlists_dequeue(engine);
> -	local_irq_enable(); /* flush irq_work (e.g. breadcrumb enabling) */
> -}
> -
>  static void clear_ports(struct i915_request **ports, int count)
>  {
>  	memset_p((void **)ports, NULL, count);
> @@ -2377,7 +2370,7 @@ static void execlists_submission_tasklet(struct tasklet_struct *t)
>  	}
>  
>  	if (!engine->execlists.pending[0]) {
> -		execlists_dequeue_irq(engine);
> +		execlists_dequeue(engine);
>  		start_timeslice(engine);
>  	}
>  

Patches look good.

For both patches:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I've been looking at running i915 with the -rt patch series, and noticed i915_request_submit fails with GEM_BUG_ON(!irqs_disabled()); presumably same failure exists for i915_request_unsubmit().

Might be worth removing those checks as well? Seems double with lockdep_assert_held on an irq lock anyway.

I've also noticed the local_irq_disable/enable is removed from intel_pipe_update_(start/end) in the rt series. It might make sense from a -rt point of view, but that code needs to run without interruptions, or i915 may show visual glitches or even locks up the system.

It should just be a set of registers hammered in, but the code might needs to be fixed to take the mmio lock as outer lock, and become a strict set of register read/writes only.

~Maarten