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

[Chris Wilson]

On Sun, Sep 25, 2016 at 10:43:37PM +0200, Gustavo Padovan wrote:
> 2016-09-23 Christian König <christian.koenig at amd.com>:
> 
> > Am 23.09.2016 um 13:30 schrieb Gustavo Padovan:
> > > 2016-09-22 Christian König <christian.koenig at amd.com>:
> > > 
> > > > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
> > > > > 2016-09-22 Christian König <christian.koenig at amd.com>:
> > > > > 
> > > > > > Dropping the rest of the patch, cause that really doesn't make sense any
> > > > > > more.
> > > > > > 
> > > > > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
> > > > > > > > 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)
> > > > > > Exactly as I feared. Even if userspace polls with timeout=0 you still need
> > > > > > to call enable_signaling().
> > > > > > 
> > > > > > Otherwise you can run into a situation where userspace only uses timeout=0
> > > > > > and so never activates the signaling check in the driver.
> > > > > > 
> > > > > > This would in turn result in an endless loop on implementations where the
> > > > > > driver never signals fences on their own.
> > > > > Polling is optional, userspace may never call it. And DRM/KMS or GPU
> > > > > drivers will be doing fence_wait() themselves so signaling is enabled at
> > > > > some point.
> > > > No they won't. We have an use case where we clearly want to avoid that as
> > > > much as possible because and so the driver never calls enable_signaling() on
> > > > it's own.
> > > > 
> > > > Exposing this poll function to userspace without enabling signaling is a
> > > > clear NAK from my side.
> > > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
> > > that one then? It is already broken.
> > 
> > Yeah, that would probably a good idea. The AMD driver changes which really
> > rely on this aren't upstream yet, so if you point me to the commit hash I
> > could revert that as well when we send that out.
> > 
> > On the other hand the idea behind fence_is_signaled() is really that you
> > check the status multiple times after enabling signaling. So I would prefer
> > if you would revert this change preliminary.
> > 
> > Double checking this patch (and thinking about it a bit more) reveals that
> > it is most likely correct. So feel free to commit this one if it is still
> > needed for something.
> 
> It is this patch:
> 
> ecebca7 dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)
> 
> But if we revert it as you are a proposing we don't need my patch here
> anymore. However we would need to revert it now because it is broken.
> Shall I send a revert part?

Adding and propagating:

diff --git a/include/linux/fence.h b/include/linux/fence.h
index 0d763053f97a..ed5e88d2b664 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -56,6 +56,7 @@ struct fence_cb;
  *
  * FENCE_FLAG_SIGNALED_BIT - fence is already signaled
  * FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called*
+ * FENCE_FLAG_AUTO_SIGNAL_BIT - ops->signaled() is accurate by itself
  * FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
  * implementer of the fence for its own purposes. Can be used in different
  * ways by different fence implementers, so do not rely on this.
@@ -85,6 +86,7 @@ struct fence {
 enum fence_flag_bits {
        FENCE_FLAG_SIGNALED_BIT,
        FENCE_FLAG_ENABLE_SIGNAL_BIT,
+       FENCE_FLAG_AUTO_SIGNAL_BIT,
        FENCE_FLAG_USER_BITS, /* must always be last member */
 };
 
is trivial (I'm just stuck on naming and explaining it in kerneldoc). The
race Christian described in this patch is simple to prevent (on x86 all
that is missing is a compiler barrier, smp_mb__before_atomic() to be
generic), and it allows for external-fences to be as cheap as native
fences.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre