[Intel-gfx] [PATCH] drm/i915/execlists: Skip a lite-restore immediately prior to a context-completion

[Chris Wilson]

Quoting Chris Wilson (2017-11-16 22:11:30)
> If we are about to do another context-switch in the near future skip
> doing performing a lite-restore now. (Forcing a lite-restore just before
> a context-switch effectively doubles the cost of that context-switch, so
> long as we can handle the interrupt and resubmit before the GPU powers
> down, which under normal conditions is expected.)
> 
> Coincidentally, skipping a lite-restore just before idling also prevents
> (not entirely, but the window for occurrence now depends on the next
> context completing within the same period and we have already noted that
> we can resubmit faster than the GPU can context switch to make this
> worthwhile) an issue on bxt/glk where it may become confused in an attempt
> to lite-restore an already idle context, after which nothing is quite the
> same again.

Well only testing will tell...

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5a22bdffd387..5272eb23d98a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -584,6 +584,25 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>                         if (port_count(&port[1]))
>                                 goto unlock;
>  
> +                       /*
> +                        * If we are about to do another context-switch in
> +                        * the near future skip doing performing a lite-restore
> +                        * now. (Forcing a lite-restore just before a
> +                        * context-switch effectively doubles the cost of that
> +                        * context-switch, so long as we can handle the
> +                        * interrupt and resubmit before the GPU powers down,
> +                        * which under normal conditions is expected.)
> +                        *
> +                        * Skipping a lite-restore just before idling also
> +                        * prevents an issue on bxt/glk where it may become
> +                        * confused in an attempt to lite-restore an already
> +                        * idle context, after which nothing is quite the same
> +                        * again.
> +                        */
> +                       if (i915_seqno_passed(intel_engine_get_seqno(engine),
> +                                             last->global_seqno - 1))
> +                               goto unlock;
> +
>                         /* WaIdleLiteRestore:bdw,skl
>                          * Apply the wa NOOPs to prevent
>                          * ring:HEAD == req:TAIL as we resubmit the

The subtext is that lite-restore are pointless... Not sure I fully agree
yet; in most scenarios pointless. The last time I was thinking of only
doing the lite-restore iff we were on the last request, otherwise we are
just interrupting a request which already has a continuation.
-Chris