[Intel-gfx] [PATCH 1/1] Let userspace know if they can trust timeslicing by including it as part of the I915_PARAM_HAS_SCHEDULER::I915_SCHEDULER_CAP_TIMESLICING

[Tvrtko Ursulin]

On 27/05/2021 11:27, Daniel Vetter wrote:
> On Thu, May 27, 2021 at 11:22:16AM +0100, Tvrtko Ursulin wrote:
>>
>> On 27/05/2021 11:13, Daniel Vetter wrote:
>>> On Wed, May 26, 2021 at 11:20:13AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 25/05/2021 15:47, Daniel Vetter wrote:
>>>>> On Tue, May 25, 2021 at 03:19:47PM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> + dri-devel as per process
>>>>>>
>>>>>> On 25/05/2021 14:55, Tejas Upadhyay wrote:
>>>>>>> v2: Only declare timeslicing if we can safely preempt userspace.
>>>>>>
>>>>>> Commit message got butchered up somehow so you'll need to fix that at some
>>>>>> point.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>> Fixes: 8ee36e048c98 ("drm/i915/execlists: Minimalistic timeslicing")
>>>>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/i915/gt/intel_engine_user.c | 1 +
>>>>>>>      include/uapi/drm/i915_drm.h                 | 1 +
>>>>>>>      2 files changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>>> index 3cca7ea2d6ea..12d165566ed2 100644
>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>>> @@ -98,6 +98,7 @@ static void set_scheduler_caps(struct drm_i915_private *i915)
>>>>>>>      		MAP(HAS_PREEMPTION, PREEMPTION),
>>>>>>>      		MAP(HAS_SEMAPHORES, SEMAPHORES),
>>>>>>>      		MAP(SUPPORTS_STATS, ENGINE_BUSY_STATS),
>>>>>>> +		MAP(TIMESLICE_BIT, TIMESLICING),
>>>>>>>      #undef MAP
>>>>>>>      	};
>>>>>>>      	struct intel_engine_cs *engine;
>>>>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>>>>> index c2c7759b7d2e..af2212d6113c 100644
>>>>>>> --- a/include/uapi/drm/i915_drm.h
>>>>>>> +++ b/include/uapi/drm/i915_drm.h
>>>>>>> @@ -572,6 +572,7 @@ typedef struct drm_i915_irq_wait {
>>>>>>>      #define   I915_SCHEDULER_CAP_PREEMPTION	(1ul << 2)
>>>>>>>      #define   I915_SCHEDULER_CAP_SEMAPHORES	(1ul << 3)
>>>>>>>      #define   I915_SCHEDULER_CAP_ENGINE_BUSY_STATS	(1ul << 4)
>>>>>>> +#define   I915_SCHEDULER_CAP_TIMESLICING	(1ul << 5)
>>>>>
>>>>> Since this is uapi I think we should at least have some nice kerneldoc
>>>>> that explains what exactly this is, what for (link to userspace) and all
>>>>> that. Ideally also minimally filing in the gaps in our uapi docs for stuff
>>>>> this references.
>>>>
>>>> IIUC there is no userspace apart from IGT needing it not to fail scheduling
>>>> tests on ADL.
>>>>
>>>> Current tests use "has preemption + has semaphores" as a proxy to answer the
>>>> "does the kernel support timeslicing" question. This stops working with the
>>>> Guc backend because GuC decided not to support semaphores (for reasons yet
>>>> unknown, see other thread), so explicit "has timeslicing" flag is needed in
>>>> order for tests to know that GuC is supposed to support timeslicing, even if
>>>> it doesn't use semaphores for inter-ring synchronisation.
>>>
>>> Since this if for igt only: Cant we do just extend the check in igt with
>>> an || GEN >= 12? I really hope that our future hw will continue to support
>>> timeslicing ...
>>
>> Not the gen 12 check, but possible I think. Explicit feature test would be better, but if definitely not allowed then along the lines of:
>>
>> has_timeslicing =
>> 	(has_preemption && has_semaphores) || uses_guc_submission;
> 
> That works too. Otoh what exactly is the "uses guc submission" flag and
> why do we have that? I've seen media use it as a stand-in for "does the
> kernel want bonded or parallel ctx?". Maybe another thing to check.

IGT derives it from the enable_guc modparam and logs it during test 
start (some tests). It's called actuall gem_submission_method(_..). It's 
useful to have as long as there are platforms where submission backend 
can be picked at runtime. Afterwards not so much I guess.

Regards,

Tvrtko

> Another option, if you really think the feature flag is the best approach
> (because future hw will drop timeslicing for some reason), then debugfs is
> the place of igt-only api.
> -Daniel
> 
>>
>> Regards,
>>
>> Tvrtko
>>> Also if it's not there yet, a shared helper to check for that (like we're
>>> adding for relocations and stuff like that right now).
>>> -Daniel
>>>
>