[Intel-gfx] [PATCH 06/21] drm/i915/guc: Use guc_class instead of engine_class in fw interface

[Joonas Lahtinen]

Quoting Michal Wajdeczko (2018-08-29 22:10:40)
> Until now the GuC and HW engine class has been the same, which allowed
> us to use them interchangeable. But it is better to start doing the
> right thing and use the GuC definitions for the firmware interface.
> 
> We also keep the same class id in the ctx descriptor to be able to have
> the same values in the driver and firmware logs.
> 
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Tomasz Lis <tomasz.lis at intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
>  
>                 /* TODO: decide what to do with SW counter (bits 55-60) */
>  
> -               desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
> +               /*
> +                * Although GuC will never see this upper part as it fills
> +                * its own descriptor, using the guc_class here will help keep
> +                * the i915 and firmware logs in sync.
> +                */
> +               if (HAS_GUC_SCHED(ctx->i915))
> +                       desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT;
> +               else
> +                       desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
>                                                                 /* bits 61-63 */

I'm fairly confident I've given this review comment long ago, but here
goes again.

The new member name should just be hw_class or something else agnostic,
and the branching of using ELSP vs. GuC identifier should happen at the engine
init time (or at another sweet spot). And then the actual write should
be unconditionally with the hw_class value.

We should not be adding GuC specifics to intel_lrc.c, I think.

Regards, Joonas