[Intel-gfx] [PATCH] drm/i915/execlists: Split the atomic test_and_clear_bit for irq handler

[Chris Wilson]

On Tue, Mar 21, 2017 at 02:09:43PM +0000, Tvrtko Ursulin wrote:
> 
> On 21/03/2017 11:33, Chris Wilson wrote:
> >Rather than impose the cost of a locked test before queuing a new
> >request, reduce it to a simple test_bit() with a following clear_bit()
> >prior to doing the CSB check. This ensure that if an interrupt does
> >occur whilst reading from the CSB, we still detect it (the interrupt
> >would trigger a rescheduling of the tasklet anyway).
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 296d125d8665..3154b98dc971 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -530,13 +530,18 @@ static void intel_lrc_irq_handler(unsigned long data)
> >
> > 	intel_uncore_forcewake_get(dev_priv, engine->fw_domains);
> >
> >-	while (test_and_clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
> >+	/* Prefer doing test_and_clear_bit() as a two stage operation to avoid
> >+	 * imposing the cost of a locked atomic transaction when submitting a
> >+	 * new request (outside of the context-switch interrupt).
> >+	 */
> >+	while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
> > 		u32 __iomem *csb_mmio =
> > 			dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine));
> > 		u32 __iomem *buf =
> > 			dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0));
> > 		unsigned int csb, head, tail;
> >
> >+		clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> > 		csb = readl(csb_mmio);
> > 		head = GEN8_CSB_READ_PTR(csb);
> > 		tail = GEN8_CSB_WRITE_PTR(csb);
> >
> 
> Looks safe to me from the point of view of potential races. If a new
> interrupt arrives and sets the bit just before the tasklet clears
> it, we would process the full set of CSB updates on the following
> line.
> 
> I can also confirm that it has a real effect of bringing the CPU
> usage of this interrupt handler down.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Thanks, pushed because CI is getting bored (or I'm getting bored of it
ignoring some patches ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre