[igt-dev] [PATCH i-g-t] lib/i915: Return actual submission method from gem_submission_method

[Tvrtko Ursulin]

On 19/10/2021 08:16, John Harrison wrote:
> On 10/18/2021 17:12, Dixit, Ashutosh wrote:
>> On Mon, 18 Oct 2021 16:59:43 -0700, Dixit, Ashutosh wrote:
>>> On Mon, 18 Oct 2021 16:39:40 -0700, John Harrison wrote:
>>>> On 10/15/2021 17:23, Ashutosh Dixit wrote:
>>>>> gem_submission_method() purports to return the currently used 
>>>>> submission
>>>>> method by the driver, as evidenced by its callers. Therefore remove 
>>>>> the
>>>>> GEM_SUBMISSION_EXECLISTS flag when GuC submission is detected.
>>>>>
>>>>> This also fixes gem_has_execlists() to match its description, 
>>>>> previously
>>>>> gem_has_execlists() would return true even if GuC submission was 
>>>>> actually
>>>>> being used in the driver.
>>>>>
>>>>> Reported-by: John Harrison <john.c.harrison at intel.com>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>>>>> ---
>>>>>    lib/i915/gem_submission.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
>>>>> index 2627b802cfb..b037d04cc4a 100644
>>>>> --- a/lib/i915/gem_submission.c
>>>>> +++ b/lib/i915/gem_submission.c
>>>>> @@ -86,7 +86,7 @@ unsigned gem_submission_method(int fd)
>>>>>         return 0;
>>>>>         if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
>>>>> -        flags |= GEM_SUBMISSION_GUC | GEM_SUBMISSION_EXECLISTS;
>>>>> +        flags |= GEM_SUBMISSION_GUC;
>>>>>         goto out;
>>>>>     }
>>>>>
>>>> Looks good to me, but as per the comments in the other thread, this 
>>>> might
>>>> have unintended side effects. Have you gone through all instances of 
>>>> the
>>>> submission query usages to check how it is used and whether this 
>>>> will break
>>>> something? E.g. if something is explicitly testing for execlist 
>>>> support to
>>>> mean 'something better than ring buffer' then it would now fail.
>>> There are at present just these two instances of gem_has_execlists:
>>>
>>> *** tests/i915/gem_ctx_shared.c:
>>> disjoint_timelines[162]        igt_require(gem_has_execlists(i915));
>>>
>>> *** tests/i915/gem_watchdog.c:
>>> virtual[225]                   igt_require(gem_has_execlists(i915));
>>>
>>> I'll try to see if I can figure out if they will be affected in any
>>> way.
>> Actually, earlier these tests were running for both execlists and guc
>> submission. If they are meant to run even for guc then they will now skip
>> in the guc case after this patch.
>>
>> Therefore I am thinking the exact equivalent of what was going on earlier
>> is this condition:
>>     igt_require(gem_has_execlists() | gem_has_guc_submission());
>>
>> So that's all that is needed now correct? But what I am not sure of 
>> yet is
>> whether they were meant to run for guc too or just execlists.
> Not sure what the ctx_shared one is about. Possibly that should be some 
> kind of 'has_contexts()' test (although I thought ring submission did 
> have context support post Gen6?). The watchdog one I would guess should 
> be 'has_watchdog()' unless this is some more specific subtest? In 
> general, it sounds like the 'has_execlists() || has_guc()' test is 
> really 'has_scheduler()', but it really is test specific I think.

Yes, gem_watchdog should keep running both with execlists and GuC. 
Perhaps most correct would be to make it skip if set_load_balance fails 
with an errno which indicates creating virtual engines is not supported?

Gem_ctx_shared/disjoint-timlines could perhaps use gem_uses_ppgtt() (for 
create vm) and gem_scheduler_enabled() for context re-ordering.

Regards,

Tvrtko