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

[Oscar Mateo]

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?).

>>          .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?)

>>      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?

>>      engine->base.id = id++;
>>      engine->base.status_page.page_addr = (void *)(engine + 1);
>>
>>
>
> Regards,
>
> Tvrtko