On 21/02/2024 11:19, Andi Shyti wrote:
Hi Tvrtko,
On Wed, Feb 21, 2024 at 08:19:34AM +0000, Tvrtko Ursulin wrote:
On 21/02/2024 00:14, Andi Shyti wrote:
On Tue, Feb 20, 2024 at 02:48:31PM +0000, Tvrtko Ursulin wrote:
On 20/02/2024 14:35, Andi Shyti wrote:
Enable only one CCS engine by default with all the compute sices
slices
Thanks!
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;
+ }
It's a bit ugly to decrement after increment, instead of somehow
restructuring the loop to satisfy both cases more elegantly.
yes, agree, indeed I had a hard time here to accept this change
myself.
But moving the check above where the counter was incremented it
would have been much uglier.
This check looks ugly everywhere you place it :-)
One idea would be to introduce a separate local counter array for
name_instance, so not use i915->engine_uabi_class_count[]. First one
increments for every engine, second only for the exposed ones. That way
feels wouldn't be too ugly.
Ah... you mean that whenever we change the CCS mode, we update
the indexes of the exposed engines from list of the real engines.
Will try.
My approach was to regenerate the list everytime the CCS mode was
changed, but your suggestion looks a bit simplier.
No, I meant just for this first stage of permanently single engine. For avoiding the decrement after increment. Something like this, but not compile tested even:
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
index 833987015b8b..4c33f30612c4 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
@@ -203,7 +203,8 @@ 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] = { };
+ u16 uabi_class, other_instance = 0;
struct legacy_ring ring = {};
struct list_head *it, *next;
struct rb_node **p, *prev;
@@ -222,15 +223,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;
+ 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 +238,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_NO_UABI_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);
In any case, I'm working on a patch that is splitting this
function in two parts and there is some refactoring happening
here (for the first initialization and the dynamic update).
Please let me know if it's OK with you or you want me to fix it
in this run.
And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0
ccs0 ccs0 ccs0.. for all instances.
I don't see this. Even in sysfs we see only one ccs. Where is it?
When you run this patch on something with two or more ccs-es, the "renamed
ccs... to ccs.." debug logs do not all log the new name as ccs0?
it shouldn't, because the name_instance is anyway incremented
normally... anyway, I will test it.
Hm maybe it needs more than two ccs engines and then it would be ccs0, ccs1, ccs2, ccs2, on a four ccs part. Or something.. It just feels the decrement of i915->engine_uabi_class_count, which engine_instance currently uses, has to mess this up somehow.
Regards,
Tvrtko
