Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime

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

On 18/11/2019 10:05, Petri Latvala wrote:
> On Fri, Nov 15, 2019 at 04:01:45PM +0000, Tvrtko Ursulin wrote:
>> On 15/11/2019 12:14, Petri Latvala wrote:
>>> The kernel supplies us an engine list with engine class and an
>>> instance id. If the hardcoded engine list doesn't have the
>>> class+instance we get, construct the engine name with printf instead
>>> of calling additional engines "unknown".
>>>
>>> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>>>
>>> ---
>>>    lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
>>>    lib/i915/gem_engine_topology.h |  2 +-
>>>    lib/igt_gt.c                   |  8 ++++++++
>>>    lib/igt_gt.h                   |  7 ++++++-
>>>    4 files changed, 34 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
>>> index 790d455f..0707f237 100644
>>> --- a/lib/i915/gem_engine_topology.c
>>> +++ b/lib/i915/gem_engine_topology.c
>>> @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>>    	const struct intel_execution_engine2 *__e2;
>>>    	static const char *unknown_name = "unknown",
>>>    			  *virtual_name = "virtual";
>>> +	const struct intel_execution_engine_class *cls;
>>>    	e2->class    = class;
>>>    	e2->instance = instance;
>>> @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>>    	/* engine is a virtual engine */
>>>    	if (class == I915_ENGINE_CLASS_INVALID &&
>>>    	    instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
>>> -		e2->name = virtual_name;
>>> +		strncpy(e2->name, virtual_name, sizeof(e2->name));
>>>    		e2->is_virtual = true;
>>>    		return;
>>>    	}
>>> @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>>    		if (__e2->class == class && __e2->instance == instance)
>>>    			break;
>>> -	if (__e2->name) {
>>> -		e2->name = __e2->name;
>>> +	if (__e2->name[0]) {
>>> +		strncpy(e2->name, __e2->name, sizeof(e2->name));
>>>    	} else {
>>> -		igt_warn("found unknown engine (%d, %d)\n", class, instance);
>>> -		e2->name = unknown_name;
>>> -		e2->flags = -1;
>>> +		for (cls = intel_execution_engine_class_map; cls->name; cls++) {
>>> +			if (cls->class == class) {
>>> +				snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
>>> +				igt_info("unknown but supported engine %s found\n", e2->name);
>>
>> Hm not really unknown, just a new instance relative to the static table. I'd
>> probably downgrade this to debug level?
> 
> Ah, a leftover from my debugging, ironically.
> 
>>
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		if (!e2->name[0]) {
>>> +			igt_info("found unknown engine (%d, %d)\n", class, instance);
>>> +			strncpy(e2->name, unknown_name, sizeof(e2->name));
>>
>> unknown<class>:<instance> ?
>>
>> And say unknown _class_ in the log message?
> 
> Ok.
> 
>>
>>
>>> +			e2->flags = -1;
>>> +		}
>>>    	}
>>>    	/* just to remark it */
>>> @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
>>>    			struct intel_execution_engine2 *__e2 =
>>>    				&engine_data.engines[engine_data.nengines];
>>> -			__e2->name       = e2->name;
>>> +			strncpy(__e2->name, e2->name, sizeof(__e2->name));
>>>    			__e2->instance   = e2->instance;
>>>    			__e2->class      = e2->class;
>>>    			__e2->flags      = e2->flags;
>>> @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
>>>    	if (ring == I915_EXEC_DEFAULT) {
>>>    		e2__.flags = I915_EXEC_DEFAULT;
>>> -		e2__.name = "default";
>>> +		strncpy(e2__.name, "default", sizeof(e2__.name));
>>>    	} else {
>>>    		const struct intel_execution_engine2 *e2;
>>> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
>>> index d98773e0..525741cc 100644
>>> --- a/lib/i915/gem_engine_topology.h
>>> +++ b/lib/i915/gem_engine_topology.h
>>> @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
>>>    struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
>>>    #define __for_each_static_engine(e__) \
>>> -	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>>> +	for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
>>>    #define for_each_context_engine(fd__, ctx__, e__) \
>>>    	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
>>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>>> index 256c7cbc..25e6c455 100644
>>> --- a/lib/igt_gt.c
>>> +++ b/lib/igt_gt.c
>>> @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>>>    	{ }
>>>    };
>>> +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
>>> +	{ "rcs", I915_ENGINE_CLASS_RENDER },
>>> +	{ "bcs", I915_ENGINE_CLASS_COPY },
>>> +	{ "vcs", I915_ENGINE_CLASS_VIDEO },
>>> +	{ "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
>>> +	{ }
>>> +};
>>> +
>>>    int gem_execbuf_flags_to_engine_class(unsigned int flags)
>>>    {
>>>    	switch (flags & 0x3f) {
>>> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
>>> index 66088d39..0268031f 100644
>>> --- a/lib/igt_gt.h
>>> +++ b/lib/igt_gt.h
>>> @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
>>>    bool gem_class_can_store_dword(int fd, int class);
>>>    extern const struct intel_execution_engine2 {
>>> -	const char *name;
>>> +	char name[10];
>>>    	int class;
>>>    	int instance;
>>>    	uint64_t flags;
>>>    	bool is_virtual;
>>>    } intel_execution_engines2[];
>>> +extern const struct intel_execution_engine_class {
>>> +	const char *name;
>>> +	int class;
>>> +} intel_execution_engine_class_map[];
>>> +
>>>    int gem_execbuf_flags_to_engine_class(unsigned int flags);
>>>    #endif /* IGT_GT_H */
>>>
>>
>> Implementation looks okay to me, but what does it gains us? Potentially
>> introduces a slight confusion between subtests as enumerated by engine and
>> engines iterated inside some tests.
> 
> When all tests that enumerate subtests per engine have been converted
> to dynamic subtests, this reduces the possible maintenance burden in
> the coming years when we might have more engines. Imagine having a new
> device tomorrow with vcs3 for example. With this patch it doesn't
> require further IGT changes to be tested.
> 
> I should have said that merging this practically requires all
> per-engine subtest enumeration converted to dynamic subtests first...
> 
> 
>>
>> Only an interim step before elimination of the static array? But danger is
>> in the above mismatch.
> 
> What mismatch? *confusion* If you mean the enumeration, I hope the
> above paragraph alleviates the dangers you see.
> 
> Interim step yes, I wouldn't call this anywhere close to clean code
> atm.
> 
> Currently we need the static array for two things:
> 
> 1) subtest enumeration
> 2) carrying the flags for the "old-style" execution
> 
> I'm hoping we can drop 1) sooner than later.

No need to explain the advantages. ;) If this would be merged together 
with removal of the static array, that is, all engine tests were 
converted to dynamic, there would be no issue. But if we merge this 
before, then we have a confusion where some tests, which seemingly use 
the new API (have been converted), can see some engines and some cannot.

I think that complaint would go away if you didn't include the "unknown 
but supported" change in this patch. Then it would behave like the 
current code where if the engine is not in the static list it always 
refuses to use it.

Although I am not sure how important that is, I guess it is open for 
discussion.

Perhaps you could split the patch into two, where the first one just 
adds the runtime name generation, and second one extends with support 
for unknown engines of a known class?

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev