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

Tvrtko Ursulin
2019-12-05T09:22:59Z

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@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?

Regards,

Tvrtko