[Intel-gfx] [PATCH 01/19] drm/i915/execlists: Always clear ring_pause if we do not submit

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> In the unlikely case (thank you CI!), we may find ourselves wanting to
> issue a preemption but having no runnable requests left. In this case,
> we set the semaphore before computing the preemption and so must unset
> it before forgetting (or else we leave the machine busywaiting until the
> next request comes along and so likely hang).
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index c8a0c9b32764..efccc31887de 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -233,13 +233,18 @@ static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>  static inline void
>  ring_set_paused(const struct intel_engine_cs *engine, int state)
>  {
> +	u32 *sema = &engine->status_page.addr[I915_GEM_HWS_PREEMPT];
> +
> +	if (*sema == state)
> +		return;
> +

So you want to avoid useless wmb, as I don't see other
benefit. Makes this look suspiciously racy but seems
to be just my usual paranoia.


>  	/*
>  	 * We inspect HWS_PREEMPT with a semaphore inside
>  	 * engine->emit_fini_breadcrumb. If the dword is true,
>  	 * the ring is paused as the semaphore will busywait
>  	 * until the dword is false.
>  	 */
> -	engine->status_page.addr[I915_GEM_HWS_PREEMPT] = state;
> +	*sema = state;
>  	wmb();
>  }
>  
> @@ -1243,6 +1248,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  		*port = execlists_schedule_in(last, port - execlists->pending);
>  		memset(port + 1, 0, (last_port - port) * sizeof(*port));
>  		execlists_submit_ports(engine);
> +	} else {
> +		ring_set_paused(engine, 0);

This looks like a right thing to do. But why did we end up
figuring things out wrong in need_preempt()?

One would think that if there were nothing to preempt into,
we would never set the pause in the first place.

Also the preempt to idle cycle mention in effective_prio()
seems to be off. Could be that someone forgot to
point that out when he did review preempt-to-busy.

-Mika

>  	}
>  }
>  
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx