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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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@xxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
