Re: [PATCH v2 1/7] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Re-enabling interrupts before `execlists_submit_ports()` violates the hardware ELSP submission constraint and causes premature `irq_work` flushing, breaking cross-engine request execution ordering.
--

commit 2338d7dc7111f08419fa617c233f6984399c8937
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

This patch modifies execlists_dequeue() to use spin_lock_irq() and 
spin_unlock_irq() directly. It removes execlists_dequeue_irq() which 
previously wrapped the entire dequeue operation in local_irq_disable().

> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 1359fc9cb88ef..e11db81dca9c5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c

[ ... ]

> @@ -1588,7 +1588,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	 */
>  	sched_engine->queue_priority_hint = queue_prio(sched_engine);
>  	i915_sched_engine_reset_on_empty(sched_engine);
> -	spin_unlock(&sched_engine->lock);
> +	spin_unlock_irq(&sched_engine->lock);

[Severity: High]
Does re-enabling interrupts here before calling execlists_submit_ports() 
violate the hardware ELSP submission constraints?

The original execlists_dequeue_irq() kept interrupts disabled across the
entire dequeue process, including the final MMIO writes. By changing to
spin_unlock_irq() here, interrupts are re-enabled before the execution of:

execlists_submit_ports() {
    ...
    for (n = execlists_num_ports(execlists); n--; ) {
        struct i915_request *rq = execlists->pending[n];

        write_desc(execlists,
                   rq ? execlists_update_context(rq) : 0,
                   n);
    }
}

Hardware requires these consecutive ELSP MMIO writes to be atomic without
intervening accesses. If an interrupt fires during this loop, could it
corrupt the submission sequence and cause a GPU hang? 

Additionally, can enabling interrupts this early cause premature irq_work
flushing, breaking cross-engine request execution ordering?

>  
>  	/*
>  	 * We can skip poking the HW if we ended up with exactly the same set

[ ... ]

> -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) */
> -}
> -

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703113259.801374-1-dev@lankhorst.se?part=1