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

[Tvrtko Ursulin]

On 22/03/2021 16:24, Jason Ekstrand wrote:
> Ugh... timezones.
> 
> On Mon, Mar 22, 2021 at 10:31 AM 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).
> 
> You should look at my IGT patch set.  I'm not deleting any tests
> except those that explicitly test the clone API.  All the other tests
> which use cloning to save a few lines when constructing new contexts
> are updated to not require the cloning API.

I dare not mention the other IGT tree. There will be a plan needed since 
I fear much more usage will be found there.

[snip]

>>>> 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.
> 
> I said it was a "leak" because, from my git archeology, the best I
> could find for justification of doing it this way was that we already
> have a timeline object so why not expose it.  Same for the
> SINGLE_TIMELINE flag.  Is a "timeline" really an internal concept?
> No, not really.  It's pretty standard.  But intel_timeline is an
> internal thing and, while this doesn't give userspace an actual handle
> to it, it gives it more visibility than needed, IMO.

Cloning of timelines absolutely - I don't see a point for that. But I 
think there was no intent there. Rather it was just a consequence of 
striving for symmetry in the uapi.

But for the single timeline flag itself (so next patch in this series 
and it's commit message), when looked at within a single GEM context, I 
still really can't see the argument that it is leaking anything to 
userspace. Certainly not intel_timeline, which is also not even backend 
specific.

We seem to all agree timeline is just context:seqno, which was exposed 
to userpsace forever. For instance if the flag wasn't called "single 
timeline" but "implicit sync", "serial context", "ordered engines", 
whatever, would you still argue it is leaking struct intel_timeline out 
to userspace?

Regards,

Tvrtko