[igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Tvrtko Ursulin]

On 05/12/2019 10:09, Petri Latvala wrote:
> On Thu, Dec 05, 2019 at 09:54:25AM +0000, Tvrtko Ursulin wrote:
>>
>> On 05/12/2019 09:47, Andi Shyti wrote:
>>> Hi,
>>>
>>> On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
>>>> On 05/12/2019 09:01, Petri Latvala wrote:
>>>>> On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
>>>>>> On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
>>>>>>> On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> On 03/12/2019 05:11, Stuart Summers wrote:
>>>>>>>>> Align with gem_exec_basic and other tests using the newer
>>>>>>>>> engine query interface into i915 to enumerate active engines.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stuart Summers <stuart.summers at intel.com>
>>>>>>>>> ---
>>>>>>>>>      tests/i915/gem_ctx_isolation.c   | 4 ++--
>>>>>>>>>      tests/i915/gem_ctx_persistence.c | 2 +-
>>>>>>>>>      2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tests/i915/gem_ctx_isolation.c
>>>>>>>>> b/tests/i915/gem_ctx_isolation.c
>>>>>>>>> index 6aa27133..9435209e 100644
>>>>>>>>> --- a/tests/i915/gem_ctx_isolation.c
>>>>>>>>> +++ b/tests/i915/gem_ctx_isolation.c
>>>>>>>>> @@ -856,6 +856,7 @@ static unsigned int
>>>>>>>>> __has_context_isolation(int fd)
>>>>>>>>>      igt_main
>>>>>>>>>      {
>>>>>>>>> +	struct intel_execution_engine2 *e;
>>>>>>>>>      	unsigned int has_context_isolation = 0;
>>>>>>>>>      	int fd = -1;
>>>>>>>>> @@ -876,8 +877,7 @@ igt_main
>>>>>>>>>      		igt_skip_on(gen > LAST_KNOWN_GEN);
>>>>>>>>>      	}
>>>>>>>>> -	for (const struct intel_execution_engine2 *e =
>>>>>>>>> intel_execution_engines2;
>>>>>>>>> -	     e->name; e++) {
>>>>>>>>> +	__for_each_physical_engine(fd, e) {
>>>>>>>>>      		igt_subtest_group {
>>>>>>>>>      			igt_fixture {
>>>>>>>>>      				igt_require(has_context_isolati
>>>>>>>>> on & (1 << e->class));
>>>>>>>>> diff --git a/tests/i915/gem_ctx_persistence.c
>>>>>>>>> b/tests/i915/gem_ctx_persistence.c
>>>>>>>>> index d68431ae..30772159 100644
>>>>>>>>> --- a/tests/i915/gem_ctx_persistence.c
>>>>>>>>> +++ b/tests/i915/gem_ctx_persistence.c
>>>>>>>>> @@ -727,7 +727,7 @@ igt_main
>>>>>>>>>      	igt_subtest("hangcheck")
>>>>>>>>>      		test_nohangcheck_hostile(i915);
>>>>>>>>> -	__for_each_static_engine(e) {
>>>>>>>>> +	__for_each_physical_engine(i915, e) {
>>>>>>>>>      		igt_subtest_group {
>>>>>>>>>      			igt_fixture {
>>>>>>>>>      				gem_require_ring(i915, e-
>>>>>>>>>> flags);
>>>>>>>>>
>>>>>>>>
>>>>>>>> __for_each_static_engine is correct, at least if you don't want CI
>>>>>>>> folks go
>>>>>>>> look for their pitchforks. :) Same for the first hunk, everything
>>>>>>>> that
>>>>>>>> enumerates subtests needs to be static.
>>>>>>>>
>>>>>>>> Option 2, the preferred one - convert the tests to
>>>>>>>> igt_subtest_with_dynamic
>>>>>>>> and then you can use __for_each_physical_engine.
>>>>>>>
>>>>>>> Doesn't __for_each_physical_engine anyway have a hack for being
>>>>>>> called in that context?
>>>>>>>
>>>>>>> Btw, option 2 is
>>>>>>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44
>>>>>>>
>>>>>>> Currently blocking that effort is getting
>>>>>>> https://patchwork.freedesktop.org/series/70286/ into shape.
>>>>>>
>>>>>> Wait I'm a little confused, sorry for the naivete here. Does this mean
>>>>>> these kinds of changes are blocked on the above series? Or are you
>>>>>> saying I should go ahead and convert this to the dynamic subtests? Or
>>>>>> that we can move forward with the current approach and convert at a
>>>>>> later time?
>>>>>
>>>>> Option 2 is blocked by series 70286, sorry for the confusion. I don't
>>>>> mind slapping your patch in while waiting for that, since it's $RAND
>>>>> days away still to get there, if it fixes a problem you're having now.
>>>>
>>>> I think hack for static enumeration if in --list-subtest mode did not work
>>>> in practice. Andi, do you remember the details?
>>>
>>> __for_each_physical_engine checks for igt_only_list_subtests(),
>>> if so it behaves like __for_each_static_engine().
>>>
>>> Was this what you asked?
>>
>> Yes. Am I mis-remembering that there was a problem with this mandating to
>> keep using __for_each_static_engine directly for subtest enumeration? Maybe
>> I am..
> 
> __physical is used in a couple of tests to enumerate subtests already,
> and there are still issues caused by that. Not in the CI sense, the
> enumeration works, but can cause havoc when executing the tests. Don't
> have links available on hand now, sorry.
> 
> In a nutshell: It's ok for now. I don't like it, but it's used already
> and it's going away throughout soon. Feel free to use it until said
> otherwise.

Okay, sorry for the confusion:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko