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.demar...@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.gg2837...@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.
