Re: [PATCH] drm/i915/gsc: Mark internal GSC engine with reserved uabi class
On 11/15/2023 3:02 AM, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
The GSC CS is not exposed to the user, so we skipped assigning a uabi
class number for it. However, the trace logs use the uabi class and
instance to identify the engine, so leaving uabi class unset makes the
GSC CS show up as the RCS in those logs.
Given that the engine is not exposed to the user, we can't add a new
case in the uabi enum, so we insted internally define a kernel
internal class as -1.
At the same time remove special handling for the name and complete
the uabi_classes array so internal class is automatically correctly
assigned.
Engine will show as 65535:0 other0 in the logs/traces which should
be unique enough.
Signed-off-by: Tvrtko Ursulin
Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command streamer to the
user")
Cc: Daniele Ceraolo Spurio
Cc: Alan Previn
Cc: Matt Roper
---
Daniele I borrowed most of your commit text as is, hope you don't mind but
I was lazy. See if you like this solution. It is also untested so lets see.
I'm ok with this approach. As you said the naming is unique so we can
easily identify the engine. I've tested this locally with a small change
(see below) and I see the expected values in the logs.
---
drivers/gpu/drm/i915/gt/intel_engine_user.c | 37 -
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
b/drivers/gpu/drm/i915/gt/intel_engine_user.c
index 118164ddbb2e..7693ccfac1f9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
@@ -41,12 +41,15 @@ void intel_engine_add_user(struct intel_engine_cs *engine)
llist_add(>uabi_llist, >i915->uabi_engines_llist);
}
+#define I915_NO_UABI_CLASS (-1)
I see the lkp is complaining about using this for comparison against a
u16. When I locally tried to reduce this to u16 my compiler also
complained that we're assigning it to a u8 in the uabi_classes array, so
I've just set I915_NO_UABI_CLASS directly to 255 and it all worked as
expected. With that fix, or an alternative change to work with all the
involved types:
Reviewed-by: Daniele Ceraolo Spurio
Daniele
+
static const u8 uabi_classes[] = {
[RENDER_CLASS] = I915_ENGINE_CLASS_RENDER,
[COPY_ENGINE_CLASS] = I915_ENGINE_CLASS_COPY,
[VIDEO_DECODE_CLASS] = I915_ENGINE_CLASS_VIDEO,
[VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
[COMPUTE_CLASS] = I915_ENGINE_CLASS_COMPUTE,
+ [OTHER_CLASS] = I915_NO_UABI_CLASS, /* Not exposed to users, no uabi
class. */
};
static int engine_cmp(void *priv, const struct list_head *A,
@@ -200,6 +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;
struct legacy_ring ring = {};
struct list_head *it, *next;
struct rb_node **p, *prev;
@@ -216,27 +220,28 @@ void intel_engines_driver_register(struct
drm_i915_private *i915)
if (intel_gt_has_unrecoverable_error(engine->gt))
continue; /* ignore incomplete engines */
- /*
-* We don't want to expose the GSC engine to the users, but we
-* still rename it so it is easier to identify in the debug logs
-*/
- if (engine->id == GSC0) {
- engine_rename(engine, "gsc", 0);
- continue;
- }
-
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;
- GEM_BUG_ON(engine->uabi_class >=
- ARRAY_SIZE(i915->engine_uabi_class_count));
- engine->uabi_instance =
- i915->engine_uabi_class_count[engine->uabi_class]++;
-
- /* Replace the internal name with the final user facing name */
+ /*
+* Replace the internal name with the final user and log facing
+* name.
+*/
engine_rename(engine,
intel_engine_class_repr(engine->class),
- engine->uabi_instance);
+ name_instance);
+
+ if (engine->uabi_class == I915_NO_UABI_CLASS)
+ continue;
Re: [PATCH] drm/i915/gsc: Mark internal GSC engine with reserved uabi class
Hi Tvrtko,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-intel/for-linux-next-fixes]
[also build test WARNING on drm-tip/drm-tip drm/drm-next
drm-exynos/exynos-drm-next drm-misc/drm-misc-next linus/master v6.7-rc1
next-20231115]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Tvrtko-Ursulin/drm-i915-gsc-Mark-internal-GSC-engine-with-reserved-uabi-class/20231115-190507
base: git://anongit.freedesktop.org/drm-intel for-linux-next-fixes
patch link:
https://lore.kernel.org/r/20231115110216.267138-1-tvrtko.ursulin%40linux.intel.com
patch subject: [PATCH] drm/i915/gsc: Mark internal GSC engine with reserved
uabi class
config: x86_64-rhel-8.3-rust
(https://download.01.org/0day-ci/archive/20231116/202311160136.etoh3ghf-...@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git
ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20231116/202311160136.etoh3ghf-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202311160136.etoh3ghf-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/gt/intel_engine_user.c:225:26: warning: result of
>> comparison of constant -1 with expression of type 'u16' (aka 'unsigned
>> short') is always false [-Wtautological-constant-out-of-range-compare]
if (engine->uabi_class == I915_NO_UABI_CLASS) {
~~ ^ ~~
drivers/gpu/drm/i915/gt/intel_engine_user.c:243:26: warning: result of
comparison of constant -1 with expression of type 'u16' (aka 'unsigned short')
is always false [-Wtautological-constant-out-of-range-compare]
if (engine->uabi_class == I915_NO_UABI_CLASS)
~~ ^ ~~
2 warnings generated.
vim +225 drivers/gpu/drm/i915/gt/intel_engine_user.c
203
204 void intel_engines_driver_register(struct drm_i915_private *i915)
205 {
206 u16 name_instance, other_instance = 0;
207 struct legacy_ring ring = {};
208 struct list_head *it, *next;
209 struct rb_node **p, *prev;
210 LIST_HEAD(engines);
211
212 sort_engines(i915, );
213
214 prev = NULL;
215 p = >uabi_engines.rb_node;
216 list_for_each_safe(it, next, ) {
217 struct intel_engine_cs *engine =
218 container_of(it, typeof(*engine), uabi_list);
219
220 if (intel_gt_has_unrecoverable_error(engine->gt))
221 continue; /* ignore incomplete engines */
222
223 GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes));
224 engine->uabi_class = uabi_classes[engine->class];
> 225 if (engine->uabi_class == I915_NO_UABI_CLASS) {
226 name_instance = other_instance++;
227 } else {
228 GEM_BUG_ON(engine->uabi_class >=
229
ARRAY_SIZE(i915->engine_uabi_class_count));
230 name_instance =
231
i915->engine_uabi_class_count[engine->uabi_class]++;
232 }
233 engine->uabi_instance = name_instance;
234
235 /*
236 * Replace the internal name with the final user and
log facing
237 * name.
238 */
239 engine_rename(engine,
240intel_engine_class_repr(engine->class),
241name_instance);
242
243 if (engine->uabi_class == I915_NO_UABI_CLASS)
244 continue;
245
246 rb_link_node(>uabi_node, prev, p);
247 rb_insert_color(>uabi_node,
>uabi_engines);
248
249 GEM_BUG_ON(intel_engine_lookup_user(i915,
250 engine->uabi_class,
251
engine->uabi_instance) != engine);
252
253 /* Fix up the mapping to match default
execbuf::user_map[] */
254 add_legacy_ring(, engin
[PATCH] drm/i915/gsc: Mark internal GSC engine with reserved uabi class
From: Tvrtko Ursulin
The GSC CS is not exposed to the user, so we skipped assigning a uabi
class number for it. However, the trace logs use the uabi class and
instance to identify the engine, so leaving uabi class unset makes the
GSC CS show up as the RCS in those logs.
Given that the engine is not exposed to the user, we can't add a new
case in the uabi enum, so we insted internally define a kernel
internal class as -1.
At the same time remove special handling for the name and complete
the uabi_classes array so internal class is automatically correctly
assigned.
Engine will show as 65535:0 other0 in the logs/traces which should
be unique enough.
Signed-off-by: Tvrtko Ursulin
Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command streamer to the
user")
Cc: Daniele Ceraolo Spurio
Cc: Alan Previn
Cc: Matt Roper
---
Daniele I borrowed most of your commit text as is, hope you don't mind but
I was lazy. See if you like this solution. It is also untested so lets see.
---
drivers/gpu/drm/i915/gt/intel_engine_user.c | 37 -
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
b/drivers/gpu/drm/i915/gt/intel_engine_user.c
index 118164ddbb2e..7693ccfac1f9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
@@ -41,12 +41,15 @@ void intel_engine_add_user(struct intel_engine_cs *engine)
llist_add(>uabi_llist, >i915->uabi_engines_llist);
}
+#define I915_NO_UABI_CLASS (-1)
+
static const u8 uabi_classes[] = {
[RENDER_CLASS] = I915_ENGINE_CLASS_RENDER,
[COPY_ENGINE_CLASS] = I915_ENGINE_CLASS_COPY,
[VIDEO_DECODE_CLASS] = I915_ENGINE_CLASS_VIDEO,
[VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
[COMPUTE_CLASS] = I915_ENGINE_CLASS_COMPUTE,
+ [OTHER_CLASS] = I915_NO_UABI_CLASS, /* Not exposed to users, no uabi
class. */
};
static int engine_cmp(void *priv, const struct list_head *A,
@@ -200,6 +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;
struct legacy_ring ring = {};
struct list_head *it, *next;
struct rb_node **p, *prev;
@@ -216,27 +220,28 @@ void intel_engines_driver_register(struct
drm_i915_private *i915)
if (intel_gt_has_unrecoverable_error(engine->gt))
continue; /* ignore incomplete engines */
- /*
-* We don't want to expose the GSC engine to the users, but we
-* still rename it so it is easier to identify in the debug logs
-*/
- if (engine->id == GSC0) {
- engine_rename(engine, "gsc", 0);
- continue;
- }
-
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;
- GEM_BUG_ON(engine->uabi_class >=
- ARRAY_SIZE(i915->engine_uabi_class_count));
- engine->uabi_instance =
- i915->engine_uabi_class_count[engine->uabi_class]++;
-
- /* Replace the internal name with the final user facing name */
+ /*
+* Replace the internal name with the final user and log facing
+* name.
+*/
engine_rename(engine,
intel_engine_class_repr(engine->class),
- engine->uabi_instance);
+ name_instance);
+
+ if (engine->uabi_class == I915_NO_UABI_CLASS)
+ continue;
rb_link_node(>uabi_node, prev, p);
rb_insert_color(>uabi_node, >uabi_engines);
--
2.40.1
