Re: [igt-dev] [PATCH i-g-t v6 1/1] tests/i915/gem_ctx_persistence: Set context with supported engines

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

On 30/01/2020 11:13, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-30 11:08:47)
>>
>> On 30/01/2020 10:55, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-01-30 10:52:26)
>>>> On 30/01/2020 10:31, Tvrtko Ursulin wrote:
>>>>> On 29/01/2020 21:09, Chris Wilson wrote:
>>>>>> Quoting Bommu Krishnaiah (2020-01-29 18:10:02)
>>>>>>> Update the context with supported engines on the platform with set_property
>>>>>>> I915_CONTEXT_PARAM_ENGINES to make sure the work load is submitted to
>>>>>>> the available engines only.
>>>>>>>
>>>>>>> v2: addressed the review comments.
>>>>>>> v3: addressed v2 review comments.
>>>>>>> V4: Re-introduced the gem_context_set_all_engines() API,test_process_mixed()
>>>>>>> test creating new forked process, to set engine map to new forked process
>>>>>>> I added gem_context_set_all_engines() API.
>>>>>>> v5: addressed v4 review comments.
>>>>>>> v6: addressed chris comments.
>>>>>>
>>>>>> This is totally useless as a recording of the history. Explain why you
>>>>>> had to make the changes, for posterity.
>>>>>>
>>>>>>> -       __for_each_static_engine(e) {
>>>>>>> +       for_each_engine(e, i915) {
>>>>>>
>>>>>> Incorrect change that leads to compiler warnings and prevents the legacy
>>>>>> tests being listed by CI.
>>>>>>
>>>>>> I've given up and fixed it.
>>>>
>>>> Where's your version Chris?
>>>
>>> 21f4d0f72c0a3519270712565b2fe688157fc18b
>>
>> I suggest the below delta to a) not add more dependencies on the static e2 list and b) also cover I915_EXEC_BSD. I'll send a patch if you agree.
>>
>> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
>> index 8b9633b214ff..4d4b735d4936 100644
>> --- a/tests/i915/gem_ctx_persistence.c
>> +++ b/tests/i915/gem_ctx_persistence.c
>> @@ -759,7 +759,8 @@ static void smoketest(int i915)
>>   
>>   igt_main
>>   {
>> -       const struct intel_execution_engine2 *e;
>> +       const struct intel_execution_engine *e;
>> +       const struct intel_execution_engine2 *e2;
>>          int i915;
>>   
>>          igt_fixture {
>> @@ -792,66 +793,68 @@ igt_main
>>          igt_subtest("hang")
>>                  test_nohangcheck_hang(i915);
>>   
>> -       __for_each_static_engine(e) {
>> +       for (e = intel_execution_engines; e->name; e++) {
> 
> I don't mind too much, I just expected the __for_each_static_engine() to
> save on typing. But yeah, eb_ring().

__for_each_static_engine is the one which we are >.< this close 
eliminating after dynamic subtests and "i915/gem_engine_topology: 
Generate engine names based on class" but more importantly my vision of 
splitting legacy eb.flags and physical engine testing was that they are 
completely separate.

Sequence of steps along the lines of:

delete __for_each_static_engine

for_each_physical_engine -> __for_each_physical_engine and required test 
adaptations

delete for_each_physical_engine

rename __for_each_pyhsical_engine into for_each_physical_engine

(optional) rename for_each_engine into for_each_engine_flag (give or take)

(optional) add convenience macro to iterate legacy eb flags statically
(alternative) convert those into dynamic subtests and use 
for_each_engine_flags


> But much yuck on e and e2. Can we move them to separate functions to
> hide the eyesore?

Move to functions, how?

Regards,

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