| Message ID | 20231115110216.267138-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gsc: Mark internal GSC engine with reserved uabi class | expand |
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-lkp@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-lkp@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 <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311160136.EtOH3ghf-lkp@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, &engines); 213 214 prev = NULL; 215 p = &i915->uabi_engines.rb_node; 216 list_for_each_safe(it, next, &engines) { 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, 240 intel_engine_class_repr(engine->class), 241 name_instance); 242 243 if (engine->uabi_class == I915_NO_UABI_CLASS) 244 continue; 245 246 rb_link_node(&engine->uabi_node, prev, p); 247 rb_insert_color(&engine->uabi_node, &i915->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(&ring, engine); 255 256 prev = &engine->uabi_node; 257 p = &prev->rb_right; 258 } 259 260 if (IS_ENABLED(CONFIG_DRM_I915_SELFTESTS) && 261 IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) { 262 struct intel_engine_cs *engine; 263 unsigned int isolation; 264 int class, inst; 265 int errors = 0; 266 267 for (class = 0; class < ARRAY_SIZE(i915->engine_uabi_class_count); class++) { 268 for (inst = 0; inst < i915->engine_uabi_class_count[class]; inst++) { 269 engine = intel_engine_lookup_user(i915, 270 class, inst); 271 if (!engine) { 272 pr_err("UABI engine not found for { class:%d, instance:%d }\n", 273 class, inst); 274 errors++; 275 continue; 276 } 277 278 if (engine->uabi_class != class || 279 engine->uabi_instance != inst) { 280 pr_err("Wrong UABI engine:%s { class:%d, instance:%d } found for { class:%d, instance:%d }\n", 281 engine->name, 282 engine->uabi_class, 283 engine->uabi_instance, 284 class, inst); 285 errors++; 286 continue; 287 } 288 } 289 } 290 291 /* 292 * Make sure that classes with multiple engine instances all 293 * share the same basic configuration. 294 */ 295 isolation = intel_engines_has_context_isolation(i915); 296 for_each_uabi_engine(engine, i915) { 297 unsigned int bit = BIT(engine->uabi_class); 298 unsigned int expected = engine->default_state ? bit : 0; 299 300 if ((isolation & bit) != expected) { 301 pr_err("mismatching default context state for class %d on engine %s\n", 302 engine->uabi_class, engine->name); 303 errors++; 304 } 305 } 306 307 if (drm_WARN(&i915->drm, errors, 308 "Invalid UABI engine mapping found")) 309 i915->uabi_engines = RB_ROOT; 310 } 311 312 set_scheduler_caps(i915); 313 } 314
On 11/15/2023 3:02 AM, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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 <tvrtko.ursulin@intel.com> > Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command streamer to the user") > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Alan Previn <alan.previn.teres.alexis@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > --- > 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(&engine->uabi_llist, &engine->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.ceraolospurio@intel.com> 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; > > 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 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(&engine->uabi_llist, &engine->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(&engine->uabi_node, prev, p); rb_insert_color(&engine->uabi_node, &i915->uabi_engines);