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

[Tvrtko Ursulin]

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?!

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.

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.

Regards,

Tvrtko

> So I do think the assessment is accurate, albeit a bit on the terse side.
> Maybe we could quote just the entire thing here in the commit message.