[igt-dev] [RFC 2/2] lib: implement new engine discovery interface

[Tvrtko Ursulin]

On 19/11/2018 20:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-11-19 19:59:19)
>>
>> On 19/11/2018 15:55, Andi Shyti wrote:
>>> Kernel commits:
>>>
>>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
>>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
>>>
>>> from [*] repository, implement a new way uapi for engine
>>> discovery that consist in first coupling context with engine [2]
>>> and then query the engines through a specific structure [1].
>>>
>>> In igt the classic way for discovering engines is done trhough
>>> the for_each_physical_engine() macro, that is replaced by the
>>> new for_each_engine_ctx().
>>
>> My idea was that we migrate for_each_physical_engine to this new scheme.
>>
>> As an easy starting point I was thinking to keep the current
>> for_each_physical_engine as is, and just add new
>> for_each_physical_engine_new and migrate one test to it as a demo.
>>
>> Then when this part is reviewed, as a second step we convert the rest
>> and rename the macro stripping the _new suffix and nuking the old one.
>>
>> With regards to implementation details I was thinking along these lines:
>>
>> On first invocation for_each_physical_engine_new initializes some hidden
>> data stored in one or more globals (which will live in igt_gt.c).
>>
>> This would query the engines and create a context with all engines mapped.
>>
>> We also add a helper to get this internal ctx id to use within
>> for_each_physical_engine_new loops.
>>
>> That should make it easy to convert simple tests over like:
>>
>>     igt_subtest {
>>          for_each_physical_engine_new(fd, engine) {
>>                  ...
>>                  execbuf.rsvd1 = gem_physical_engine_ctx();
>>                  execbuf.flags = gem_physical_engine_idx(engine);
>>                  gem_execbuf(fd, execbuf);
>>          }
>>     }
> 
> Granted that we have a lot of tests that just use the default ctx, I
> don't think the iterator interface we create should enforce that.
> 
> for_each_physical_engine_new(fd, ctx, engine) {
> 	execbuf.rsvd1 = ctx;
> 	execbuf.flags = engine;
> 	gem_execbuf(fd, execbuf);
> }

To be clear, you think we should convert all such tests to use a 
non-default context? My idea was to minimize the churn, but I am also 
okay with this plan.

> where ctx is provided, and engine the iterator. Off the top of my head,
> I have a silly idea like
> 
> for (int __max_engine__; (__max_engine__ = igt_physical_engine_iter_init(fd, ctx)); )
> 	for (engine = 1; engine <= __max_engine__; engine++)
> 
> where igt_physical_engine_iter_init(int fd, uint32_t ctx) {
> 	if (ctx_getparam(fd, ctx, ENGINES).count > 0) {
> 		ctx_setparam(fd, ctx, ENGINES, NULL);
> 		return 0;
> 	}
> 
> 	// query and set engine array
> 	return count;
> }

That works and avoids the global on the face of it. However with engine 
iterator a simple int, we will probably still need some to enable 
querying iterated engine properties like class, instance and name for 
subtest enumeration and similar.

Regards,

Tvrtko