From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Exit cherryview_irq_handler() after one pass Date: Thu, 10 Mar 2016 14:01:27 +0200 Message-ID: <20160310120127.GD10446@intel.com> References: <1457610268-25299-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <1457610268-25299-1-git-send-email-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Antti =?iso-8859-1?Q?Koskip=E4=E4?= , Tvrtko Ursulin , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 10, 2016 at 11:44:28AM +0000, Chris Wilson wrote: > This effectively reverts >=20 > commit 8e5fd599eb219f1054e39b40d18b217af669eea9 > Author: Ville Syrj=E4l=E4 > Date: Wed Apr 9 13:28:50 2014 +0300 >=20 > drm/i915/chv: Make CHV irq handler loop until all interrupts are = consumed >=20 > 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. >=20 > [ 552.756051] clocksource: timekeeping watchdog on CPU0: Marking clo= cksource 'tsc' as unstable because the skew is too large: > [ 552.756080] clocksource: 'refined-jiffies' w= d_now: 10003b480 wd_last: 10003b28c mask: ffffffff > [ 552.756091] clocksource: 'tsc' cs_now: d55d3= 1aa50 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 PYBSWC= EL.86A.0027.2015.0507.1758 05/07/2015 > [ 575.217915] 0000000000000000 ffff88027fd05bc0 ffffffff81288c6d 00= 00000000000000 > [ 575.217935] 0000000000000001 ffff88027fd05be0 ffffffff810e72d1 00= 00000000000000 > [ 575.217951] ffff88027fd05c80 ffff88027fd05c20 ffffffff81114b60 00= 00000181015f1e > [ 575.217967] Call Trace: > [ 575.217973] [] dump_stack+0x4f/0x72 > [ 575.217994] [] watchdog_overflow_callback+0x151= /0x160 > [ 575.218003] [] __perf_event_overflow+0xa0/0x1e0 > [ 575.218016] [] perf_event_overflow+0x14/0x20 > [ 575.218028] [] intel_pmu_handle_irq+0x1da/0x460 > [ 575.218042] [] ? poll_idle+0x3e/0x70 > [ 575.218052] [] ? poll_idle+0x3e/0x70 > [ 575.218064] [] perf_event_nmi_handler+0x28/0x50 > [ 575.218075] [] nmi_handle+0x60/0x130 > [ 575.218086] [] ? poll_idle+0x3e/0x70 > [ 575.218096] [] do_nmi+0x140/0x470 > [ 575.218108] [] end_repeat_nmi+0x1a/0x1e > [ 575.218119] [] ? poll_idle+0x3e/0x70 > [ 575.218129] [] ? poll_idle+0x3e/0x70 > [ 575.218139] [] ? poll_idle+0x3e/0x70 > [ 575.218148] <> [] cpuidle_enter_state+0xf= 3/0x2f0 > [ 575.218164] [] cpuidle_enter+0x17/0x20 > [ 575.218175] [] call_cpuidle+0x2a/0x40 > [ 575.218185] [] cpu_startup_entry+0x273/0x330 > [ 575.218196] [] start_secondary+0x10e/0x130 >=20 > However, not servicing all available IIR within the handler does hurt= the > throughput of pathological nop execbuf by about 20%, with a similar e= ffect > upon the dispatch latency of a series of execbuf. >=20 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D93467 > Testcase: igt/gem_exec_nop/basic # requires NMI watchdog > Signed-off-by: Chris Wilson > Cc: Ville Syrj=E4l=E4 > Cc: Antti Koskip=E4=E4 Cc: Tvrtko Ursulin > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++++------------= --------- > 1 file changed, 19 insertions(+), 21 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i= 915_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 waker= ef */ > disable_rpm_wakeref_asserts(dev_priv); > =20 > - for (;;) { > - master_ctl =3D I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTR= OL; > - iir =3D I915_READ(VLV_IIR); > + master_ctl =3D I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTRO= L; > + iir =3D I915_READ(VLV_IIR); > =20 > - if (master_ctl =3D=3D 0 && iir =3D=3D 0) > - break; > + if (master_ctl =3D=3D 0 && iir =3D=3D 0) > + break; goto something? Apart from that I have no objections if it doesn't cause problems with interrupts getting lost and whatnot. That was the original reason for it I think, but at least I myself never really looked into it. IIRC Rafael just told me they needed to do it to get the thing working, so I just put the patch in. And that was before I had even seen any silico= n. > =20 > - ret =3D IRQ_HANDLED; > + ret =3D IRQ_HANDLED; > =20 > - I915_WRITE(GEN8_MASTER_IRQ, 0); > + I915_WRITE(GEN8_MASTER_IRQ, 0); > =20 > - /* Find, clear, then process each source of interrupt */ > + /* Find, clear, then process each source of interrupt */ > =20 > - 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); > + } > =20 > - gen8_gt_irq_handler(dev_priv, master_ctl); > + gen8_gt_irq_handler(dev_priv, master_ctl); > =20 > - /* 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); > =20 > - 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); > =20 > enable_rpm_wakeref_asserts(dev_priv); > =20 > --=20 > 2.7.0 --=20 Ville Syrj=E4l=E4 Intel OTC