[Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

[Tvrtko Ursulin]

On 30/04/2021 13:30, Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
> <tvrtko.ursulin at linux.intel.com> wrote:
>> On 30/04/2021 07:53, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>
>>>> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>>>>>
>>>>> On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
>>>>>> On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>>>>>>> On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
>>>>>>>> On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>>>>>>>>>> +     ret = set_proto_ctx_param(file_priv, pc, args);
>>>>>>>>>
>>>>>>>>> I think we should have a FIXME here of not allowing this on some future
>>>>>>>>> platforms because just use CTX_CREATE_EXT.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>> +     if (ret == -ENOTSUPP) {
>>>>>>>>>> +             /* Some params, specifically SSEU, can only be set on fully
>>>>>>>>>
>>>>>>>>> I think this needs a FIXME: that this only holds during the conversion?
>>>>>>>>> Otherwise we kinda have a bit a problem me thinks ...
>>>>>>>>
>>>>>>>> I'm not sure what you mean by that.
>>>>>>>
>>>>>>> Well I'm at least assuming that we wont have this case anymore, i.e.
>>>>>>> there's only two kinds of parameters:
>>>>>>> - those which are valid only on proto context
>>>>>>> - those which are valid on both (like priority)
>>>>>>>
>>>>>>> This SSEU thing looks like a 3rd parameter, which is only valid on
>>>>>>> finalized context. That feels all kinds of wrong. Will it stay? If yes
>>>>>>> *ugh* and why?
>>>>>>
>>>>>> Because I was being lazy.  The SSEU stuff is a fairly complex param to
>>>>>> parse and it's always set live.  I can factor out the SSEU parsing
>>>>>> code if you want and it shouldn't be too bad in the end.
>>>>>
>>>>> Yeah I think the special case here is a bit too jarring.
>>>>
>>>> I rolled a v5 that allows you to set SSEU as a create param.  I'm not
>>>> a huge fan of that much code duplication for the SSEU set but I guess
>>>> that's what we get for deciding to "unify" our context creation
>>>> parameter path with our on-the-fly parameter path....
>>>>
>>>> You can look at it here:
>>>>
>>>> https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
>>>
>>> Hm yeah the duplication of the render engine check is a bit annoying.
>>> What's worse, if you tthrow another set_engines on top it's probably
>>> all wrong then. The old thing solved that by just throwing that
>>> intel_context away.
>>>
>>> You're also not keeping the engine id in the proto ctx for this, so
>>> there's probably some gaps there. We'd need to clear the SSEU if
>>> userspace puts another context there. But also no userspace does that.
>>>
>>> Plus cursory review of userspace show
>>> - mesa doesn't set this
>>> - compute sets its right before running the batch
>>> - media sets it as the last thing of context creation
>>
>> Noticed a long sub-thread so looked inside..
>>
>> SSEU is a really an interesting one.
>>
>> For current userspace limiting to context creation is fine, since it is
>> only allowed for Icelake/VME use case. But if you notice the comment inside:
>>
>>                  /* ABI restriction - VME use case only. */
>>
>> It is a hint there was, or could be, more to this uapi than that.
>>
>> And from memory I think limiting to creation time will nip the hopes
>> media had to use this dynamically on other platforms in the bud. So not
>> that good really. They had convincing numbers what gets significantly
>> better if we allowed dynamic control to this, just that as always, open
>> source userspace was not there so we never allowed it. However if you
>> come up with a new world order where it can only be done at context
>> creation, as said already, the possibility for that improvement (aka
>> further improving the competitive advantage) is most likely dashed.
> 
> Hm are you sure that this is create-time only? media-driver uses it
> like that, but from my checking compute-runtime updates SSEU mode
> before every execbuf call. So it very much looked like we have to keep
> this dynamic.

Ah okay, I assumed it's more of the overall drive to eliminate 
set_param. If sseu set_param stays then it's fine for what I had in mind.

> Or do you mean this is defacto dead code? this = compute setting it
> before every batch I mean here.

No idea, wasn't aware of the compute usage.

Before every execbuf is not very ideal though since we have to inject a 
foreign context operation to update context image, which means stream of 
work belonging to the context cannot be coalesced (assuming it could to 
start with). There is also a hw cost to reconfigure the sseu which adds 
latency on top.

Anyway, I was only aware of the current media usage, which is static as 
you say, and future/wishlist media usage, which would be dynamic, but a 
complicated story to get right (partly due downsides mentioned in the 
previous paragraph mean balancing benefit vs cost of dynamic sseu is not 
easy).

Regards,

Tvrtko