| Message ID | 20250325120137.1302748-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/gt: Avoid duplicating CCS mode workaround | expand |
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@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@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@intel.com> Date: Tue Mar 19 23:03:03 2024 -0700 drm/i915: Remove special handling for !RCS_MASK() -Chris
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@intel.com> > > Signed-off-by: Andi Shyti <andi.shyti@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@intel.com> > 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(). 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)
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@intel.com> >> > Signed-off-by: Andi Shyti <andi.shyti@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@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)
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@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 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 noticed that we end up applying the workaround twice. So the !RCS_MASK(gt) check is still needed. Alternatively, as Matt suggested, we could assign I915_ENGINE_FIRST_RENDER_COMPUTE only to the RCS. 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.
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@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.
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); } if (engine->class == COMPUTE_CLASS)
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@intel.com> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)