gpu: Allow fences to check whether a flush has occurred

gpu: Allow fences to check whether a flush has occurred

This skips waiting on a fence that was created but never committed.

BUG=352419
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258122

[no sievers, 2014-03-14 22:48:27]

How's this?

I could plumb it up to SwapBuffers() also. Would allow us to imply some flushes
(potentially for the read fences also) without the risk.

[piman, 2014-03-14 23:34:44]

I'm not sure how this solves anything.

From my reading of it, you're essentially asserting on the GPU side that the
client does the right thing, always calling glFlush after a write before the
texture is read from another context (in another GLShareGroup).

If the client doesn't do that, in debug you assert, in release you (may) hang.

Did I miss something, an implicit glFlush somewhere that ensures that the fence
is always flushed?

Because otherwise it's not ok:
1- malicious untrusted code can trigger this (e.g. NaCl)
2- even our synchronoization bugs go form something relatively harmless (maybe
an out-of-date texture if we're unlucky) to a GPU hang that's very hard to debug
in production

[piman, 2014-03-15 00:18:40]

Ok, after discussion, maybe we should do this for now, since it's strictly
better than before, although there are edge cases that are hard to reason about
- e.g. (for posterity)

context1 writes to (shared) texture
context1 Flush()
context1 InsertSyncPoint()
context1 "writes" to (shared) texture (but may not actually modify it, e.g.
color mask)
context1 ShallowFlush()

context2 WaitSyncPoint()
context2 reads texture

In that case the second write will overwrite the fence (that was properly
flushed) by one that was not flushed. Racily, that means context2 may skip the
ServerWait and not "see" even the first write.

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_context.cc#newc...
ui/gl/gl_context.cc:36: base::subtle::NoBarrier_AtomicExchange(&state_, 1);
I'm not convinced whether this is correct or not.
Maybe we can reuse base::CancellationFlag that has the same semantics, but is
well tested?

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc
File ui/gl/gl_fence.cc (right):

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcode40
ui/gl/gl_fence.cc:40: if (flush_event_->IsSignaled()) {
(here an below), you also need to check for flush_event_ being NULL (for the
create-with-flush case).

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcode44
ui/gl/gl_fence.cc:44: NOTREACHED();
Maybe remove the NOTREACHED. It can be reached from untrusted clients.

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcod...
ui/gl/gl_fence.cc:165: if (!gfx::GLContext::GetCurrent()) {
nit: simply DCHECK(gfx::GLContext::GetCurrent()) ?

[no sievers, 2014-03-19 01:25:21]

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_context.cc#newc...
ui/gl/gl_context.cc:36: base::subtle::NoBarrier_AtomicExchange(&state_, 1);
On 2014/03/15 00:18:40, piman wrote:
> I'm not convinced whether this is correct or not.
> Maybe we can reuse base::CancellationFlag that has the same semantics, but is
> well tested?

Done.

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc
File ui/gl/gl_fence.cc (right):

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcode40
ui/gl/gl_fence.cc:40: if (flush_event_->IsSignaled()) {
On 2014/03/15 00:18:40, piman wrote:
> (here an below), you also need to check for flush_event_ being NULL (for the
> create-with-flush case).

Done.

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcode44
ui/gl/gl_fence.cc:44: NOTREACHED();
On 2014/03/15 00:18:40, piman wrote:
> Maybe remove the NOTREACHED. It can be reached from untrusted clients.

Done.

https://codereview.chromium.org/197563003/diff/20001/ui/gl/gl_fence.cc#newcod...
ui/gl/gl_fence.cc:165: if (!gfx::GLContext::GetCurrent()) {
On 2014/03/15 00:18:40, piman wrote:
> nit: simply DCHECK(gfx::GLContext::GetCurrent()) ?

Done.

[piman, 2014-03-19 03:42:24]

lgtm

[no sievers, 2014-03-19 19:44:56]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2014-03-19 19:45:06]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2014-03-19 19:45:34]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-03-19 19:46:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/197563003/60001

[commit-bot: I haz the power, 2014-03-19 22:09:45]

Change committed as 258122