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

[Tvrtko Ursulin]

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>

Regards,

Tvrtko