| Message ID | 20240308202223.406384-3-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Disable automatic load CCS load balancing | expand |
On Fri, Mar 08, 2024 at 09:22:17PM +0100, Andi Shyti wrote: > For the upcoming changes we need a cleaner way to build the list > of uabi engines. > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > Cc: <stable@vger.kernel.org> # v6.2+ I don't really see why we need patches 2 & 3 in this series. If we want to restrict the platform to a single CCS engine for now (and give that single engine access to all of the cslices), it would be much simpler to only create a single intel_engine_cs which which would then cause both i915 and userspace to only consider a single engine, even if more than one is physically present. That could be done with a simple adjustment to engine_mask_apply_compute_fuses() to mask off extra bits from the engine mask such that only a single CCS can get returned rather than the mask of all CCSs that are present. Managing all of the engines in the KMD but only exposing one (some) of them to userspace might be something we need if you want to add extra functionality down to road to "hotplug" extra engines, or to allow userspace to explicitly request multi-CCS mode. But none of that seems necessary for this series, especially for something you're backporting to stable kernels. Matt > --- > drivers/gpu/drm/i915/gt/intel_engine_user.c | 29 ++++++++++++--------- > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c > index 833987015b8b..11cc06c0c785 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c > @@ -203,7 +203,7 @@ static void engine_rename(struct intel_engine_cs *engine, const char *name, u16 > > void intel_engines_driver_register(struct drm_i915_private *i915) > { > - u16 name_instance, other_instance = 0; > + u16 class_instance[I915_LAST_UABI_ENGINE_CLASS + 2] = { }; > struct legacy_ring ring = {}; > struct list_head *it, *next; > struct rb_node **p, *prev; > @@ -214,6 +214,8 @@ void intel_engines_driver_register(struct drm_i915_private *i915) > prev = NULL; > p = &i915->uabi_engines.rb_node; > list_for_each_safe(it, next, &engines) { > + u16 uabi_class; > + > struct intel_engine_cs *engine = > container_of(it, typeof(*engine), uabi_list); > > @@ -222,15 +224,14 @@ void intel_engines_driver_register(struct drm_i915_private *i915) > > GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes)); > engine->uabi_class = uabi_classes[engine->class]; > - if (engine->uabi_class == I915_NO_UABI_CLASS) { > - name_instance = other_instance++; > - } else { > - GEM_BUG_ON(engine->uabi_class >= > - ARRAY_SIZE(i915->engine_uabi_class_count)); > - name_instance = > - i915->engine_uabi_class_count[engine->uabi_class]++; > - } > - engine->uabi_instance = name_instance; > + > + if (engine->uabi_class == I915_NO_UABI_CLASS) > + uabi_class = I915_LAST_UABI_ENGINE_CLASS + 1; > + else > + uabi_class = engine->uabi_class; > + > + GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance)); > + engine->uabi_instance = class_instance[uabi_class]++; > > /* > * Replace the internal name with the final user and log facing > @@ -238,11 +239,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915) > */ > engine_rename(engine, > intel_engine_class_repr(engine->class), > - name_instance); > + engine->uabi_instance); > > - if (engine->uabi_class == I915_NO_UABI_CLASS) > + if (uabi_class > I915_LAST_UABI_ENGINE_CLASS) > continue; > > + GEM_BUG_ON(uabi_class >= > + ARRAY_SIZE(i915->engine_uabi_class_count)); > + i915->engine_uabi_class_count[uabi_class]++; > + > rb_link_node(&engine->uabi_node, prev, p); > rb_insert_color(&engine->uabi_node, &i915->uabi_engines); > > -- > 2.43.0 >
On Tue, Mar 12, 2024 at 10:08:33AM -0700, Matt Roper wrote: > On Fri, Mar 08, 2024 at 09:22:17PM +0100, Andi Shyti wrote: > > For the upcoming changes we need a cleaner way to build the list > > of uabi engines. > > > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > > Cc: <stable@vger.kernel.org> # v6.2+ > > I don't really see why we need patches 2 & 3 in this series. For patch number '2' We had a round of review with Tvrtko and we wanted to avoid the change I pasted at the bottom[*], which would decrease something that was increased earlier. > If we want > to restrict the platform to a single CCS engine for now (and give that > single engine access to all of the cslices), it would be much simpler to > only create a single intel_engine_cs which which would then cause both > i915 and userspace to only consider a single engine, even if more than > one is physically present. That could be done with a simple adjustment > to engine_mask_apply_compute_fuses() to mask off extra bits from the > engine mask such that only a single CCS can get returned rather than the > mask of all CCSs that are present. > > Managing all of the engines in the KMD but only exposing one (some) of > them to userspace might be something we need if you want to add extra > functionality down to road to "hotplug" extra engines, or to allow > userspace to explicitly request multi-CCS mode. But none of that seems > necessary for this series, especially for something you're backporting > to stable kernels. It's true, it would even be easier to mask out all the CCS engines after the first. I thought of this. On one hand hand, adding a for_each_available_engine() throught the stable path its a bit of abusing, but it's functional to the single CCS mode. I was aiming for a longer term solution. If I add a patch to mask off CCS engines, then I will need to revert it quite soon for the stable release. I'm not sure which one is better, though. Thanks, Andi [*] diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c index 833987015b8b..7041acc77810 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c @@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915) if (engine->uabi_class == I915_NO_UABI_CLASS) continue; + /* + * Do not list and do not count CCS engines other than the first + */ + if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE && + engine->uabi_instance > 0) { + i915->engine_uabi_class_count[engine->uabi_class]--; + continue; + } + rb_link_node(&engine->uabi_node, prev, p); rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c index 833987015b8b..11cc06c0c785 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c @@ -203,7 +203,7 @@ static void engine_rename(struct intel_engine_cs *engine, const char *name, u16 void intel_engines_driver_register(struct drm_i915_private *i915) { - u16 name_instance, other_instance = 0; + u16 class_instance[I915_LAST_UABI_ENGINE_CLASS + 2] = { }; struct legacy_ring ring = {}; struct list_head *it, *next; struct rb_node **p, *prev; @@ -214,6 +214,8 @@ void intel_engines_driver_register(struct drm_i915_private *i915) prev = NULL; p = &i915->uabi_engines.rb_node; list_for_each_safe(it, next, &engines) { + u16 uabi_class; + struct intel_engine_cs *engine = container_of(it, typeof(*engine), uabi_list); @@ -222,15 +224,14 @@ void intel_engines_driver_register(struct drm_i915_private *i915) GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes)); engine->uabi_class = uabi_classes[engine->class]; - if (engine->uabi_class == I915_NO_UABI_CLASS) { - name_instance = other_instance++; - } else { - GEM_BUG_ON(engine->uabi_class >= - ARRAY_SIZE(i915->engine_uabi_class_count)); - name_instance = - i915->engine_uabi_class_count[engine->uabi_class]++; - } - engine->uabi_instance = name_instance; + + if (engine->uabi_class == I915_NO_UABI_CLASS) + uabi_class = I915_LAST_UABI_ENGINE_CLASS + 1; + else + uabi_class = engine->uabi_class; + + GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance)); + engine->uabi_instance = class_instance[uabi_class]++; /* * Replace the internal name with the final user and log facing @@ -238,11 +239,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915) */ engine_rename(engine, intel_engine_class_repr(engine->class), - name_instance); + engine->uabi_instance); - if (engine->uabi_class == I915_NO_UABI_CLASS) + if (uabi_class > I915_LAST_UABI_ENGINE_CLASS) continue; + GEM_BUG_ON(uabi_class >= + ARRAY_SIZE(i915->engine_uabi_class_count)); + i915->engine_uabi_class_count[uabi_class]++; + rb_link_node(&engine->uabi_node, prev, p); rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
For the upcoming changes we need a cleaner way to build the list of uabi engines. Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> Cc: <stable@vger.kernel.org> # v6.2+ --- drivers/gpu/drm/i915/gt/intel_engine_user.c | 29 ++++++++++++--------- 1 file changed, 17 insertions(+), 12 deletions(-)