Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan <gustavo@padovan.org>]

2016-09-22 Christian König <christian.koenig@amd.com>:

> Am 22.09.2016 um 09:44 schrieb Gustavo Padovan:
> > Hi Christian,
> > 
> > 2016-09-21 Christian König <christian.koenig@amd.com>:
> > 
> > > Am 21.09.2016 um 13:36 schrieb Gustavo Padovan:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > If the fences in the fence_array signal on the fence_array does not have
> > > > signalling enabled num_pending will not be updated accordingly.
> > > > 
> > > > So when signaling is disabled check the signal of every fence with
> > > > fence_is_signaled() and then compare with num_pending to learn if the
> > > > fence_array was signalled or not.
> > > > 
> > > > If we want to keep the poll_does_not_wait optimization I think we need
> > > > something like this. It keeps the same behaviour if signalling is enabled
> > > > but tries to calculated the state otherwise.
> > > > 
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > First of all the patch is horrible wrong because fence_array_signaled() is
> > > called without any locks held. So you can run into a race condition between
> > > checking the fences here and enable signaling.
> > Yes. it can, but I don't think the race condition is actually a problem.
> > Maybe you have some use case that we are not seeing?
> 
> I'm not sure if that can really race, but if it does the check would return
> true while not all necessary fences are signaled yet.

How? If signaling is disabled num_pending is equal to the number of
fences (or 1 if signal_on_any) then we just check all fences. If all of
them are signaled then num_pending will be zero.

> 
> That would be really really bad for things like TTM where we just do a quick
> check in the page fault handler for example.
> 
> I need to double check if that really could be a problem.
> 
> > > Additional to that I'm not sure if that is such a good idea or not, cause
> > > fence_array_signaled() should be light weight and without calling
> > > enable_signaling there is not guarantee that fences will ever signal.
> > It is still lightweight for the case when signaling is enabled and
> > fences can signal with signaling enabled or disabled
> Nope that's not correct. The signaled callback is only optional.
> 
> See the comment on the fence_is_signaled function:
> >  * Returns true if the fence was already signaled, false if not. Since
> > this
> >  * function doesn't enable signaling, it is not guaranteed to ever return
> >  * true if fence_add_callback, fence_wait or fence_enable_sw_signaling
> >  * haven't been called before.

Right, I was with explicit fencing in mind, we only enable signaling
there if userspace polls.

> E.g. for example it is illegal to do something like
> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> doing this.
> 
> Could just be a misunderstanding, but the comments on your patch actually
> sounds a bit like somebody is trying to do exactly that.

I think the usecase in mind here is poll(fd, timeout=0)

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel