[Intel-gfx] [PATCH] drm/i915/execlists: Drop clear_gtiir() on GPU reset

Michel Thierry michel.thierry at intel.com
Fri Jul 13 14:33:05 UTC 2018


On 7/13/2018 1:38 AM, Chris Wilson wrote:
> With the new CSB processing code, we are not vulnerable to delayed
> delivery of a pre-reset interrupt as we use the CSB status pointers in
> the HWSP to decide if we need to parse any CSB events and no longer need
> to wait for the first post-reset interrupt to be assured that the CSB
> mmio registers are valid.
> 
> The new icl code to clear registers has a nasty lock inversion:
> [   57.409776] ======================================================
> [   57.409779] WARNING: possible circular locking dependency detected
> [   57.409783] 4.18.0-rc4-CI-CI_DII_1137+ #1 Tainted: G     U  W
> [   57.409785] ------------------------------------------------------
> [   57.409788] swapper/6/0 is trying to acquire lock:
> [   57.409790] 000000004f304ee5 (&engine->timeline.lock/1){-.-.}, at: execlists_submit_request+0x2b/0x1a0 [i915]
> [   57.409841]
>                 but task is already holding lock:
> [   57.409844] 00000000aad89594 (&(&rq->lock)->rlock#2){-.-.}, at: notify_ring+0x2b2/0x480 [i915]
> [   57.409869]
>                 which lock already depends on the new lock.
> 
> [   57.409872]
>                 the existing dependency chain (in reverse order) is:
> [   57.409876]
>                 -> #2 (&(&rq->lock)->rlock#2){-.-.}:
> [   57.409900]        notify_ring+0x2b2/0x480 [i915]
> [   57.409922]        gen8_cs_irq_handler+0x39/0xa0 [i915]
> [   57.409943]        gen11_irq_handler+0x2f0/0x420 [i915]
> [   57.409949]        __handle_irq_event_percpu+0x42/0x370
> [   57.409952]        handle_irq_event_percpu+0x2b/0x70
> [   57.409956]        handle_irq_event+0x2f/0x50
> [   57.409959]        handle_edge_irq+0xe7/0x190
> [   57.409964]        handle_irq+0x67/0x160
> [   57.409967]        do_IRQ+0x5e/0x120
> [   57.409971]        ret_from_intr+0x0/0x1d
> [   57.409974]        _raw_spin_unlock_irqrestore+0x4e/0x60
> [   57.409979]        tasklet_action_common.isra.5+0x47/0xb0
> [   57.409982]        __do_softirq+0xd9/0x505
> [   57.409985]        irq_exit+0xa9/0xc0
> [   57.409988]        do_IRQ+0x9a/0x120
> [   57.409991]        ret_from_intr+0x0/0x1d
> [   57.409995]        cpuidle_enter_state+0xac/0x360
> [   57.409999]        do_idle+0x1f3/0x250
> [   57.410004]        cpu_startup_entry+0x6a/0x70
> [   57.410010]        start_secondary+0x19d/0x1f0
> [   57.410015]        secondary_startup_64+0xa5/0xb0
> [   57.410018]
>                 -> #1 (&(&dev_priv->irq_lock)->rlock){-.-.}:
> [   57.410081]        clear_gtiir+0x30/0x200 [i915]
> [   57.410116]        execlists_reset+0x6e/0x2b0 [i915]
> [   57.410140]        i915_reset_engine+0x111/0x190 [i915]
> [   57.410165]        i915_handle_error+0x11a/0x4a0 [i915]
> [   57.410198]        i915_hangcheck_elapsed+0x378/0x530 [i915]
> [   57.410204]        process_one_work+0x248/0x6c0
> [   57.410207]        worker_thread+0x37/0x380
> [   57.410211]        kthread+0x119/0x130
> [   57.410215]        ret_from_fork+0x3a/0x50
> [   57.410217]
>                 -> #0 (&engine->timeline.lock/1){-.-.}:
> [   57.410224]        _raw_spin_lock_irqsave+0x33/0x50
> [   57.410256]        execlists_submit_request+0x2b/0x1a0 [i915]
> [   57.410289]        submit_notify+0x8d/0x124 [i915]
> [   57.410314]        __i915_sw_fence_complete+0x81/0x250 [i915]
> [   57.410339]        dma_i915_sw_fence_wake+0xd/0x20 [i915]
> [   57.410344]        dma_fence_signal_locked+0x79/0x200
> [   57.410368]        notify_ring+0x2ba/0x480 [i915]
> [   57.410392]        gen8_cs_irq_handler+0x39/0xa0 [i915]
> [   57.410416]        gen11_irq_handler+0x2f0/0x420 [i915]
> [   57.410421]        __handle_irq_event_percpu+0x42/0x370
> [   57.410425]        handle_irq_event_percpu+0x2b/0x70
> [   57.410428]        handle_irq_event+0x2f/0x50
> [   57.410432]        handle_edge_irq+0xe7/0x190
> [   57.410436]        handle_irq+0x67/0x160
> [   57.410439]        do_IRQ+0x5e/0x120
> [   57.410445]        ret_from_intr+0x0/0x1d
> [   57.410449]        cpuidle_enter_state+0xac/0x360
> [   57.410453]        do_idle+0x1f3/0x250
> [   57.410456]        cpu_startup_entry+0x6a/0x70
> [   57.410460]        start_secondary+0x19d/0x1f0
> [   57.410464]        secondary_startup_64+0xa5/0xb0
> [   57.410466]
>                 other info that might help us debug this:
> 
> [   57.410471] Chain exists of:
>                   &engine->timeline.lock/1 --> &(&dev_priv->irq_lock)->rlock --> &(&rq->lock)->rlock#2
> 
> [   57.410481]  Possible unsafe locking scenario:
> 
> [   57.410485]        CPU0                    CPU1
> [   57.410487]        ----                    ----
> [   57.410490]   lock(&(&rq->lock)->rlock#2);
> [   57.410494]                                lock(&(&dev_priv->irq_lock)->rlock);
> [   57.410498]                                lock(&(&rq->lock)->rlock#2);
> [   57.410503]   lock(&engine->timeline.lock/1);
> [   57.410506]
>                  *** DEADLOCK ***
> 
> [   57.410511] 4 locks held by swapper/6/0:
> [   57.410514]  #0: 0000000074575789 (&(&dev_priv->irq_lock)->rlock){-.-.}, at: gen11_irq_handler+0x8a/0x420 [i915]
> [   57.410542]  #1: 000000009b29b30e (rcu_read_lock){....}, at: notify_ring+0x1a/0x480 [i915]
> [   57.410573]  #2: 00000000aad89594 (&(&rq->lock)->rlock#2){-.-.}, at: notify_ring+0x2b2/0x480 [i915]
> [   57.410601]  #3: 000000009b29b30e (rcu_read_lock){....}, at: submit_notify+0x35/0x124 [i915]
> [   57.410635]
>                 stack backtrace:
> [   57.410640] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G     U  W         4.18.0-rc4-CI-CI_DII_1137+ #1
> [   57.410644] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.2222.A01.1805300339 05/30/2018
> [   57.410650] Call Trace:
> [   57.410652]  <IRQ>
> [   57.410657]  dump_stack+0x67/0x9b
> [   57.410662]  print_circular_bug.isra.16+0x1c8/0x2b0
> [   57.410666]  __lock_acquire+0x1897/0x1b50
> [   57.410671]  ? lock_acquire+0xa6/0x210
> [   57.410674]  lock_acquire+0xa6/0x210
> [   57.410706]  ? execlists_submit_request+0x2b/0x1a0 [i915]
> [   57.410711]  _raw_spin_lock_irqsave+0x33/0x50
> [   57.410741]  ? execlists_submit_request+0x2b/0x1a0 [i915]
> [   57.410769]  execlists_submit_request+0x2b/0x1a0 [i915]
> [   57.410774]  ? _raw_spin_unlock_irqrestore+0x39/0x60
> [   57.410804]  submit_notify+0x8d/0x124 [i915]
> [   57.410828]  __i915_sw_fence_complete+0x81/0x250 [i915]
> [   57.410854]  dma_i915_sw_fence_wake+0xd/0x20 [i915]
> [   57.410858]  dma_fence_signal_locked+0x79/0x200
> [   57.410882]  notify_ring+0x2ba/0x480 [i915]
> [   57.410907]  gen8_cs_irq_handler+0x39/0xa0 [i915]
> [   57.410933]  gen11_irq_handler+0x2f0/0x420 [i915]
> [   57.410938]  __handle_irq_event_percpu+0x42/0x370
> [   57.410943]  handle_irq_event_percpu+0x2b/0x70
> [   57.410947]  handle_irq_event+0x2f/0x50
> [   57.410951]  handle_edge_irq+0xe7/0x190
> [   57.410955]  handle_irq+0x67/0x160
> [   57.410958]  do_IRQ+0x5e/0x120
> [   57.410962]  common_interrupt+0xf/0xf
> [   57.410965]  </IRQ>
> [   57.410969] RIP: 0010:cpuidle_enter_state+0xac/0x360
> [   57.410972] Code: 44 00 00 31 ff e8 84 93 91 ff 45 84 f6 74 12 9c 58 f6 c4 02 0f 85 31 02 00 00 31 ff e8 7d 30 98 ff e8 e8 0e 94 ff fb 4c 29 fb <48> ba cf f7 53 e3 a5 9b c4 20 48 89 d8 48 c1 fb 3f 48 f7 ea b8 ff
> [   57.411015] RSP: 0018:ffffc90000133e90 EFLAGS: 00000216 ORIG_RAX: ffffffffffffffdd
> [   57.411023] RAX: ffff8804ae748040 RBX: 000000000002a97d RCX: 0000000000000000
> [   57.411029] RDX: 0000000000000046 RSI: ffffffff82141263 RDI: ffffffff820f05a7
> [   57.411035] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
> [   57.411041] R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff8229f078
> [   57.411045] R13: ffff8804ab2adfa8 R14: 0000000000000000 R15: 0000000d5de092e3
> [   57.411052]  do_idle+0x1f3/0x250
> [   57.411055]  cpu_startup_entry+0x6a/0x70
> [   57.411059]  start_secondary+0x19d/0x1f0
> [   57.411064]  secondary_startup_64+0xa5/0xb0
> 
> The easiest remedy is to remove the defunct code.
> 
> Fixes: ff047a87cfac ("drm/i915/icl: Correctly clear lost ctx-switch interrupts across reset for Gen11")
> References: fd8526e50902 ("drm/i915/execlists: Trust the CSB")

If I read "drm/i915/execlists: Trust the CSB" correctly, reset_irq is 
indeed no longer needed as you say.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

Reviewed-by: Michel Thierry <michel.thierry at intel.com>

> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 68 --------------------------------
>   1 file changed, 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 35d37af0cb9a..8fd8de71c2b5 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -795,72 +795,6 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
>   	execlists_user_end(execlists);
>   }
>   
> -static void clear_gtiir(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -	int i;
> -
> -	/*
> -	 * Clear any pending interrupt state.
> -	 *
> -	 * We do it twice out of paranoia that some of the IIR are
> -	 * double buffered, and so if we only reset it once there may
> -	 * still be an interrupt pending.
> -	 */
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		static const struct {
> -			u8 bank;
> -			u8 bit;
> -		} gen11_gtiir[] = {
> -			[RCS] = {0, GEN11_RCS0},
> -			[BCS] = {0, GEN11_BCS},
> -			[_VCS(0)] = {1, GEN11_VCS(0)},
> -			[_VCS(1)] = {1, GEN11_VCS(1)},
> -			[_VCS(2)] = {1, GEN11_VCS(2)},
> -			[_VCS(3)] = {1, GEN11_VCS(3)},
> -			[_VECS(0)] = {1, GEN11_VECS(0)},
> -			[_VECS(1)] = {1, GEN11_VECS(1)},
> -		};
> -		unsigned long irqflags;
> -
> -		GEM_BUG_ON(engine->id >= ARRAY_SIZE(gen11_gtiir));
> -
> -		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -		for (i = 0; i < 2; i++) {
> -			gen11_reset_one_iir(dev_priv,
> -					    gen11_gtiir[engine->id].bank,
> -					    gen11_gtiir[engine->id].bit);
> -		}
> -		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> -	} else {
> -		static const u8 gtiir[] = {
> -			[RCS]  = 0,
> -			[BCS]  = 0,
> -			[VCS]  = 1,
> -			[VCS2] = 1,
> -			[VECS] = 3,
> -		};
> -
> -		GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir));
> -
> -		for (i = 0; i < 2; i++) {
> -			I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]),
> -				   engine->irq_keep_mask);
> -			POSTING_READ(GEN8_GT_IIR(gtiir[engine->id]));
> -		}
> -		GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) &
> -			   engine->irq_keep_mask);
> -	}
> -}
> -
> -static void reset_irq(struct intel_engine_cs *engine)
> -{
> -	/* Mark all CS interrupts as complete */
> -	smp_store_mb(engine->execlists.active, 0);
> -
> -	clear_gtiir(engine);
> -}
> -
>   static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>   {
>   	/*
> @@ -904,7 +838,6 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   
>   	/* Cancel the requests on the HW and clear the ELSP tracker. */
>   	execlists_cancel_port_requests(execlists);
> -	reset_irq(engine);
>   
>   	/* Mark all executing requests as skipped. */
>   	list_for_each_entry(rq, &engine->timeline.requests, link) {
> @@ -1975,7 +1908,6 @@ static void execlists_reset(struct intel_engine_cs *engine,
>   	 * requests were completed.
>   	 */
>   	execlists_cancel_port_requests(execlists);
> -	reset_irq(engine);
>   
>   	/* Push back any incomplete requests for replay after the reset. */
>   	__unwind_incomplete_requests(engine);
> 


More information about the Intel-gfx mailing list