[PATCH] drm/i915/gt: Avoid duplicating CCS mode workaround

[Lucas De Marchi]

On Tue, Mar 25, 2025 at 03:26:24PM +0100, Andi Shyti wrote:
>On Tue, Mar 25, 2025 at 01:57:42PM +0100, Chris Wilson wrote:
>> Quoting Andi Shyti (2025-03-25 13:01:37)
>> > When generating workarounds for the CCS engine, specifically for
>> > setting the CCS mode related to compute load balancing, the
>> > function 'ccs_engine_wa_mode()' is called twice: once for the
>> > render engine and once for the compute engine.
>> >
>> > Add a check to ensure the engine class is compute before applying
>> > the workaround to avoid redundant programming.
>> >
>> > Suggested-by: Arshad Mehmood <arshad.mehmood at intel.com>
>> > Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > index 116683ebe074..37251546b755 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > @@ -2897,7 +2897,9 @@ engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal
>> >          */
>> >         if (engine->flags & I915_ENGINE_FIRST_RENDER_COMPUTE) {
>> >                 general_render_compute_wa_init(engine, wal);
>> > -               ccs_engine_wa_mode(engine, wal);
>> > +
>> > +               if (engine->class == COMPUTE_CLASS)
>> > +                       ccs_engine_wa_mode(engine, wal);
>>
>> FIRST_RENDER_COMPUTE is meant to only be on the first engine of either
>> rcs or ccs (which share certain register domains), one engine.
>>
>> It looks like that was broken by
>>
>> 	commit 1bfc03b1375244f9029bb448ee8224b3b6dae99f
>> 	Author: Lucas De Marchi <lucas.demarchi at intel.com>

yep, my bad.

>> 	Date:   Tue Mar 19 23:03:03 2024 -0700
>>
>> 	    drm/i915: Remove special handling for !RCS_MASK()
>
>Aha! So the logic here[*] breaks the meaning of
>I915_ENGINE_FIRST_RENDER_COMPUTE, becasue, other than PVC, we
>forgot that we have DG2 that needs the special check that uses
>!RCS_MASK().

no, that would mean a DG2 without render engine.

The previous check to enable I915_ENGINE_FIRST_RENDER_COMPUTE was:

	if ((engine->class == COMPUTE_CLASS && !RCS_MASK(engine->gt) &&
	     __ffs(CCS_MASK(engine->gt)) == engine->instance) ||
	     engine->class == RENDER_CLASS)

i.e. if render is fused off, it enables it in the first compute engine.
Otherwise it enables it in the render.

And assuming we don't have platforms with render fused off (which still
holds true as far as I'm aware), that became:

	if ((engine->class == COMPUTE_CLASS || engine->class == RENDER_CLASS) &&
	    __ffs(CCS_MASK(engine->gt) | RCS_MASK(engine->gt)) == engine->instance)

It was supposed to mean the same thing... but doesn't as engine->instance
starts from 0 for each class.

Just checking for RENDER_CLASS and eventually even removing the
I915_ENGINE_FIRST_RENDER_COMPUTE flag would be better. See
https://lore.kernel.org/all/20240314205746.GG2837363@mdroper-desk1.amr.corp.intel.com/#t

Lucas De Marchi

>
>I need then to restore the original check.
>
>Thanks Chris!
>Andi
>
>[*]
>-       if ((engine->class == COMPUTE_CLASS && !RCS_MASK(engine->gt) &&
>-            __ffs(CCS_MASK(engine->gt)) == engine->instance) ||
>-            engine->class == RENDER_CLASS)
>+       if ((engine->class == COMPUTE_CLASS || engine->class == RENDER_CLASS) &&
>+           __ffs(CCS_MASK(engine->gt) | RCS_MASK(engine->gt)) == engine->instance)