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

[John Harrison]

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.

John.