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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Signed-off-by: Oscar Mateo <oscar.mateo@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. But I guess null-terminating it at the 
end wouldn't harm. Or maybe snprintf to eliminate the dilemma if it 
creates neater code?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx