Re: [Intel-gfx] [PATCH 02/28] drm/i915: use new iterator in i915_gem_object_wait_reservation

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 11, 2021 at 12:36:47PM +0100, Christian König wrote:
> Am 01.11.21 um 10:41 schrieb Tvrtko Ursulin:
> > 
> > On 28/10/2021 16:30, Daniel Vetter wrote:
> > > On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote:
> > > > Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin:
> > > > > 
> > > > > On 21/10/2021 12:06, Maarten Lankhorst wrote:
> > > > > > Op 21-10-2021 om 12:38 schreef Christian König:
> > > > > > > Am 21.10.21 um 12:35 schrieb Maarten Lankhorst:
> > > > > > > > From: Christian König <christian.koenig@amd.com>
> > > > > > > > 
> > > > > > > > Simplifying the code a bit.
> > > > > > > > 
> > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > > > > [mlankhorst: Handle timeout = 0 correctly, use new
> > > > > > > > i915_request_wait_timeout.]
> > > > > > > > Signed-off-by: Maarten Lankhorst
> > > > > > > > <maarten.lankhorst@linux.intel.com>
> > > > > > > 
> > > > > > > LGTM, do you want to push it or should I pick it up
> > > > > > > into drm-misc-next?
> > > > > > 
> > > > > > I think it can be applied to drm-intel-gt-next, after a backmerge.
> > > > > > It needs patch 1 too, which fixes
> > > > > > 
> > > > > > i915_request_wait semantics when used in dma-fence. It exports a
> > > > > > dma-fence compatible i915_request_wait_timeout function, used in
> > > > > > this patch.
> > > > 
> > > > What about the other i915 patches? I guess you then want to merge them
> > > > through drm-intel-gt-next as well.
> > > > 
> > > > > I don't think my open has been resolved, at least I haven't
> > > > > seen a reply
> > > > > from Daniel on the topic of potential for infinite waits
> > > > > with untrusted
> > > > > clients after this change. +Daniel
> > > > 
> > > > Please resolve that internally and let me know the result. I'm
> > > > fine to use
> > > > any of the possible approaches, I just need to know which one.
> > > 
> > > I thought I explained this in the patch set from Maarten. This isn't an
> > > issue, since the exact same thing can happen if you get interrupts and
> > > stuff.
> > 
> > Ah were you trying to point out all this time the infinite wait just got
> > moved from inside the "old" dma_resv_get_fences to the new iterator
> > caller?
> 
> At least I think so, yes. But Daniel needs to answer that.

Well maybe there's also an infinite wait inside the old
dma_resv_get_fences, what I mean is that when you have some signals
interrupting you, then the infinite wait is already there due to a retry
loop outside of the syscall even.

Anyway _any_ userspace which wants to use this wait on a shared bo and
waits to be safe against the other end adding more rendering has to use
something else (like the sync_file export ioctl on the dma-buf that Jason
typed). Trying to make this ioctl here against fence addition is just bs.

> > Regards,
> > 
> > Tvrtko
> > 
> > > 
> > > The only proper fix for bounding the waits is a) compositor grabs a
> > > stable
> > > set of dma_fence from the dma-buf through the proposed fence export
> > > ioctl
> > > b) compositor waits on that fence (or drm_syncobj).
> > > 
> > > Everything else is cargo-culted nonsense, and very much includes
> > > that igt
> > > patch that's floating around internally.
> > > 
> > > I can also whack this into drm-next if this is stuck in this silly
> > > bikeshed.
> 
> Can we move forward with those patches? I still don't see them in
> drm-misc-next.
> 
> I you want I can also pick Maartens modified version here up and send it out
> with the reset of the i915 patches once more.
> 
> Everything else is already pushed.

Please push to drm-misc-next or wherever (assuming CI is happy) and feel
free to add my ack.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch