Re: [PATCH v3 1/4] drm/i915/gt: Refactor uabi engine class/instance list creation

2024-03-05 Thread Andi Shyti 
Hi Joonas,

...

> >  void intel_engines_driver_register(struct drm_i915_private *i915)
> >  {
> > -   u16 name_instance, other_instance = 0;
> > +   u16 class_instance[I915_LAST_UABI_ENGINE_CLASS + 1] = { };
> 
> Do you mean this to be size I915_LAST_UABI_ENGINE_CLASS + 2? Because ...

Yes, this is an oversight. I was playing around with indexes to
optimize the code a bit more and I forgot to restore this back.

Thanks,
Andi

> 
> 
> > @@ -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;
> 
> .. otherwise this ...
> 
> > +   else
> > +   uabi_class = engine->uabi_class;
> > +
> > +   GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance));
> 
> .. will trigger this assertion?
> 
> Regards, Joonas

Re: [PATCH v3 1/4] drm/i915/gt: Refactor uabi engine class/instance list creation

2024-03-05 Thread Joonas Lahtinen 
Quoting Andi Shyti (2024-03-01 01:28:56)
> For the upcoming changes we need a cleaner way to build the list
> of uabi engines.
> 
> Suggested-by: Tvrtko Ursulin 
> Signed-off-by: Andi Shyti 
> ---
>  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..cf8f24ad88f6 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 + 1] = { };

Do you mean this to be size I915_LAST_UABI_ENGINE_CLASS + 2? Because ...



> @@ -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;

.. otherwise this ...

> +   else
> +   uabi_class = engine->uabi_class;
> +
> +   GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance));

.. will trigger this assertion?

Regards, Joonas

[PATCH v3 1/4] drm/i915/gt: Refactor uabi engine class/instance list creation

2024-02-29 Thread Andi Shyti 
For the upcoming changes we need a cleaner way to build the list
of uabi engines.

Suggested-by: Tvrtko Ursulin 
Signed-off-by: Andi Shyti 
---
 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..cf8f24ad88f6 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 + 1] = { };
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 = >uabi_engines.rb_node;
list_for_each_safe(it, next, ) {
+   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(>uabi_node, prev, p);
rb_insert_color(>uabi_node, >uabi_engines);
 
-- 
2.43.0