2016-09-22 Christian König <christian.koenig at amd.com>: > Am 22.09.2016 um 09:44 schrieb Gustavo Padovan: > > Hi Christian, > > > > 2016-09-21 Christian König <christian.koenig at amd.com>: > > > > > Am 21.09.2016 um 13:36 schrieb Gustavo Padovan: > > > > From: Gustavo Padovan <gustavo.padovan at 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 at collabora.co.uk> > > > > Reviewed-by: Chris Wilson <chris at 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
