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

[Tvrtko Ursulin]

On 30/04/2021 14:07, Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
> <tvrtko.ursulin at linux.intel.com> wrote:
>> 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.
> 
> They filter out no-op changes. I just meant that from look at
> compute-runtime, it seems like sseu can change whenever.

i915 does it as well for good measure - since the penalty is global we 
have to. So I guess they don't know when VME block will be used ie it is 
per batch.

Regards,

Tvrtko