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

[Lucas De Marchi]

On Tue, Mar 25, 2025 at 04:39:57PM +0100, Andi Shyti wrote:
>Hi Lucas,
>
>> > > > @@ -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.
>
>OK, I don't know the history, I thought that the idea was to
>remove support for PVC, the only multi-CCS machine that once i915
>supported other than DG2.

the problem is not about having multiple compute engines. The removal of
PVC meant we don't have any platform left 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)
>
>The difference is that this applies in two cases: it's true for
>the first CCS we encounter and also for the only RCS. Arshad

yes, this is the same thing I said in my reply:

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

>noticed that we end up applying the workaround twice.
>
>So the !RCS_MASK(gt) check is still needed.

I don't think the !RCS_MASK() makes sense - you are checking for "do we
have any render engine?" when we always do and it's always 1.
All platforms supported by i915 have 1 render engine.

>
>Alternatively, as Matt suggested, we could assign
>I915_ENGINE_FIRST_RENDER_COMPUTE only to the RCS.

yes, that's what I said, but the reply here was cut short:

	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 have a slight preference for the way it was done before because
>it make the logic clearer.
>
>Thanks,
>Andi
>
>> It was supposed to mean the same thing... but doesn't as engine->instance
>> starts from 0 for each class.