[Intel-gfx] [PATCH 1/2] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

[Joonas Lahtinen]

On Mon, 2017-11-20 at 09:09 +0000, Chris Wilson wrote:
> Since commit e1fee72c2ea2e9c0c6e6743d32a6832f21337d6c
> Author: Oscar Mateo <oscar.mateo at intel.com>

Cc:ing oscar as git didn't pick this line up.

> Date:   Thu Jul 24 17:04:40 2014 +0100
> 
>     drm/i915/bdw: Avoid non-lite-restore preemptions
> 
> execlists has listened to (ACTIVE_IDLE | ELEMENT_SWITCH) for detecting
> when one context completed and it either continued onto the next (in port
> 1) or idled. We would always see COMPLETE | ACTIVE_IDLE on the final
> context-switch event, but on recent gen it appears that we now get
> separate ACTIVE_IDLE and COMPLETE events. In particular, the ACTIVE_IDLE
> events may not be coupled to a context (since it is a general state rather
> than a specific context completion event).
> 
> v2: Update the history, execlists did originally start out by listening
> to the COMPLETE event not ACTIVE_IDLE.
> v3: Update preempt completion test to also use COMPLETE not ACTIVE_IDLE.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=103800
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Acked-by: Michel Thierry <michel.thierry at intel.com>

It's not too verbose, byt worthy adding anyway:

Bspec: 12255

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

One comment below.

> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -154,7 +154,7 @@
>  #define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
>  
>  #define GEN8_CTX_STATUS_COMPLETED_MASK \
> -	 (GEN8_CTX_STATUS_ACTIVE_IDLE | \
> +	 (GEN8_CTX_STATUS_COMPLETE | \
>  	  GEN8_CTX_STATUS_PREEMPTED | \
>  	  GEN8_CTX_STATUS_ELEMENT_SWITCH)

Not related to this patch but ELEMENT_SWITCH should probably be dropped
from here.

The context only "completes" as the driver is concerned by actually
being completed or by getting preempted. Everything else is ELSP state
changes, idle->active, port1->port2, active->idle.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation