List sync points to wait on in AsyncFlush message

List sync points to wait on in AsyncFlush message

This patch puts a vector of sync points to wait on in the AsyncFlush message, instead of embedding them in the command buffer. This allows the code to ensure the sync points have been inserted before they're waited on, which avoids a deadlock.

This may also reduce the number of glmakecurrent calls that happen, because it can now wait for all the dependencies of a set of commands to execute before making the context current.

BUG=373452

[jbauman, 2014-12-05 01:23:37]

jbauman@chromium.org changed reviewers:
+ sievers@chromium.org

[no sievers, 2014-12-05 20:35:07]

To fix the security issue couldn't we just detect the bad pattern entirely from
the service by storing which Stub a sync point is created from and not allowing
descheduling from GpuCommandBufferStub::OnWaitSyncPoint() if the stub is the
same?

Regarding the context switching: It's a tradeoff. It benefits use cases where
you wait on many sync points from a single flush, but do we really often do
that? On the other hand it hurts use cases where we insert some costly commands
that don't have a dependency without a flush before the client waits for a sync
point.

[jbauman, 2014-12-05 23:34:02]

On 2014/12/05 20:35:07, sievers wrote:
> To fix the security issue couldn't we just detect the bad pattern entirely
from
> the service by storing which Stub a sync point is created from and not
allowing
> descheduling from GpuCommandBufferStub::OnWaitSyncPoint() if the stub is the
> same?

There could be cycles between sync points created by two different command
buffers, so that may still cause problems. Also it would have to be per-channel.
> 
> Regarding the context switching: It's a tradeoff. It benefits use cases where
> you wait on many sync points from a single flush, but do we really often do
> that? On the other hand it hurts use cases where we insert some costly
commands
> that don't have a dependency without a flush before the client waits for a
sync
> point.

It can help even with one sync point, as it might still avoid context switches
if any commands are in the buffer before a context switch.. Generally if one
command buffer is waiting on another, the one that's being waited on can execute
immediately with no delay, so this shouldn't hurt throughput. With future sync
points there could be problems, but generally they shouldn't be used in a way
that would cause issues.

I guess on android MailboxManagerSync::PullTextureUpdates could be called
earlier, which could cause performance issues in some circumstances. One
solution is to keep the WaitSyncPoint in the command buffer, and use it just for
PullTextureUpdates. Or we could continue to do the wait in WaitSyncPoint, but
ignore sync points that aren't listed in the AsyncFlush, which would allow us to
prevent the deadlock.

[no sievers, 2014-12-06 00:04:05]

I meant that not inlining glWaitSyncPoint() also hurts this use case (and it's
not very obvious.. the fact that it's not inline essentially):

glDrawSomething()
glWaitSyncPoint(N)
glDoSomethingElseWithDependency()

Now glDrawSomething gets delayed according to the dependency it does not have.
The client would have to explicitly Flush after the Draw.

[no sievers, 2014-12-06 00:05:10]

On 2014/12/06 00:04:05, sievers wrote:
> I meant that not inlining glWaitSyncPoint() also hurts this use case (and it's
> not very obvious.. the fact that it's not inline essentially):
> 
> glDrawSomething()
> glWaitSyncPoint(N)
> glDoSomethingElseWithDependency()
> 
> Now glDrawSomething gets delayed according to the dependency it does not have.
> The client would have to explicitly Flush after the Draw.

plus we mostly try to minimize flushes in the critical path because of the IPC
delay.

[jbauman, 2014-12-06 00:13:22]

On 2014/12/06 00:05:10, sievers wrote:
> On 2014/12/06 00:04:05, sievers wrote:
> > I meant that not inlining glWaitSyncPoint() also hurts this use case (and
it's
> > not very obvious.. the fact that it's not inline essentially):
> > 
> > glDrawSomething()
> > glWaitSyncPoint(N)
> > glDoSomethingElseWithDependency()
> > 
> > Now glDrawSomething gets delayed according to the dependency it does not
have.
> > The client would have to explicitly Flush after the Draw.
> 
> plus we mostly try to minimize flushes in the critical path because of the IPC
> delay.

glDrawSomething() gets delayed, but whatever command buffer stub
glRetireSyncPoint(N) is executing on gets to go instead (ignoring other
SetScheduled(false), which never happen anymore). And since it's currently
essentially random which command buffer starts executing first, that doesn't
seem like an issue to me.

[no sievers, 2014-12-08 21:21:32]

On 2014/12/06 00:13:22, jbauman wrote:
> On 2014/12/06 00:05:10, sievers wrote:
> > On 2014/12/06 00:04:05, sievers wrote:
> > > I meant that not inlining glWaitSyncPoint() also hurts this use case (and
> it's
> > > not very obvious.. the fact that it's not inline essentially):
> > > 
> > > glDrawSomething()
> > > glWaitSyncPoint(N)
> > > glDoSomethingElseWithDependency()
> > > 
> > > Now glDrawSomething gets delayed according to the dependency it does not
> have.
> > > The client would have to explicitly Flush after the Draw.
> > 
> > plus we mostly try to minimize flushes in the critical path because of the
IPC
> > delay.
> 
> glDrawSomething() gets delayed, but whatever command buffer stub
> glRetireSyncPoint(N) is executing on gets to go instead (ignoring other
> SetScheduled(false), which never happen anymore). And since it's currently
> essentially random which command buffer starts executing first, that doesn't
> seem like an issue to me.

Ok, letting the service know about the dependencies definitely seems useful.
How would you feel about getting the best of both worlds and keeping the current
in-line sync-points for hard dependencies, the way they work now by
descheduling, and adding an up-front list of dependencies with the Flush like in
your patch, but using it for preemption (rather than descheduling decisions)?

That way a cmdbuffer with dependencies could still start running if there are no
other clients with work. We could also get fancier later and resolve
dependencies and schedule in the best order.