[Intel-gfx] [PATCH 3/5] drm/i915: Generate the engine name based on the instance number

[Chris Wilson]

On Thu, Apr 06, 2017 at 07:37:20PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/04/2017 12:22, Oscar Mateo wrote:
> >On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote:
> >>On 05/04/2017 10:30, Oscar Mateo wrote:
> >>>Not really needed, but makes the next change a little bit more compact.
> >>>
> >>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >>>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >>>Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> >>>---
> >>> drivers/gpu/drm/i915/intel_engine_cs.c       | 8 ++++++--
> >>> drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
> >>> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
> >>> 3 files changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>index abc0a9c..530f822 100644
> >>>--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>@@ -71,7 +71,7 @@
> >>>         .init_legacy = intel_init_bsd_ring_buffer,
> >>>     },
> >>>     [VCS2] = {
> >>>-        .name = "vcs2",
> >>>+        .name = "vcs",
> >>
> >>Rename the field to class_name perhaps?
> >>
> >
> >In the following patch, when I move .name to the class table, it becomes
> >much more obvious what it refers to, but I can change it to class_name
> >if you feel strongly about it (or change it here and then back to name
> >in the next patch?).
> 
> No it's fine like it is, just a consequence of not looking-ahead enough.
> 
> >
> >>>         .hw_id = VCS2_HW,
> >>>         .exec_id = I915_EXEC_BSD,
> >>>         .class = VIDEO_DECODE_CLASS,
> >>>@@ -100,6 +100,7 @@
> >>> {
> >>>     const struct engine_info *info = &intel_engines[id];
> >>>     struct intel_engine_cs *engine;
> >>>+    char instance[3] = "";
> >>>
> >>>     GEM_BUG_ON(dev_priv->engine[id]);
> >>>     engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> >>>@@ -108,7 +109,10 @@
> >>>
> >>>     engine->id = id;
> >>>     engine->i915 = dev_priv;
> >>>-    engine->name = info->name;
> >>>+    /* For historical reasons the engines are called: name, name2... */
> >>>+    if (info->instance)
> >>>+        snprintf(instance, sizeof(instance), "%u", info->instance + 1);
> >>>+    snprintf(engine->name, sizeof(engine->name), "%s%s", info->name,
> >>>instance);
> >>
> >>Since Chris has recently renamed all the engines, I'd say who cares
> >>about the numbering scheme. Just drop it for simplicity.
> >>
> >
> >Oooohh!
> >Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1
> >... xcsN?)
> 
> Sounds like the verdict is yes.
> 
> >
> >>>     engine->exec_id = info->exec_id;
> >>>     engine->hw_id = engine->guc_id = info->hw_id;
> >>>     engine->mmio_base = info->mmio_base;
> >>>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>index 5c1a27f..d617049 100644
> >>>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>@@ -187,9 +187,11 @@ enum intel_engine_id {
> >>>     VECS
> >>> };
> >>>
> >>>+#define INTEL_ENGINE_CS_MAX_NAME 8
> >>>+
> >>> struct intel_engine_cs {
> >>>     struct drm_i915_private *i915;
> >>>-    const char    *name;
> >>>+    char name[INTEL_ENGINE_CS_MAX_NAME];
> >>>     enum intel_engine_id id;
> >>>     unsigned int exec_id;
> >>>     unsigned int hw_id;
> >>>diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>b/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>index b89050e..4a1ffca 100644
> >>>--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>@@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct
> >>>drm_i915_private *i915,
> >>>
> >>>     /* minimal engine setup for requests */
> >>>     engine->base.i915 = i915;
> >>>-    engine->base.name = name;
> >>>+    strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);
> >>
> >>Can this miss to null-terminate? Or it relies on the mock_engine being
> >>kzalloced?
> >>
> >
> >It relies on the kzalloc, but I can add a \0 at the end for extra security?
> 
> Not sure at the moment without checking in detail how much
> mock_engine already depends on being zeroed.

It does depend on the kzalloc.

> But I guess
> null-terminating it at the end wouldn't harm. Or maybe snprintf to
> eliminate the dilemma if it creates neater code?

Indeed, I want my mockN! We may want multiple instances of the mock
engine class in the future.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre