[Intel-gfx] [PATCH 3/4] drm/i915: Drop the CONTEXT_CLONE API

[Tvrtko Ursulin]

On 22/03/2021 16:43, Daniel Vetter wrote:
> On Mon, Mar 22, 2021 at 4:31 PM Tvrtko Ursulin
> <tvrtko.ursulin at linux.intel.com> wrote:
>>
>>
>> On 22/03/2021 14:57, Daniel Vetter wrote:
>>> On Mon, Mar 22, 2021 at 3:33 PM Tvrtko Ursulin
>>> <tvrtko.ursulin at linux.intel.com> wrote:
>>>>
>>>>
>>>> On 22/03/2021 14:09, Daniel Vetter wrote:
>>>>> On Mon, Mar 22, 2021 at 11:22:01AM +0000, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 19/03/2021 22:38, Jason Ekstrand wrote:
>>>>>>> This API allows one context to grab bits out of another context upon
>>>>>>> creation.  It can be used as a short-cut for setparam(getparam()) for
>>>>>>> things like I915_CONTEXT_PARAM_VM.  However, it's never been used by any
>>>>>>> real userspace.  It's used by a few IGT tests and that's it.  Since it
>>>>>>> doesn't add any real value (most of the stuff you can CLONE you can copy
>>>>>>> in other ways), drop it.
>>>>>>
>>>>>> No complaints to remove if it ended up unused outside IGT. Latter is a _big_
>>>>>> problem though, since it is much more that a few IGT tests. So I really
>>>>>> think there really needs to be an evaluation and a plan for that (we don't
>>>>>> want to lose 50% of the coverage over night).
>>>>>>
>>>>>>> There is one thing that this API allows you to clone which you cannot
>>>>>>> clone via getparam/setparam: timelines.  However, timelines are an
>>>>>>> implementation detail of i915 and not really something that needs to be
>>>>>>
>>>>>> Not really true timelines are i915 implementation detail. They are in fact a
>>>>>> dma-fence context:seqno concept, nothing more that than. I think you are
>>>>>> probably confusing struct intel_timeline with the timeline wording in the
>>>>>> uapi. Former is i915 implementation detail, but context:seqno are truly
>>>>>> userspace timelines.
>>>>>
>>>>> I think you're both saying the same thing and talking a bit past each
>>>>> another.
>>>>>
>>>>> Yes the timeline is just a string of dma_fence, that's correct. Now
>>>>> usually if you submit batches with execbuf, we have 3 ways to synchronize
>>>>> concurrent submission: implicit sync, sync_file and drm_syncob. They all
>>>>> map to different needs in different protocols/render apis.
>>>>>
>>>>> Now in one additional case the kernel makes sure that batchbuffers are
>>>>> ordered, and that's when you submit them to the same hw ctx. Because
>>>>> there's only 1 hw context and you really can't have batchbuffers run on
>>>>> that single hw context out of order. That's what the timeline object we
>>>>> talk about here is. But that largely is an internal implementation detail,
>>>>> which happens to also use most/all the same infrastructure as the
>>>>> dma_fence uapi pieces above.
>>>>>
>>>>> Now the internal implementation detail leaking here is that we exposed
>>>>> this to userspace, without there being any need for this. What Jason
>>>>> implements with syncobj in the next patch is essentially what userspace
>>>>> should have been using for cross-engine sync. media userspace doesn't care
>>>>> about interop with winsys/client apis, so they equally could have used
>>>>> implicit sync or sync_file here (which I think is the solution now for the
>>>>> new uapi prepped internally), since they all are about equally powerful
>>>>> for stringing batchbuffers together.
>>>>
>>>> Are you saying we exposed a single timeline of execution per hw context
>>>> via the single timeline flag?!
>>>
>>> Nope.
>>>
>>>> Timelines of execution were always exposed. Any "engine" (ring
>>>> previously) in I915_EXEC_RING_MASK was a single timeline of execution.
>>>> It is completely the same with engine map engines, which are also
>>>> different indices into I915_EXEC_RING_MASK space.
>>>>
>>>> Userspace was aware of these timelines forever as well. Media was
>>>> creating multiple contexts to have multiple timelines (so parallelism).
>>>> Everyone knew that engine-hopping submissions needs to be either
>>>> implicitly or explicitly synchronised, etc.
>>>
>>> Yup, I think we're saying the same thing here.
>>>
>>>> So I really don't see that we have leaked timelines as a concept *now*.
>>>> What the patch has exposed to userspace is a new way to sync between
>>>> timelines and nothing more.
>>>
>>> We've leaked it as something you can now share across hw context.
>>
>> Okay so we agree on most things but apparently have different
>> definitions of what it means to leak internal implementation details.
>>
>> While at the same time proof that we haven't leaked the internal
>> implementation details is that Jason was able to implement the single
>> timeline flag with a drm syncobj at the execbuf top level. (Well mostly,
>> ignoring the probably inconsequential difference of one vs multiple
>> fence contexts.)
> 
> It's not a matching implementation. It's only good enough for what
> media needs, and essentially what media should have done to begin
> with.
> 
> There's substantially different behaviour between SINGLE_TIMELINE and
> what Jason has done here when you race concurrent execbuf calls:
> Former guarantees total ordering, the latter doesn't even try. They
> are not the same thing, but luckily userspace doesn't care about that
> difference.

Sounds like a very important difference to stress in the commit message.

Secondly, I am unclear whether we have agreement on whether the single 
timeline flag is leaking implementation details of the execlists 
scheduler to userspace or not?

Regards,

Tvrtko

> 
> Aside, just to make sure this wont get lost: I do agree that we should
> only allow this up to maybe ADL, and reject it on anything new (maybe
> including dg1 while we're at it, since the pci ids for that aren't
> even close to upstream yet).
> -Daniel
> 
>>> Which is possible because of how it's internally implemented (I think
>>> load balancer relies on that), but not really a synchronization
>>
>> Virtual engine is a single timeline by definition and it is still that
>> regardless of the implementation details (execlists or GuC, in both
>> cases it is a single hardware context and a single timeline).
>>
>>> primitive we want to export as such to userspace. We have other
>>> interfaces and concepts for that.
>>
>> Yes, that is the only point to argue IMO. We can say it wasn't needed
>> and should have been avoided, but I still maintain we can't really say
>> we leaked anything backend specific to userspace via it.
>>
>> Regards,
>>
>> Tvrtko
> 
> 
>