On Thu, Mar 25, 2021 at 10:48 AM Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > > > On 24/03/2021 17:18, Jason Ekstrand wrote: > > On Wed, Mar 24, 2021 at 6:36 AM Tvrtko Ursulin > > <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > >> > >> > >> On 24/03/2021 09:52, Daniel Vetter wrote: > >>> On Wed, Mar 24, 2021 at 09:28:58AM +0000, Tvrtko Ursulin wrote: > >>>> > >>>> On 23/03/2021 17:51, Jason Ekstrand wrote: > >>>>> This API is entirely unnecessary and I'd love to get rid of it. If > >>>>> userspace wants a single timeline across multiple contexts, they can > >>>>> either use implicit synchronization or a syncobj, both of which existed > >>>>> at the time this feature landed. The justification given at the time > >>>>> was that it would help GL drivers which are inherently single-timeline. > >>>>> However, neither of our GL drivers actually wanted the feature. i965 > >>>>> was already in maintenance mode at the time and iris uses syncobj for > >>>>> everything. > >>>>> > >>>>> Unfortunately, as much as I'd love to get rid of it, it is used by the > >>>>> media driver so we can't do that. We can, however, do the next-best > >>>>> thing which is to embed a syncobj in the context and do exactly what > >>>>> we'd expect from userspace internally. This isn't an entirely identical > >>>>> implementation because it's no longer atomic if userspace races with > >>>>> itself by calling execbuffer2 twice simultaneously from different > >>>>> threads. It won't crash in that case; it just doesn't guarantee any > >>>>> ordering between those two submits. > >>>>> > >>>>> Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical > >>>>> advantages beyond mere annoyance. One is that intel_timeline is no > >>>>> longer an api-visible object and can remain entirely an implementation > >>>>> detail. This may be advantageous as we make scheduler changes going > >>>>> forward. Second is that, together with deleting the CLONE_CONTEXT API, > >>>>> we should now have a 1:1 mapping between intel_context and > >>>>> intel_timeline which may help us reduce locking. > >>>> > >>>> Much, much better commit message although I still fail to understand where > >>>> do you see implementation details leaking out. So for me this is still > >>>> something I'd like to get to the bottom of. > > > > I didn't say "leaking". I said it's no longer API-visible. That's > > just true. It used to be a kernel object that userspace was unaware > > of, then we added SINGLE_TIMELINE and userspace now has some influence > > over the object. With this patch, it's hidden again. I don't get why > > that's confusing. > > I am definitely glad we moved on from leaking to less dramatic wording, > but it is still not API "visible" in so much struct file_operations * is > not user visible in any negative way when you do open(2), being just > implementation detail. But I give up. > > >>>> I would also mention the difference regarding fence context change. > > > > There are no fence context changes. The fence that we stuff in the > > syncobj is an i915 fence and the fence we pull back out is an i915 > > fence. A syncobj is just a fancy wrapper for a dma_buf pointer. > > Change is in the dma_fence->context. > > Current code single timeline is one fence context. With this patch that > is no longer the case. > > Not sure that it has any practical implications for the internal > dma_fence code like is_later checks , haven't analysed it. We merge fewer fences at the higher levels and rely more on the fence dependency tracking of the scheduler frontend to sort stuff out. > And sync fence info ioctl exposes this value to userspace which probably > has no practical implications. Unless some indirect effect when merging > fences. Userspace can use that to do fence merging. Which is always only a strict optimization. I'm not even sure whether Android does that or not. > Main point is that I think these are the things which need to be > discussed in the commit message. Yeah makes sense to add these as impact, next to the "we don't deal with races anymore" part. -Daniel > > >>>> And in general I would maintain this patch as part of a series which ends up > >>>> demonstrating the "mays" and "shoulds". > >>> > >>> I disagree. The past few years we've merged way too much patches and > >>> features without carefully answering the high level questions of > >>> - do we really need to solve this problem > >>> - and if so, are we really solving this problem in the right place > >>> > >>> Now we're quite in a hole, and we're not going to get out of this hole if > >>> we keep applying the same standards that got us here. Anything that does > >>> not clearly and without reservation the above two questions with "yes" > >>> needs to be removed or walled off, just so we can eventually see which > >>> complexity we really need, and what is actually superflous. > >> > >> I understand your general point but when I apply it to this specific > >> patch, even if it is simple, it is neither removing the uapi or walling > >> it off. So I see it as the usual review standard to ask to see what > >> "mays" and "shoulds" actually get us in concrete terms. > > > > Instead of focusing on the term "leak", let's focus on this line of > > the commit message instead: > > > >> Second is that, together with deleting the CLONE_CONTEXT API, > >> we should now have a 1:1 mapping between intel_context and > >> intel_timeline which may help us reduce locking. > > > > Now, I've not written any patches yet which actually reduce the > > locking. I can try to look into that today. I CC'd Maarten on the > > first send of this because I was hoping he would have good intuition > > about this. It may be that this object will always have to require > > some amount of locking if the scheduler has to touch them in parallel > > with other stuff. What I can say concretely, however, is that this > > does reduce the sharing of an internal object even if it doesn't get > > rid of it completely. The one thing that is shared all over the place > > with this patch is a syncobj which is explicitly designed for exactly > > this sort of case and can be used and abused by as many threads as > > you'd like. That seems like it's going in the right direction. > > > > I can further weasel-word the commit message to make the above more > > prominent if you'd like. > > I am not interested in making you weasel-word anything but reaching a > consensus and what is actually true and accurate. > > Regards, > > Tvrtko -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
