[igt-dev] [PATCH i-g-t] lib/i915: Fix gem_has_execlists to match description

[John Harrison]

On 10/19/2021 01:53, Tvrtko Ursulin wrote:
> On 19/10/2021 00:40, John Harrison wrote:
>> On 10/18/2021 00:43, Tvrtko Ursulin wrote:
>>> On 16/10/2021 00:57, John Harrison wrote:
>>>> On 10/15/2021 16:39, Dixit, Ashutosh wrote:
>>>>> On Fri, 15 Oct 2021 16:42:12 -0700, John Harrison wrote:
>>>>>> On 10/15/2021 16:38, Ashutosh Dixit wrote:
>>>>>>> Driver is using execlists if it is not using GuC submission.
>>>>>>> GEM_SUBMISSION_EXECLISTS flag indicates if execlist submission is
>>>>>>> available, not if it is being used by the driver.
>>>>>>>
>>>>>>> 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..4312a73bd85 100644
>>>>>>> --- a/lib/i915/gem_submission.c
>>>>>>> +++ b/lib/i915/gem_submission.c
>>>>>>> @@ -153,7 +153,7 @@ bool gem_has_semaphores(int fd)
>>>>>>>     */
>>>>>>>    bool gem_has_execlists(int fd)
>>>>>>>    {
>>>>>>> -    return gem_submission_method(fd) & GEM_SUBMISSION_EXECLISTS;
>>>>>>> +    return !gem_has_guc_submission(fd);
>>>>>> Don't we use ring submission on older platforms?
>>>>> Hmm, I wasn't aware that there was yet another submission method :/
>>>>>
>>>>> So execlist submission is only available for gen >= 8 as the code 
>>>>> says, in
>>>>> case anyone knows?
>>>> Yes. Execlists was new feature of the hardware not all that long 
>>>> ago (ELSP and all that). Gen8 sounds plausible.
>>>
>>> Yes Gen8+.
>>>
>>>> Of course, it all depends why the code is asking the question? Does 
>>>> it really need to know what the submission hardware is? Or is it 
>>>> actually asking about the scheduling algorithm (as is the case for 
>>>> gem_exec_fair). Or something else entirely? The scheduler question 
>>>> is going to change again when we move to using the DRM scheduler 
>>>> instead of our own private one in the execlist code. Currently 
>>>> though, I would assume we use the execlist scheduler for ring 
>>>> submission but certainly not for GuC submission (because the 
>>>> scheduler is in the hardware).
>>>
>>> Nope, no scheduling with ringbuffer backend.
>>>
>>> There is I915_PARAM_HAS_SCHEDULER and respective IGT helpers to 
>>> query what kind of scheduling is supported on the device in question.
>>>
>>> It will be on a test per test basis why it is asking the question 
>>> and what would be the appropriate test.
>>>
>> Are you saying that we need to add an extra flag (or even enum field) 
>> to say what type of scheduler is supported? As in 'fair', 'round 
>> robin', 'DRM', etc.?
>
> No strong opinion. I was just saying what exists today,
Okay. I was also thinking that extending/adding a UAPI for this is not 
going to fly anyway because we have no UMD use case for it. It would be 
purely for the IGT. I think just skipping on GuC is the best way forward 
for the moment. As/when we have a customer requirement for a different 
scheduling algorithm, the test can be re-visited and updated to match.


> and noticing gem_exec_fair does not only fail with the GuC.
Not my problem, guv ;)

My understanding is that it is supposed to pass in execlist mode. As to 
whether any failures are test issues or KMD issues I don't know. But 
that would need to be investigated by someone who knows the test and/or 
the execlist scheduler. I am neither of those people.

>
> If a new feature test is needed, which is not possible via all that is 
> currently available, then the question is how/where to expose the data.
>
> But in principle is it not okay even to have a failing test since ci 
> known failure list takes care of that? Because I know there is a 
> customer requirement for fair scheduling already so it sounds better 
> to highlight in the CI matrix we are not there yet.
There *was* a customer requirement. I don't believe that exists any 
more. At least, not the specific customer on the specific product that 
was the reason for adding this test.

For execlist failures, as above. My view is to leave them as sporadic 
failures and whoever is assigned to fix them can fix them. For GuC 
submission, it is a known product difference. No need to track that as 
an IGT failure. It is tracked at the program management level as part of 
the feature list for each platform / requirement list for each customer.

John.


>
> Regards,
>
> Tvrtko
>
>> I'm not seeing any scheduling _CAP flag that would be relevant to 
>> differentiating between the GuC and the execlist scheduler 
>> implementations.
>>
>> John.
>>
>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>
>>>> John.
>>>>
>>>>>
>>>>>> Isn't the better fix to just not set the EXECLIST flag when GuC
>>>>>> submission is detected?
>>>>> Yes, let me post this patch too. Thanks!
>>>>
>>