Re: [Intel-gfx] [PATCH] drm/i915: Exit cherryview_irq_handler() after one pass

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/03/16 11:44, Chris Wilson wrote:
> This effectively reverts
>
> commit 8e5fd599eb219f1054e39b40d18b217af669eea9
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Apr 9 13:28:50 2014 +0300
>
>      drm/i915/chv: Make CHV irq handler loop until all interrupts are consumed
>
> as under continuous execlists load we can saturate the IRQ handler,
> destablising the tsc clock and triggering the NMI watchdog to declare a hung
> CPU.
>
> [  552.756051] clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
> [  552.756080] clocksource:                       'refined-jiffies' wd_now: 10003b480 wd_last: 10003b28c mask: ffffffff
> [  552.756091] clocksource:                       'tsc' cs_now: d55d31aa50 cs_last: d17446166c mask: ffffffffffffffff
> [  552.756210] clocksource: Switched to clocksource refined-jiffies
> [  575.217870] NMI watchdog: Watchdog detected hard LOCKUP on cpu 1
> [  575.217893] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.5.0-rc7+ #18
> [  575.217905] Hardware name:                  /NUC5CPYB, BIOS PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
> [  575.217915]  0000000000000000 ffff88027fd05bc0 ffffffff81288c6d 0000000000000000
> [  575.217935]  0000000000000001 ffff88027fd05be0 ffffffff810e72d1 0000000000000000
> [  575.217951]  ffff88027fd05c80 ffff88027fd05c20 ffffffff81114b60 0000000181015f1e
> [  575.217967] Call Trace:
> [  575.217973]  <NMI>  [<ffffffff81288c6d>] dump_stack+0x4f/0x72
> [  575.217994]  [<ffffffff810e72d1>] watchdog_overflow_callback+0x151/0x160
> [  575.218003]  [<ffffffff81114b60>] __perf_event_overflow+0xa0/0x1e0
> [  575.218016]  [<ffffffff811154c4>] perf_event_overflow+0x14/0x20
> [  575.218028]  [<ffffffff8101d2ca>] intel_pmu_handle_irq+0x1da/0x460
> [  575.218042]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218052]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218064]  [<ffffffff81014ae8>] perf_event_nmi_handler+0x28/0x50
> [  575.218075]  [<ffffffff81007540>] nmi_handle+0x60/0x130
> [  575.218086]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218096]  [<ffffffff810079c0>] do_nmi+0x140/0x470
> [  575.218108]  [<ffffffff81559ec7>] end_repeat_nmi+0x1a/0x1e
> [  575.218119]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218129]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218139]  [<ffffffff814a8aae>] ? poll_idle+0x3e/0x70
> [  575.218148]  <<EOE>>  [<ffffffff814a8353>] cpuidle_enter_state+0xf3/0x2f0
> [  575.218164]  [<ffffffff814a8587>] cpuidle_enter+0x17/0x20
> [  575.218175]  [<ffffffff810aaa3a>] call_cpuidle+0x2a/0x40
> [  575.218185]  [<ffffffff810aade3>] cpu_startup_entry+0x273/0x330
> [  575.218196]  [<ffffffff81033a1e>] start_secondary+0x10e/0x130
>
> However, not servicing all available IIR within the handler does hurt the
> throughput of pathological nop execbuf by about 20%, with a similar effect
> upon the dispatch latency of a series of execbuf.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93467
> Testcase: igt/gem_exec_nop/basic # requires NMI watchdog
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Antti Koskipää <antti.koskipaa@linux.intel.com
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++++---------------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 53e5104964b3..8a3230427884 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1829,35 +1829,33 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>   	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
>   	disable_rpm_wakeref_asserts(dev_priv);
>
> -	for (;;) {
> -		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> -		iir = I915_READ(VLV_IIR);
> +	master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> +	iir = I915_READ(VLV_IIR);
>
> -		if (master_ctl == 0 && iir == 0)
> -			break;
> +	if (master_ctl == 0 && iir == 0)
> +		break;

return ret; ?

>
> -		ret = IRQ_HANDLED;
> +	ret = IRQ_HANDLED;
>
> -		I915_WRITE(GEN8_MASTER_IRQ, 0);
> +	I915_WRITE(GEN8_MASTER_IRQ, 0);
>
> -		/* Find, clear, then process each source of interrupt */
> +	/* Find, clear, then process each source of interrupt */
>
> -		if (iir) {
> -			/* Consume port before clearing IIR or we'll miss events */
> -			if (iir & I915_DISPLAY_PORT_INTERRUPT)
> -				i9xx_hpd_irq_handler(dev);
> -			I915_WRITE(VLV_IIR, iir);
> -		}
> +	if (iir) {
> +		/* Consume port before clearing IIR or we'll miss events */
> +		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> +			i9xx_hpd_irq_handler(dev);
> +		I915_WRITE(VLV_IIR, iir);
> +	}
>
> -		gen8_gt_irq_handler(dev_priv, master_ctl);
> +	gen8_gt_irq_handler(dev_priv, master_ctl);
>
> -		/* Call regardless, as some status bits might not be
> -		 * signalled in iir */
> -		valleyview_pipestat_irq_handler(dev, iir);
> +	/* Call regardless, as some status bits might not be
> +	 * signalled in iir */
> +	valleyview_pipestat_irq_handler(dev, iir);
>
> -		I915_WRITE(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL);
> -		POSTING_READ(GEN8_MASTER_IRQ);
> -	}
> +	I915_WRITE(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL);
> +	POSTING_READ(GEN8_MASTER_IRQ);
>
>   	enable_rpm_wakeref_asserts(dev_priv);
>
>

Ack on this from me since it looks obviously immensely dangerous to loop 
like that. I can't test it unfortunately.

Regards,

Tvrtko