On 22/03/2021 16:24, Jason Ekstrand wrote:
Ugh... timezones.
On Mon, Mar 22, 2021 at 10:31 AM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 22/03/2021 14:57, Daniel Vetter wrote:
On Mon, Mar 22, 2021 at 3:33 PM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
