Implemented new fence syncs which replaces the old sync points.

Implemented new fence syncs which replaces the old sync points.

The new GL_CHROMIUM_sync_point proposal has been implemented now. The
old functions are all different enough that no names were overloaded,
so all of the old functionality still exists. This way, we can
incrementally move over call sites of the old functions to use the new
ones.

R=jam@chromium.org, piman@chromium.org
BUG=514815
Committed: https://crrev.com/5c38a030b88f43b57f404dc60d777572b33ea3c4
Cr-Commit-Position: refs/heads/master@{#352901}

[David Yen, 2015-09-10 21:06:13]

One thing I was unsure of was what the channel_id and route_id should be for
CommandBufferService channels, I just set them to be 0 but that seems wrong for
any case where we have more than 1 command buffer.

+jam for mojo changes. I am also getting this presubmit warning, do you have any
insight into this?

** Presubmit Warnings **
When updating or adding third party code the appropriate
'README.chromium' file should also be updated with the correct
version and package information.
 
third_party/mojo/src/mojo/public/c/gles2/gles2_call_visitor_chromium_sync_point_autogen.h

[piman, 2015-09-10 23:55:31]

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

[piman, 2015-09-10 23:55:32]

+sievers for the interaction with InProcessCommandBuffer
(not sure if you saw the design doc, if not, dyen@ can share with you).

Overall the pieces are reasonable. But I think we need to make sure what we do
allows synchronization between in-process (InProcessCommandBuffer) and
cross-process (GpuCommandBufferStub and friends). I list a few things that
should allow us to do that.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:380: if (channel_id)
maybe just DCHECK(channel_id) and DCHECK(route_id), the callers always have
valid pointers to both.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1048:
release_channel->GetProcessedOrderNum();
I think it would be good to start reasoning in terms of streams rather than
channels. Once we have multiple streams within a channel, each of them will have
a separate processed order number (because each IPC corresponds to a unique
stream), and the goal of this is to allow ordering between different streams.

So I figure you could have a GetProcessedOrderNum on GpuCommandBufferStub, that
queries the state on the GpuChannel using its stream_id. That state would then
go onto GpuChannel::StreamState.
At least putting the API in place even if the state is updated for all streams
at once for now, so that transitioning to unordered stream execution is easier.


Actually, that would go hand-in-hand with the second thing: we need to deal with
the fact that on at least Android we need to combine cross-process and
in-process command buffers, and synchronization between them. What that means is
that:
- you can't assume here that what you're waiting for is a
GpuChannel/GpuCommandBufferStub
- vice versa, in a InProcessCommandBuffer, you can't assume you're waiting on
another InProcessCommandBuffer.

What I think we would want is an interface ("FenceSyncClient"?), that provides
at least GetProcessedOrderNum, GetUnprocessedOrderNum, ValidateFenceSyncRelease,
and a GetFenceSyncManager()  (or directly
IsFenceSyncReleased/AddFenceSyncCallback to satisfy the Law of Demeter), that
would be implemented by both GpuCommandBufferStub and InProcessCommandBuffer. We
would also want a central registration place where you could associate the
channel_id/route_id pair to one of these FenceSyncClient. That place could be
the SyncPointManager which should be shared between all of these.
IOW:
- in the GpuCommandBufferStub and InProcessCommandBuffer constructor, they would
register themselves to the SyncPointManager as a FenceSyncClient. They would
unregister themselves in their respective destructors
- in the OnWaitFenceSync implementation (for each of them), you would lookup the
release FenceSyncClient using the channel_id/route_id pair, and operate on that.
- it turns out that the OnWaitFenceSync implementation would probably then be
extremely similar, if not identical, between GpuCommandBufferStub and
InProcessCommandBuffer, and could probably be shared.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1054:
release_channel->GetUnprocessedOrderNum();
I think same here, this should be a per-stream state.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:4065: 'type': 'Custom',
I think this one doesn't have to be Custom, because you don't need specific
validation, and you don't change the return value. So if you remove 'type':
'Custom' and implement DoInsertFenceSyncCHROMIUM instead of
HandleInsertFenceSyncCHROMIUM, you leverage more of the autogen.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:311: GL_APICALL GLuint        
GL_APIENTRY glInsertFenceSyncCHROMIUM (void);
nit: alignment

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/command_buffer.h:119: virtual void
GetRouteInformation(int* channel_id, uint32_t* route_id) = 0;
Because this is only used on the client side, you can put it on GpuControl
instead, so that you don't have to implement a placeholder for
CommandBufferService.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:609: *route_id = 0;
As mentioned above, we're going to need an implementation of the FenceSync
mechanism in this class as well (which combines both the client side and the
server side).


As you mention in the description, one key question is what should be the
channel_id/route_id pair for this. But related to what I said above, really what
we need them for is to look up the FenceSyncClient, so really it is simply a
globally-unique namespace... it doesn't have to be explicitly tied to a
GpuChannel/GpuCommandBufferStub, though it's nice if we can generate that
namespace from the client side without round-trip. So what we could do is:
- instead of an explicit channel_id/route_id, we could make it a semi-opaque
FenceSyncNamespace struct, that has 2 ids, which could be as generic as
"word1"/"word2" (but could be extended if we need more specificity, e.g. for
mojo).
- for CommandBufferProxyImpl, word1 would be the channel id, word2 would be the
route id
- for InProcessCommandBuffer, we'd need a unique word1 that's different than any
channel id, and we'd need a unique id for each InProcessCommandBuffer instance
  * for word1, we could make it a property of the
InProcessCommandBuffer::Service (which is shared across the various
InProcessCommandBuffers). If it is the default service (from the
GpuInProcessThreadHolder), it can simply be 0, since it doesn't share anything
with cross-process command buffers. If it is the injected one (created in
SynchronousCompositorFactoryImpl::GpuThreadService), we could create an id in
the same namespace as the channels, namely using
ChildProcessHostImpl::GenerateChildProcessUniqueId() (see e.g.
BrowserGpuChannelHostFactory)
  * for word2, we could make it either a globally unique id (using a
StaticAtomicSequenceNumber) or a counter in the InProcessCommandBuffer::Service
  * because InProcessCommandBuffer is both the client and the service side, the
same FenceSyncNamespace is available on both sides
- both the GpuCommandBufferStub and the InProcessCommandBuffer could register
themselves to the SyncPointManager using the FenceSyncNamespace directly
- SyncToken could then use a FenceSyncNamespace + release id.
- we don't have to talk about routes in gpu/command_buffer, which knows nothing
about it.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_manager.h (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_manager.h:34: uint32 release) = 0;
nit: Maybe you can pass a const SyncToken& here and below.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_manager_sync.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_manager_sync.cc:27: typedef std::pair<int,
std::pair<uint32, uint32>> SyncToken;
Can you use SyncToken from gles2_cmd_format.h instead?

https://codereview.chromium.org/1331843005/diff/20001/ppapi/proxy/ppapi_comma...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/ppapi/proxy/ppapi_comma...
ppapi/proxy/ppapi_command_buffer_proxy.cc:172: *route_id = 0;
We will need to reflect the real channel_id/route_id here (or
FenceSyncNamespace), if we want to use the new sync point in pepper.
The way PpapiCommandBufferProxy works is that it forwards all command buffer
IPCs to the renderer, that then sends them to the GPU process using its own
channel. So channel_id and route_id are known on the renderer side at
initialization time, and it's just a matter of sending the FenceSyncNamespace in
the reply of PpapiHostMsg_PPBGraphics3D_Create. On the pepper side
(PPB_Graphics3D_Proxy::CreateProxyResource), it's just a matter of wiring it to
the PpapiCommandBufferProxy via Graphics3D::Init. On the renderer side
(PPB_Graphics3D_Proxy::OnMsgCreate), it's just a matter of passing it through
ResourceCreationImpl::CreateGraphics3DRaw -> PPB_Graphics3D_Impl::CreateRaw ->
PPB_Graphics3D_Impl::InitRaw and grabbing it there from command_buffer_.

[no sievers, 2015-09-12 01:01:29]

Yes, thanks for thinking about the mixed Android WebView use-case. The
suggestion with the shared interface and namespace generalization sounds great.

Also note that the in-process sync points wait by blocking the service thread.
You can probably keep that behavior by doing the same thing we do now which is
broadcast the retirement to unlock the condition var.

https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5401:
sync_token_data->release_count = fence_sync;
So how do we guarantee that this release is seen by the GPU IO thread before a
Flush through another channel?

Don't we need something like a roundtrip IPC here?

[David Yen, 2015-09-12 01:53:51]

https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5401:
sync_token_data->release_count = fence_sync;
On 2015/09/12 01:01:28, sievers wrote:
> So how do we guarantee that this release is seen by the GPU IO thread before a
> Flush through another channel?
> 
> Don't we need something like a roundtrip IPC here?

The "sync_token" is the token they need to wait on (see WaitSyncTokenCHROMIUM
below), so for proper usage the wait cannot be flushed before the release is
flushed.

In the case where a misbehaved renderer "guesses" the sync_token without using
this function (so it doesn't validate it has been flushed), on the service side
we order all the flushes by a global order number. The wait token can detect
that the release was not flushed yet by comparing the order number of the 2
channels. If it detects that the release was not flushed properly, it ignores
the wait.

[piman, 2015-09-14 20:09:46]

On Fri, Sep 11, 2015 at 6:01 PM, <sievers@chromium.org> wrote:

> Yes, thanks for thinking about the mixed Android WebView use-case. The
> suggestion with the shared interface and namespace generalization sounds
> great.
>
> Also note that the in-process sync points wait by blocking the service
> thread.
> You can probably keep that behavior by doing the same thing we do now
> which is
> broadcast the retirement to unlock the condition var.
>
>
>
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
>
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:5401:
> sync_token_data->release_count = fence_sync;
> So how do we guarantee that this release is seen by the GPU IO thread
> before a Flush through another channel?
>
> Don't we need something like a roundtrip IPC here?
>

We do need a roundtrip IPC for cross-channel cases, but it could happen any
time between the Flush that commits the fence sync, and when we send the
sync token to another process. Here is appropriate for cross-channel cases,
but I think we want to avoid it for same-channel/different-stream cases.


> https://codereview.chromium.org/1331843005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2015-09-14 21:44:11]

On 2015/09/14 20:09:46, piman (slow to review) wrote:
> On Fri, Sep 11, 2015 at 6:01 PM, <mailto:sievers@chromium.org> wrote:
> 
> > Yes, thanks for thinking about the mixed Android WebView use-case. The
> > suggestion with the shared interface and namespace generalization sounds
> > great.
> >
> > Also note that the in-process sync points wait by blocking the service
> > thread.
> > You can probably keep that behavior by doing the same thing we do now
> > which is
> > broadcast the retirement to unlock the condition var.
> >
> >
> >
> >
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
> > File gpu/command_buffer/client/gles2_implementation.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
> > gpu/command_buffer/client/gles2_implementation.cc:5401:
> > sync_token_data->release_count = fence_sync;
> > So how do we guarantee that this release is seen by the GPU IO thread
> > before a Flush through another channel?
> >
> > Don't we need something like a roundtrip IPC here?
> >
> 
> We do need a roundtrip IPC for cross-channel cases, but it could happen any
> time between the Flush that commits the fence sync, and when we send the
> sync token to another process. Here is appropriate for cross-channel cases,
> but I think we want to avoid it for same-channel/different-stream cases.
> 

Seems like that would have to also be exposed in the API somehow (unless we do
something crazy and hide it in the IPC subsystem).
You'd somehow have to specify the target context, or have some sort of explicit
namespaces for sync points to be able to tell which ones need to synchronize
(with a roundtrip IPC, or waiting for a token or whatever).

The Android WebView use case should probably also fall under this since it's a
similar race of flushes where you need a global order (between in-process
service and the gpu channel service).

> 
> > https://codereview.chromium.org/1331843005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-14 22:09:30]

On Mon, Sep 14, 2015 at 2:44 PM, <sievers@chromium.org> wrote:

> On 2015/09/14 20:09:46, piman (slow to review) wrote:
>
>> On Fri, Sep 11, 2015 at 6:01 PM, <mailto:sievers@chromium.org> wrote:
>>
>
> > Yes, thanks for thinking about the mixed Android WebView use-case. The
>> > suggestion with the shared interface and namespace generalization sounds
>> > great.
>> >
>> > Also note that the in-process sync points wait by blocking the service
>> > thread.
>> > You can probably keep that behavior by doing the same thing we do now
>> > which is
>> > broadcast the retirement to unlock the condition var.
>> >
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
>
>> > File gpu/command_buffer/client/gles2_implementation.cc (right):
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1331843005/diff/90001/gpu/command_buffer/clie...
>
>> > gpu/command_buffer/client/gles2_implementation.cc:5401:
>> > sync_token_data->release_count = fence_sync;
>> > So how do we guarantee that this release is seen by the GPU IO thread
>> > before a Flush through another channel?
>> >
>> > Don't we need something like a roundtrip IPC here?
>> >
>>
>
> We do need a roundtrip IPC for cross-channel cases, but it could happen any
>> time between the Flush that commits the fence sync, and when we send the
>> sync token to another process. Here is appropriate for cross-channel
>> cases,
>> but I think we want to avoid it for same-channel/different-stream cases.
>>
>
>
> Seems like that would have to also be exposed in the API somehow (unless
> we do
> something crazy and hide it in the IPC subsystem).
> You'd somehow have to specify the target context, or have some sort of
> explicit
> namespaces for sync points to be able to tell which ones need to
> synchronize
> (with a roundtrip IPC, or waiting for a token or whatever).
>

I was thinking more along the lines of having the embedder explicitly do
the dummy IPC round-trip (because it is the one that has the notion of what
it wants to synchronize to), but if we want to have the GLES2Implementation
do it, you're right that we would need a notion of scope here.

>
> The Android WebView use case should probably also fall under this since
> it's a
> similar race of flushes where you need a global order (between in-process
> service and the gpu channel service).
>

Yep. Considering the InProcess Service as similar to a channel is probably
the right mental model.

Global order-wise, I think we don't need a round-trip per se, for the
in-process service (provided we make sure the global order is not increased
until after the task is known to be enqueued), but I realize that we'll
need to expose global_order_counter_ to InProcessCommandBuffer, and make it
atomic (maybe it should move to SyncPointManager too).


>
> > https://codereview.chromium.org/1331843005/
>> >
>>
>
> To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1331843005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2015-09-23 18:30:34]

Does it make sense to expose the idea of streams at an API level (IE. in
CHROMIUM_sync_point.txt). Otherwise, we could just make an internal
GenSyncStreamToken() function or something that we can use internally when we
know we will be waiting on it within a single channel but across multiple
streams. In that command I can skip the flush verification.

The wait command can take a token from either function but for tokens generated
with the stream version it can just check that we are in fact on the same
channel.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:380: if (channel_id)
On 2015/09/10 23:55:31, piman (slow to review) wrote:
> maybe just DCHECK(channel_id) and DCHECK(route_id), the callers always have
> valid pointers to both.

No longer relevant.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1048:
release_channel->GetProcessedOrderNum();
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> I think it would be good to start reasoning in terms of streams rather than
> channels. Once we have multiple streams within a channel, each of them will
have
> a separate processed order number (because each IPC corresponds to a unique
> stream), and the goal of this is to allow ordering between different streams.
> 
> So I figure you could have a GetProcessedOrderNum on GpuCommandBufferStub,
that
> queries the state on the GpuChannel using its stream_id. That state would then
> go onto GpuChannel::StreamState.
> At least putting the API in place even if the state is updated for all streams
> at once for now, so that transitioning to unordered stream execution is
easier.
> 
> 
> Actually, that would go hand-in-hand with the second thing: we need to deal
with
> the fact that on at least Android we need to combine cross-process and
> in-process command buffers, and synchronization between them. What that means
is
> that:
> - you can't assume here that what you're waiting for is a
> GpuChannel/GpuCommandBufferStub
> - vice versa, in a InProcessCommandBuffer, you can't assume you're waiting on
> another InProcessCommandBuffer.
> 
> What I think we would want is an interface ("FenceSyncClient"?), that provides
> at least GetProcessedOrderNum, GetUnprocessedOrderNum,
ValidateFenceSyncRelease,
> and a GetFenceSyncManager()  (or directly
> IsFenceSyncReleased/AddFenceSyncCallback to satisfy the Law of Demeter), that
> would be implemented by both GpuCommandBufferStub and InProcessCommandBuffer.
We
> would also want a central registration place where you could associate the
> channel_id/route_id pair to one of these FenceSyncClient. That place could be
> the SyncPointManager which should be shared between all of these.
> IOW:
> - in the GpuCommandBufferStub and InProcessCommandBuffer constructor, they
would
> register themselves to the SyncPointManager as a FenceSyncClient. They would
> unregister themselves in their respective destructors
> - in the OnWaitFenceSync implementation (for each of them), you would lookup
the
> release FenceSyncClient using the channel_id/route_id pair, and operate on
that.
> - it turns out that the OnWaitFenceSync implementation would probably then be
> extremely similar, if not identical, between GpuCommandBufferStub and
> InProcessCommandBuffer, and could probably be shared.

No longer relevant, although the ideas here were fleshed out more and handled in
other CLs.

https://codereview.chromium.org/1331843005/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1054:
release_channel->GetUnprocessedOrderNum();
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> I think same here, this should be a per-stream state.

Done.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:4065: 'type': 'Custom',
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> I think this one doesn't have to be Custom, because you don't need specific
> validation, and you don't change the return value. So if you remove 'type':
> 'Custom' and implement DoInsertFenceSyncCHROMIUM instead of
> HandleInsertFenceSyncCHROMIUM, you leverage more of the autogen.

The 'cmd_args' modification doesn't seem to work without type 'Custom'. Perhaps
it is a bug? I'll leave it as type custom for now.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:311: GL_APICALL GLuint        
GL_APIENTRY glInsertFenceSyncCHROMIUM (void);
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> nit: alignment

Done.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/command_buffer.h:119: virtual void
GetRouteInformation(int* channel_id, uint32_t* route_id) = 0;
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> Because this is only used on the client side, you can put it on GpuControl
> instead, so that you don't have to implement a placeholder for
> CommandBufferService.

No longer relevant.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:609: *route_id = 0;
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> As mentioned above, we're going to need an implementation of the FenceSync
> mechanism in this class as well (which combines both the client side and the
> server side).
> 
> 
> As you mention in the description, one key question is what should be the
> channel_id/route_id pair for this. But related to what I said above, really
what
> we need them for is to look up the FenceSyncClient, so really it is simply a
> globally-unique namespace... it doesn't have to be explicitly tied to a
> GpuChannel/GpuCommandBufferStub, though it's nice if we can generate that
> namespace from the client side without round-trip. So what we could do is:
> - instead of an explicit channel_id/route_id, we could make it a semi-opaque
> FenceSyncNamespace struct, that has 2 ids, which could be as generic as
> "word1"/"word2" (but could be extended if we need more specificity, e.g. for
> mojo).
> - for CommandBufferProxyImpl, word1 would be the channel id, word2 would be
the
> route id
> - for InProcessCommandBuffer, we'd need a unique word1 that's different than
any
> channel id, and we'd need a unique id for each InProcessCommandBuffer instance
>   * for word1, we could make it a property of the
> InProcessCommandBuffer::Service (which is shared across the various
> InProcessCommandBuffers). If it is the default service (from the
> GpuInProcessThreadHolder), it can simply be 0, since it doesn't share anything
> with cross-process command buffers. If it is the injected one (created in
> SynchronousCompositorFactoryImpl::GpuThreadService), we could create an id in
> the same namespace as the channels, namely using
> ChildProcessHostImpl::GenerateChildProcessUniqueId() (see e.g.
> BrowserGpuChannelHostFactory)
>   * for word2, we could make it either a globally unique id (using a
> StaticAtomicSequenceNumber) or a counter in the
InProcessCommandBuffer::Service
>   * because InProcessCommandBuffer is both the client and the service side,
the
> same FenceSyncNamespace is available on both sides
> - both the GpuCommandBufferStub and the InProcessCommandBuffer could register
> themselves to the SyncPointManager using the FenceSyncNamespace directly
> - SyncToken could then use a FenceSyncNamespace + release id.
> - we don't have to talk about routes in gpu/command_buffer, which knows
nothing
> about it.

Done.

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_manager.h (right):

https://codereview.chromium.org/1331843005/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_manager.h:34: uint32 release) = 0;
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> nit: Maybe you can pass a const SyncToken& here and below.

Done.

https://codereview.chromium.org/1331843005/diff/20001/ppapi/proxy/ppapi_comma...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1331843005/diff/20001/ppapi/proxy/ppapi_comma...
ppapi/proxy/ppapi_command_buffer_proxy.cc:172: *route_id = 0;
On 2015/09/10 23:55:32, piman (slow to review) wrote:
> We will need to reflect the real channel_id/route_id here (or
> FenceSyncNamespace), if we want to use the new sync point in pepper.
> The way PpapiCommandBufferProxy works is that it forwards all command buffer
> IPCs to the renderer, that then sends them to the GPU process using its own
> channel. So channel_id and route_id are known on the renderer side at
> initialization time, and it's just a matter of sending the FenceSyncNamespace
in
> the reply of PpapiHostMsg_PPBGraphics3D_Create. On the pepper side
> (PPB_Graphics3D_Proxy::CreateProxyResource), it's just a matter of wiring it
to
> the PpapiCommandBufferProxy via Graphics3D::Init. On the renderer side
> (PPB_Graphics3D_Proxy::OnMsgCreate), it's just a matter of passing it through
> ResourceCreationImpl::CreateGraphics3DRaw -> PPB_Graphics3D_Impl::CreateRaw ->
> PPB_Graphics3D_Impl::InitRaw and grabbing it there from command_buffer_.

Done.

[sunnyps, 2015-09-23 21:44:45]

sunnyps@chromium.org changed reviewers:
+ sunnyps@chromium.org

[sunnyps, 2015-09-23 21:44:45]

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1108: return
scheduler_->scheduled();
Can we use the waiting_for_sync_point_ variable here? See WaitForSyncPoint.

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1115:
DCHECK(waiting_for_sync_point_);
This DCHECK will fail because OnWaitFenceSync does not set
waiting_for_sync_point_.

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:253: void
PullTextureUpdates(gpu::CommandBufferNamespace namespace_id,
PullTextureUpdates(SyncToken)?

https://codereview.chromium.org/1331843005/diff/130001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/1331843005/diff/130001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.h:355: uint32_t
next_fence_sync_release_;
I think this should be part of GpuChannelHost::StreamFlushInfo. That way the id
used for the sync token is { GPU_IO, channel_id << 32 | stream_id, release_count
} and on the service side SyncPointClientState is also a per-stream thing. WDYT?

[sunnyps, 2015-09-23 22:38:30]

Thinking about this a bit more, here's one weird edge case:

1. On context A, insert sync point.
2. On context A, ordering barrier.
3. On context B, insert sync point.
4. On context B, flush.
5. On context A, generate sync token - fails.
6. On context B, generate sync token - succeeds.

[sunnyps, 2015-09-23 22:38:55]

On 2015/09/23 22:38:30, sunnyps wrote:
> Thinking about this a bit more, here's one weird edge case:
> 
> 1. On context A, insert sync point.
> 2. On context A, ordering barrier.
> 3. On context B, insert sync point.
> 4. On context B, flush.
> 5. On context A, generate sync token - fails.
> 6. On context B, generate sync token - succeeds.

Note: Context A and B are on the same stream (in the same channel).

[David Yen, 2015-09-24 00:04:19]

On 2015/09/23 22:38:55, sunnyps wrote:
> On 2015/09/23 22:38:30, sunnyps wrote:
> > Thinking about this a bit more, here's one weird edge case:
> > 
> > 1. On context A, insert sync point.
> > 2. On context A, ordering barrier.
> > 3. On context B, insert sync point.
> > 4. On context B, flush.
> > 5. On context A, generate sync token - fails.
> > 6. On context B, generate sync token - succeeds.
> 
> Note: Context A and B are on the same stream (in the same channel).

I am going to move the release count logic from command buffer helper to
GpuControl instead. The in process command buffer must be implemented
differently anyways. Each implementation will be in charge of working out how
ordering barriers and flushes release fence syncs.

[David Yen, 2015-09-24 00:04:31]

On 2015/09/23 22:38:55, sunnyps wrote:
> On 2015/09/23 22:38:30, sunnyps wrote:
> > Thinking about this a bit more, here's one weird edge case:
> > 
> > 1. On context A, insert sync point.
> > 2. On context A, ordering barrier.
> > 3. On context B, insert sync point.
> > 4. On context B, flush.
> > 5. On context A, generate sync token - fails.
> > 6. On context B, generate sync token - succeeds.
> 
> Note: Context A and B are on the same stream (in the same channel).

I am going to move the release count logic from command buffer helper to
GpuControl instead. The in process command buffer must be implemented
differently anyways. Each implementation will be in charge of working out how
ordering barriers and flushes release fence syncs.

[David Yen, 2015-09-25 20:13:33]

PTAL.

I have moved the fence sync implementation to GpuControl and implemented it for
the various command buffers we have.

I also have fence sync validation working now (by using a synchronous NOP). A
single synchronous NOP will validate all flushes that happened on a single
channel (including all streams).

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1108: return
scheduler_->scheduled();
On 2015/09/23 21:44:45, sunnyps wrote:
> Can we use the waiting_for_sync_point_ variable here? See WaitForSyncPoint.

Yes, I missed this new variable in the merge. Done.

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1115:
DCHECK(waiting_for_sync_point_);
On 2015/09/23 21:44:45, sunnyps wrote:
> This DCHECK will fail because OnWaitFenceSync does not set
> waiting_for_sync_point_.

Done.

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1331843005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:253: void
PullTextureUpdates(gpu::CommandBufferNamespace namespace_id,
On 2015/09/23 21:44:45, sunnyps wrote:
> PullTextureUpdates(SyncToken)?

I think it's easier at this level to use 3 variables since that's what they
have. Otherwise I would have to create the struct at every call site instead of
once in the function.

https://codereview.chromium.org/1331843005/diff/130001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/1331843005/diff/130001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.h:355: uint32_t
next_fence_sync_release_;
On 2015/09/23 21:44:45, sunnyps wrote:
> I think this should be part of GpuChannelHost::StreamFlushInfo. That way the
id
> used for the sync token is { GPU_IO, channel_id << 32 | stream_id,
release_count
> } and on the service side SyncPointClientState is also a per-stream thing.
WDYT?

The command buffer ID should be per context (as that is the level of where
release counts are kept track). I discussed though this has been moved to
GpuControl.

[David Yen, 2015-09-25 20:14:49]

+dcheng@ for gpu_messages.h. I added a simple NOP message which is used to
validate all previous asynchronous messages have been received.

[David Yen, 2015-09-25 20:15:45]

dyen@chromium.org changed reviewers:
+ dcheng@chromium.org

[David Yen, 2015-09-25 20:16:12]

+dcheng@ for gpu_messages.h. I added a simple NOP message which is used to
validate all previous asynchronous messages have been received.

[David Yen, 2015-09-25 21:03:41]

dyen@chromium.org changed reviewers:
+ rjkroege@chromium.org

[David Yen, 2015-09-25 21:03:42]

+rjkroege for mojo command buffer changes.

[David Yen, 2015-09-25 23:32:10]

On 2015/09/25 21:03:42, David Yen wrote:
> +rjkroege for mojo command buffer changes.

Whoops, I just realized I didn't actually implement the actual fence sync
signal/release functions in InProcessCommandBuffer (or the mojo ones for that
matter, that should probably be a TODO?). I just added the implementation for
InProcessCommandBuffer.

Unless I am terribly mistaken, it doesn't seem like we need to use a condition
variable that broadcasts to all threads waiting on releases. Isn't this just a
simple waitable event situation? I basically replaced that code by using the
waitable event's signal as the callback, and then just calling Wait() in
InProcessCommandBuffer.

[piman, 2015-09-26 00:09:51]

Early comments, I reviewed the client side, but I need to spend more time on the
service side.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:247: flush_id));
TBH all this tracking seems costly, and most of it is only to check that the
client isn't screwing up (i.e. it'd be ok to limit that to debug). Keep in mind
that unless the client code explicitly checks for them (which it shouldn't),
raising GL errors doesn't do anything useful.
The other part is to automatically send the nop IPC, which we may want to make
more explicit because it isn't needed for same-channel synchronization (the
justification for this feature).

Anyway, I'm not sure we need to do anything about it now, we can try to ship
this and see if the performance is a problem, but at least add a TODO to revisit
- it may very well be simpler and cheaper to ignore the validation and have the
clients explicitly do the nop IPC where necessary (e.g. once a frame before
sending data to other processes).

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:525: return release != 0
&& release < next_fence_sync_release_;
Do we need to handle wraparound? 1 per frame would wrap after 828 days, but if
it's 100 per frame, then it's 8 days which seems within reach.

One option is to cut the space in half (2^31), and Finish() when switching half.
It causes a hitch (once every several days), but it ensures synchronization
still works (conservatively). Any sync point that's >2^31 old is guaranteed to
have passed, so if we still ignore sync points that look to be "in the future"
(because of wrapping), we're still fine.

Another option is to give up, but then we should CHECK in
GenerateFenceSyncRelease to avoid security issues. It'll cause crashes if it
happens in practice though.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:415:
validate_flushes[iter_stream_id] = flush_info.flushed_stream_flush_id;
If there is a pending flush on that stream, you also need to InternalFlush it,
otherwise the flush id isn't an guarantee that it's visible on the GPU process.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:420: if (Send(new
GpuChannelMsg_Nop())) {
nit: you can skip if validate_flushes is empty.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:810: OnNop)
You want to handle this on the IO thread (in the filter).
Once this arrives on the IO thread, you know all previous messages have arrived
too, hence you generated a valid ordering id for them, they're in the queue,
etc. and that's the important part.

You don't want to handle it on the main thread, because it may be busy executing
blocking commands (e.g. glFinish etc).

https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
ppapi/proxy/ppapi_command_buffer_proxy.cc:206: bool
PpapiCommandBufferProxy::IsFenceSyncFlushed(uint32_t release) {
Same as GpuChannelHost, you need to FlushInternal if a flush is pending.

[David Yen, 2015-09-26 00:24:56]

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:247: flush_id));
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> TBH all this tracking seems costly, and most of it is only to check that the
> client isn't screwing up (i.e. it'd be ok to limit that to debug). Keep in
mind
> that unless the client code explicitly checks for them (which it shouldn't),
> raising GL errors doesn't do anything useful.
> The other part is to automatically send the nop IPC, which we may want to make
> more explicit because it isn't needed for same-channel synchronization (the
> justification for this feature).
> 
> Anyway, I'm not sure we need to do anything about it now, we can try to ship
> this and see if the performance is a problem, but at least add a TODO to
revisit
> - it may very well be simpler and cheaper to ignore the validation and have
the
> clients explicitly do the nop IPC where necessary (e.g. once a frame before
> sending data to other processes).

None of these variables are atomic so there is not too much extra overhead here
(which would probably be soaked up by cache cycles anyways) so the main overhead
here is the allocation for the queue which should only happen once in practice
on the first release flush. On the GpuChannelHost I've added a flush counter
that gets incremented only when we previously already had a lock so that
shouldn't add too much overhead either. The send function do have an extra 2
locks before and after but it should be dwarfed by the synchronous ipc overhead.
Overall I don't think it adds any noticeable overhead.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:415:
validate_flushes[iter_stream_id] = flush_info.flushed_stream_flush_id;
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> If there is a pending flush on that stream, you also need to InternalFlush it,
> otherwise the flush id isn't an guarantee that it's visible on the GPU
process.

The pending flushes here are ordering barriers which have not been flushed yet.
Those shouldn't be counted as "flushed" right?

[piman, 2015-09-26 00:34:51]

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:415:
validate_flushes[iter_stream_id] = flush_info.flushed_stream_flush_id;
On 2015/09/26 00:24:56, David Yen wrote:
> On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > If there is a pending flush on that stream, you also need to InternalFlush
it,
> > otherwise the flush id isn't an guarantee that it's visible on the GPU
> process.
> 
> The pending flushes here are ordering barriers which have not been flushed
yet.
> Those shouldn't be counted as "flushed" right?

Oh, right. flushed_stream_flush_id is only set after the real flush. Maybe it's
ok to require a real flush instead of implicitly flushing pending
OrderingBarriers. Either way is reasonable.

[rjkroege, 2015-09-26 00:40:44]

mojo code: lgtm

mus code: looks like it need attention. leaving it TODO is ok with me --
+penghuang@chromium.org can perhaps help/comment as as has been fixing the
existing sync point implementation in CommandBufferDriver.

[Peng, 2015-09-26 12:54:28]

On 2015/09/26 00:41:48, rjkroege wrote:

mus code lgtm.

[dcheng, 2015-09-28 07:39:31]

dcheng@chromium.org changed reviewers:
+ penghuang@chromium.org

[dcheng, 2015-09-28 07:39:33]

Just some small nits.

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:317: if (!release_state.get())
No .get()

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:295: if (!release_state.get())
No .get()

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:165: void
GpuChannelHost::InternalFlush(StreamFlushInfo& flush_info) {
Mutable reference arguments are not permitted by the Chromium C++ style guide.

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:168:
DCHECK(flush_info.flushed_stream_flush_id < flush_info.flush_id);
DCHECK_LT for a better log message if this ever fails.

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1105: if (!release_state.get())
No .get()

https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:889: if
(!release_state.get())
No .get()

[dcheng, 2015-09-28 07:39:35]

Just some small nits.

[David Yen, 2015-09-28 17:38:18]

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:525: return release != 0
&& release < next_fence_sync_release_;
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> Do we need to handle wraparound? 1 per frame would wrap after 828 days, but if
> it's 100 per frame, then it's 8 days which seems within reach.
> 
> One option is to cut the space in half (2^31), and Finish() when switching
half.
> It causes a hitch (once every several days), but it ensures synchronization
> still works (conservatively). Any sync point that's >2^31 old is guaranteed to
> have passed, so if we still ignore sync points that look to be "in the future"
> (because of wrapping), we're still fine.
> 
> Another option is to give up, but then we should CHECK in
> GenerateFenceSyncRelease to avoid security issues. It'll cause crashes if it
> happens in practice though.

What about just using a uint64? This number doesn't need to be atomic so we
don't have an Atomic64 problem.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:420: if (Send(new
GpuChannelMsg_Nop())) {
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> nit: you can skip if validate_flushes is empty.

Even better, I made it skip the validation if the current stream has no flushes
to validate. Other streams can validate themselves later and hopefully be able
to validate even more flushes.

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:810: OnNop)
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> You want to handle this on the IO thread (in the filter).
> Once this arrives on the IO thread, you know all previous messages have
arrived
> too, hence you generated a valid ordering id for them, they're in the queue,
> etc. and that's the important part.
> 
> You don't want to handle it on the main thread, because it may be busy
executing
> blocking commands (e.g. glFinish etc).

Done.

https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
ppapi/proxy/ppapi_command_buffer_proxy.cc:206: bool
PpapiCommandBufferProxy::IsFenceSyncFlushed(uint32_t release) {
On 2015/09/26 00:09:51, piman (slow to review) wrote:
> Same as GpuChannelHost, you need to FlushInternal if a flush is pending.

Same as previous comment, I would rather rely on them flushing. I have moved the
pending/flushed assignments around to FlushInternal() though, makes it a bit
easier to understand.

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:317: if (!release_state.get())
On 2015/09/28 07:39:33, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:295: if (!release_state.get())
On 2015/09/28 07:39:33, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:165: void
GpuChannelHost::InternalFlush(StreamFlushInfo& flush_info) {
On 2015/09/28 07:39:33, dcheng wrote:
> Mutable reference arguments are not permitted by the Chromium C++ style guide.

Done, changed to a pointer.

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:168:
DCHECK(flush_info.flushed_stream_flush_id < flush_info.flush_id);
On 2015/09/28 07:39:33, dcheng wrote:
> DCHECK_LT for a better log message if this ever fails.

Done.

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:1105: if (!release_state.get())
On 2015/09/28 07:39:33, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:889: if
(!release_state.get())
On 2015/09/28 07:39:33, dcheng wrote:
> No .get()

Done.

[David Yen, 2015-09-29 00:36:23]

On 2015/09/28 17:38:18, David Yen wrote:
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> content/common/gpu/client/command_buffer_proxy_impl.cc:525: return release !=
0
> && release < next_fence_sync_release_;
> On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > Do we need to handle wraparound? 1 per frame would wrap after 828 days, but
if
> > it's 100 per frame, then it's 8 days which seems within reach.
> > 
> > One option is to cut the space in half (2^31), and Finish() when switching
> half.
> > It causes a hitch (once every several days), but it ensures synchronization
> > still works (conservatively). Any sync point that's >2^31 old is guaranteed
to
> > have passed, so if we still ignore sync points that look to be "in the
future"
> > (because of wrapping), we're still fine.
> > 
> > Another option is to give up, but then we should CHECK in
> > GenerateFenceSyncRelease to avoid security issues. It'll cause crashes if it
> > happens in practice though.
> 
> What about just using a uint64? This number doesn't need to be atomic so we
> don't have an Atomic64 problem.
> 
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> File content/common/gpu/client/gpu_channel_host.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> content/common/gpu/client/gpu_channel_host.cc:420: if (Send(new
> GpuChannelMsg_Nop())) {
> On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > nit: you can skip if validate_flushes is empty.
> 
> Even better, I made it skip the validation if the current stream has no
flushes
> to validate. Other streams can validate themselves later and hopefully be able
> to validate even more flushes.
> 
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
> File content/common/gpu/gpu_channel.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
> content/common/gpu/gpu_channel.cc:810: OnNop)
> On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > You want to handle this on the IO thread (in the filter).
> > Once this arrives on the IO thread, you know all previous messages have
> arrived
> > too, hence you generated a valid ordering id for them, they're in the queue,
> > etc. and that's the important part.
> > 
> > You don't want to handle it on the main thread, because it may be busy
> executing
> > blocking commands (e.g. glFinish etc).
> 
> Done.
> 
>
https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
> File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
> ppapi/proxy/ppapi_command_buffer_proxy.cc:206: bool
> PpapiCommandBufferProxy::IsFenceSyncFlushed(uint32_t release) {
> On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > Same as GpuChannelHost, you need to FlushInternal if a flush is pending.
> 
> Same as previous comment, I would rather rely on them flushing. I have moved
the
> pending/flushed assignments around to FlushInternal() though, makes it a bit
> easier to understand.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> File components/mus/gles2/command_buffer_driver.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> components/mus/gles2/command_buffer_driver.cc:317: if (!release_state.get())
> On 2015/09/28 07:39:33, dcheng wrote:
> > No .get()
> 
> Done.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> File components/mus/gles2/command_buffer_local.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> components/mus/gles2/command_buffer_local.cc:295: if (!release_state.get())
> On 2015/09/28 07:39:33, dcheng wrote:
> > No .get()
> 
> Done.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> File content/common/gpu/client/gpu_channel_host.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> content/common/gpu/client/gpu_channel_host.cc:165: void
> GpuChannelHost::InternalFlush(StreamFlushInfo& flush_info) {
> On 2015/09/28 07:39:33, dcheng wrote:
> > Mutable reference arguments are not permitted by the Chromium C++ style
guide.
> 
> Done, changed to a pointer.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> content/common/gpu/client/gpu_channel_host.cc:168:
> DCHECK(flush_info.flushed_stream_flush_id < flush_info.flush_id);
> On 2015/09/28 07:39:33, dcheng wrote:
> > DCHECK_LT for a better log message if this ever fails.
> 
> Done.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
> content/common/gpu/gpu_command_buffer_stub.cc:1105: if (!release_state.get())
> On 2015/09/28 07:39:33, dcheng wrote:
> > No .get()
> 
> Done.
> 
>
https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
> gpu/command_buffer/service/in_process_command_buffer.cc:889: if
> (!release_state.get())
> On 2015/09/28 07:39:33, dcheng wrote:
> > No .get()
> 
> Done.

Hold off on reviewing, I am writing some tests and some of the server side logic
is a bit off.

[David Yen, 2015-09-29 00:49:49]

On 2015/09/29 00:36:23, David Yen wrote:
> On 2015/09/28 17:38:18, David Yen wrote:
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> > content/common/gpu/client/command_buffer_proxy_impl.cc:525: return release
!=
> 0
> > && release < next_fence_sync_release_;
> > On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > > Do we need to handle wraparound? 1 per frame would wrap after 828 days,
but
> if
> > > it's 100 per frame, then it's 8 days which seems within reach.
> > > 
> > > One option is to cut the space in half (2^31), and Finish() when switching
> > half.
> > > It causes a hitch (once every several days), but it ensures
synchronization
> > > still works (conservatively). Any sync point that's >2^31 old is
guaranteed
> to
> > > have passed, so if we still ignore sync points that look to be "in the
> future"
> > > (because of wrapping), we're still fine.
> > > 
> > > Another option is to give up, but then we should CHECK in
> > > GenerateFenceSyncRelease to avoid security issues. It'll cause crashes if
it
> > > happens in practice though.
> > 
> > What about just using a uint64? This number doesn't need to be atomic so we
> > don't have an Atomic64 problem.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> > File content/common/gpu/client/gpu_channel_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/cli...
> > content/common/gpu/client/gpu_channel_host.cc:420: if (Send(new
> > GpuChannelMsg_Nop())) {
> > On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > > nit: you can skip if validate_flushes is empty.
> > 
> > Even better, I made it skip the validation if the current stream has no
> flushes
> > to validate. Other streams can validate themselves later and hopefully be
able
> > to validate even more flushes.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
> > File content/common/gpu/gpu_channel.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/content/common/gpu/gpu...
> > content/common/gpu/gpu_channel.cc:810: OnNop)
> > On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > > You want to handle this on the IO thread (in the filter).
> > > Once this arrives on the IO thread, you know all previous messages have
> > arrived
> > > too, hence you generated a valid ordering id for them, they're in the
queue,
> > > etc. and that's the important part.
> > > 
> > > You don't want to handle it on the main thread, because it may be busy
> > executing
> > > blocking commands (e.g. glFinish etc).
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
> > File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/300001/ppapi/proxy/ppapi_comm...
> > ppapi/proxy/ppapi_command_buffer_proxy.cc:206: bool
> > PpapiCommandBufferProxy::IsFenceSyncFlushed(uint32_t release) {
> > On 2015/09/26 00:09:51, piman (slow to review) wrote:
> > > Same as GpuChannelHost, you need to FlushInternal if a flush is pending.
> > 
> > Same as previous comment, I would rather rely on them flushing. I have moved
> the
> > pending/flushed assignments around to FlushInternal() though, makes it a bit
> > easier to understand.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> > File components/mus/gles2/command_buffer_driver.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> > components/mus/gles2/command_buffer_driver.cc:317: if (!release_state.get())
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > No .get()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> > File components/mus/gles2/command_buffer_local.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/components/mus/gles2/c...
> > components/mus/gles2/command_buffer_local.cc:295: if (!release_state.get())
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > No .get()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> > File content/common/gpu/client/gpu_channel_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> > content/common/gpu/client/gpu_channel_host.cc:165: void
> > GpuChannelHost::InternalFlush(StreamFlushInfo& flush_info) {
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > Mutable reference arguments are not permitted by the Chromium C++ style
> guide.
> > 
> > Done, changed to a pointer.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/cli...
> > content/common/gpu/client/gpu_channel_host.cc:168:
> > DCHECK(flush_info.flushed_stream_flush_id < flush_info.flush_id);
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > DCHECK_LT for a better log message if this ever fails.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
> > File content/common/gpu/gpu_command_buffer_stub.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/content/common/gpu/gpu...
> > content/common/gpu/gpu_command_buffer_stub.cc:1105: if
(!release_state.get())
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > No .get()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1331843005/diff/360001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/in_process_command_buffer.cc:889: if
> > (!release_state.get())
> > On 2015/09/28 07:39:33, dcheng wrote:
> > > No .get()
> > 
> > Done.
> 
> Hold off on reviewing, I am writing some tests and some of the server side
logic
> is a bit off.

Unit tests added, PTAL.

[David Yen, 2015-09-30 19:43:32]

This CL has gotten quite large, let me know if you want me to split it up into
smaller chunks. I think it wouldn't be too bad.

[piman, 2015-09-30 22:50:23]

I think the client side parts are good, as well as the command handling
(GLES2Implementation/GLES2Decoder and common parts), so you can extract them in
another CL if that makes it easier.
There's a few more things on the service side.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_channel.cc:310: return true;
You will need to send the reply here. This is typically done by the
IPC_MESSAGE_HANDLER macros, but since we're not using them you need to do it
manually:

IPC::Message* reply = IPC::SyncMessage::GenerateReply(&message);
Send(reply);
return true;

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:994: void
GpuCommandBufferStub::OnWaitSyncPointCompleted(uint32 sync_point) {
nit: At this point, this is equivalent to OnWaitFenceSyncCompleted(GPU_IO, 0,
sync_point) if you coalesce the TRACE_EVENTS. Maybe just do that, and have a
single function?

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:1057: if
(!sync_point_client_->client_state()->IsFenceSyncReleased(release)) {
It's worth DLOG(ERROR) if this is false, because it means the client did
something wrong (or is malicious).

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:1120:
PullTextureUpdates(namespace_id, command_buffer_id, release);
You would want to reset waiting_for_sync_point_ here.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/in_process_command_buffer.cc:857: if
(!sync_point_client_->client_state()->IsFenceSyncReleased(release)) {
Can you make this a DCHECK? For the in-process case, we can assume trust, so if
releases are out-of-order, the client messed up.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:82:
base::subtle::Release_Store(&processed_order_num_, order_num);
This should happen before we do the EnsureReleased, so that if the other client
tries to WaitForRelease on the same order, it will immediately call its
callback, instead of inserting an OrderFence, which would not be released until
later (if at all).

Actually, I believe modifying processed_order_num_ should happen under the lock,
otherwise there is a data race:
say thread 1 is in SyncPointClientState::WaitForRelease on l.140 (has the lock),
and thread 2 is in SyncPointOrderData::FinishProcessingOrderNumber on l.78
(doesn't have the lock). Thread 1 believe the order isn't passed, so it will add
the OrderFence. Thread 2 however is already done processing the order, but
hasn't updated processed_order_num_ yet.


So in the end, I don't think processed_order_num_ can simply be an atomic, it
also needs to be protected by the lock (at which point you can make it a
non-atomic simple variable).

You can probalby go through the same logic for unprocessed_order_num_ actually.
At the end of the day, you have an invariant: all the OrderFence in
order_fence_queue_ must have an order_num that is between processed_num_+1 and
unprocessed_order_num_ (inclusive). This invariant can only be consistent when
the lock is held.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:142: if ((processed_num + 1) >=
wait_order_num)
I think this should be processed_num >= wait_order_num. If wait_order_num ==
processed_num+1, you would want to wait, not early exit.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.h:81: virtual
~SyncPointOrderData();
This doesn't need to be virtual, because nothing derives from it, and it has no
other virtual member.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.h:147: virtual
~SyncPointClientState();
same here, no need to be virtual.

[David Yen, 2015-09-30 23:55:18]

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_channel.cc:310: return true;
On 2015/09/30 22:50:22, piman (slow to review) wrote:
> You will need to send the reply here. This is typically done by the
> IPC_MESSAGE_HANDLER macros, but since we're not using them you need to do it
> manually:
> 
> IPC::Message* reply = IPC::SyncMessage::GenerateReply(&message);
> Send(reply);
> return true;

Done.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:994: void
GpuCommandBufferStub::OnWaitSyncPointCompleted(uint32 sync_point) {
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> nit: At this point, this is equivalent to OnWaitFenceSyncCompleted(GPU_IO, 0,
> sync_point) if you coalesce the TRACE_EVENTS. Maybe just do that, and have a
> single function?

These functions are about to be deleted so it's probably ok to keep it separate
for now.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:1057: if
(!sync_point_client_->client_state()->IsFenceSyncReleased(release)) {
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> It's worth DLOG(ERROR) if this is false, because it means the client did
> something wrong (or is malicious).

Done.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/content/common/...
content/common/gpu/gpu_command_buffer_stub.cc:1120:
PullTextureUpdates(namespace_id, command_buffer_id, release);
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> You would want to reset waiting_for_sync_point_ here.

Done.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/in_process_command_buffer.cc:857: if
(!sync_point_client_->client_state()->IsFenceSyncReleased(release)) {
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> Can you make this a DCHECK? For the in-process case, we can assume trust, so
if
> releases are out-of-order, the client messed up.

Done.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:82:
base::subtle::Release_Store(&processed_order_num_, order_num);
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> This should happen before we do the EnsureReleased, so that if the other
client
> tries to WaitForRelease on the same order, it will immediately call its
> callback, instead of inserting an OrderFence, which would not be released
until
> later (if at all).
> 
> Actually, I believe modifying processed_order_num_ should happen under the
lock,
> otherwise there is a data race:
> say thread 1 is in SyncPointClientState::WaitForRelease on l.140 (has the
lock),
> and thread 2 is in SyncPointOrderData::FinishProcessingOrderNumber on l.78
> (doesn't have the lock). Thread 1 believe the order isn't passed, so it will
add
> the OrderFence. Thread 2 however is already done processing the order, but
> hasn't updated processed_order_num_ yet.
> 
> 
> So in the end, I don't think processed_order_num_ can simply be an atomic, it
> also needs to be protected by the lock (at which point you can make it a
> non-atomic simple variable).
> 
> You can probalby go through the same logic for unprocessed_order_num_
actually.
> At the end of the day, you have an invariant: all the OrderFence in
> order_fence_queue_ must have an order_num that is between processed_num_+1 and
> unprocessed_order_num_ (inclusive). This invariant can only be consistent when
> the lock is held.

That is true, in that case even the release counts all need to be checked under
the lock. I have added a ValidateReleaseOrderNumber() function that does all of
this and replaces AddOrderFenceGuard().

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:142: if ((processed_num + 1) >=
wait_order_num)
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> I think this should be processed_num >= wait_order_num. If wait_order_num ==
> processed_num+1, you would want to wait, not early exit.

This is a bit tricky and I only found it after writing the unit test, but we
must have an unprocessed order number less than the wait order number so wait
order num has to be processed_num + 2. For example, if we processed up to 9 and
the wait order number is 10, the release can't exist.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.h:81: virtual
~SyncPointOrderData();
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> This doesn't need to be virtual, because nothing derives from it, and it has
no
> other virtual member.

Done.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.h:147: virtual
~SyncPointClientState();
On 2015/09/30 22:50:23, piman (slow to review) wrote:
> same here, no need to be virtual.

Done.

[piman, 2015-10-01 01:00:23]

Ok, we're close I think. There's one more issue that needs a bit more thoughts.

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:142: if ((processed_num + 1) >=
wait_order_num)
On 2015/09/30 23:55:18, David Yen wrote:
> On 2015/09/30 22:50:23, piman (slow to review) wrote:
> > I think this should be processed_num >= wait_order_num. If wait_order_num ==
> > processed_num+1, you would want to wait, not early exit.
> 
> This is a bit tricky and I only found it after writing the unit test, but we
> must have an unprocessed order number less than the wait order number so wait
> order num has to be processed_num + 2. For example, if we processed up to 9
and
> the wait order number is 10, the release can't exist.

Ok, I think I convinced myself of why, but I think by going through that mental
process, I found an issue with the fact that we force-retire only in
FinishProcessingOrderNumber, after we reached or passed the waiting order num.

Say you have 2 clients and the following commands:

Client1:
[order 0] ...
[order 2] Release(1)
[order 5] Wait(2)

Client2:
[order 1] ...
[order 3] Wait(1)
[order 4] Release(2)
[order 6] ...

A valid execution order is [0 1 2 3 4 5 6], and that would work fine. However,
if Client1 doesn't Release(1) in [2], then when executing [3], Client2 will add
(Client2, [3], 1) to Client1's order_fence_queue_ (wait_order_num=[3],
client1.processed=0, client1.unprocessed=5), and unschedule. Client1 can
continue to [5] but because the fence 2 wasn't released, it will add itself to
Client2's order_fence_queue (wait_order_num=[5], client2.processed=1,
client2.unprocessed=6) and unschedule, never force-releasing 1 (and 2 won't be
force-released either).
-> deadlock.

The reason is that we really want to force-releasing 1 and unblock Client2 as
soon as Client1 goes beyond 3, that is, as soon as it starts processing [5], or
(if there was no [5]) as soon as it's out of things to do.


I think it would be useful to have a test for this case.

[David Yen, 2015-10-01 17:15:51]

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/500001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:142: if ((processed_num + 1) >=
wait_order_num)
On 2015/10/01 01:00:23, piman (slow to review) wrote:
> On 2015/09/30 23:55:18, David Yen wrote:
> > On 2015/09/30 22:50:23, piman (slow to review) wrote:
> > > I think this should be processed_num >= wait_order_num. If wait_order_num
==
> > > processed_num+1, you would want to wait, not early exit.
> > 
> > This is a bit tricky and I only found it after writing the unit test, but we
> > must have an unprocessed order number less than the wait order number so
wait
> > order num has to be processed_num + 2. For example, if we processed up to 9
> and
> > the wait order number is 10, the release can't exist.
> 
> Ok, I think I convinced myself of why, but I think by going through that
mental
> process, I found an issue with the fact that we force-retire only in
> FinishProcessingOrderNumber, after we reached or passed the waiting order num.
> 
> Say you have 2 clients and the following commands:
> 
> Client1:
> [order 0] ...
> [order 2] Release(1)
> [order 5] Wait(2)
> 
> Client2:
> [order 1] ...
> [order 3] Wait(1)
> [order 4] Release(2)
> [order 6] ...
> 
> A valid execution order is [0 1 2 3 4 5 6], and that would work fine. However,
> if Client1 doesn't Release(1) in [2], then when executing [3], Client2 will
add
> (Client2, [3], 1) to Client1's order_fence_queue_ (wait_order_num=[3],
> client1.processed=0, client1.unprocessed=5), and unschedule. Client1 can
> continue to [5] but because the fence 2 wasn't released, it will add itself to
> Client2's order_fence_queue (wait_order_num=[5], client2.processed=1,
> client2.unprocessed=6) and unschedule, never force-releasing 1 (and 2 won't be
> force-released either).
> -> deadlock.
> 
> The reason is that we really want to force-releasing 1 and unblock Client2 as
> soon as Client1 goes beyond 3, that is, as soon as it starts processing [5],
or
> (if there was no [5]) as soon as it's out of things to do.
> 
> 
> I think it would be useful to have a test for this case.

Ah, that is a tricky case. I think we actually did think of this case when we
were discussing the different attacks. I added a unit test for this and verified
that it was failing, and also fixed it now.

[piman, 2015-10-01 17:58:16]

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:101: while
(!order_fence_queue_.empty()) {
Do we need this any more?
The only case not handled by BeginProcessingOrderNumber is order_num ==
wait_order_num, which means a client is waiting on itself.

This case actually needs to be made invalid, which would only happen (in
ValidateReleaseOrderNumber) if the previous processed order number was
order_num-1, so I think we have a missing bit here too (add a test? ;) )


I think there's 2 ways of handling this:
- either explicitly refuse self-waits (in SyncPointClient::Wait, if
client_state_->order_data() == release_state->order_data())
- or actually set processed_order_num_ in BeginProcessingOrderNumber. I think
that makes a lot of sense (though we may want to rename it, in that we haven't
"processed" it, but we have released up-to that order). It also means we can
completely remove FinishProcessingOrderNumber, which seems like an interesting
property in itself.

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager_unittest.cc:345: // Assign Release
stream 1, 4 and assign Wait stream 2, 3.
nit: maybe comment that order [3] waits on fence (1), that is supposed to be
released by [1] but doesn't?

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager_unittest.cc:376: // Beginning
order number 4 should immediately release 3.
I think I'd like a similar test where there is no 4. I don't think it works
(there's nothing to finally cancel 1), but it needs to. Maybe when
processed_num_ becomes equal to unprocessed_num_, we need to release all
OrderFence?

[David Yen, 2015-10-01 18:12:00]

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.cc:101: while
(!order_fence_queue_.empty()) {
On 2015/10/01 17:58:16, piman (slow to review) wrote:
> Do we need this any more?
> The only case not handled by BeginProcessingOrderNumber is order_num ==
> wait_order_num, which means a client is waiting on itself.
> 
> This case actually needs to be made invalid, which would only happen (in
> ValidateReleaseOrderNumber) if the previous processed order number was
> order_num-1, so I think we have a missing bit here too (add a test? ;) )
> 
> 
> I think there's 2 ways of handling this:
> - either explicitly refuse self-waits (in SyncPointClient::Wait, if
> client_state_->order_data() == release_state->order_data())
> - or actually set processed_order_num_ in BeginProcessingOrderNumber. I think
> that makes a lot of sense (though we may want to rename it, in that we haven't
> "processed" it, but we have released up-to that order). It also means we can
> completely remove FinishProcessingOrderNumber, which seems like an interesting
> property in itself.

The way I have come to understand these 2 cases is if a release is missing from
an order number, then the FinishProcessingOrderNumber() will catch it. If the
release is missing and the order number is also missing, then the
BeginProcessingOrderNumber() will catch it (hence the 2 tests I have:
NonExistentRelease, and NonExistentOrderNumRelease.

The order number in the "order fence" is not necessarily the wait order number,
it is the something more like "highest order number the release should be
released by". Calculated during enqueue time by: "min(unprocessed_order_num_,
wait_order_num)".

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager_unittest.cc (right):

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager_unittest.cc:345: // Assign Release
stream 1, 4 and assign Wait stream 2, 3.
On 2015/10/01 17:58:16, piman (slow to review) wrote:
> nit: maybe comment that order [3] waits on fence (1), that is supposed to be
> released by [1] but doesn't?

What this test is really testing is releases and order numbers that do not
exist, from the release stream's point of view all it knows is that the release
did not exist in 1 and it has an unprocessed order of 4. It *could* have an
order number of 2 which releases it, but doesn't know since the wait stream had
processed order number 2.

Maybe a comment that explains order [3] waits on fence (1) that is supposed to
be released by [2] but doesn't exist on that stream?

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager_unittest.cc:376: // Beginning
order number 4 should immediately release 3.
On 2015/10/01 17:58:16, piman (slow to review) wrote:
> I think I'd like a similar test where there is no 4. I don't think it works
> (there's nothing to finally cancel 1), but it needs to. Maybe when
> processed_num_ becomes equal to unprocessed_num_, we need to release all
> OrderFence?

If there is no #4, then this test is the same as the previous case where the
release was expected to be in #1 but it didn't exist. This test is really
testing that the release should have been after 1 but before 3 (non existent
order number in the release stream).

Basically, the EndProcessing of #1 will release the fence. I can add a test for
this though, since it does seem a little bit different from the previous test
although internally it is handled the same.

[David Yen, 2015-10-01 18:25:43]

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager_unittest.cc (right):

https://codereview.chromium.org/1331843005/diff/560001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager_unittest.cc:376: // Beginning
order number 4 should immediately release 3.
On 2015/10/01 18:12:00, David Yen wrote:
> On 2015/10/01 17:58:16, piman (slow to review) wrote:
> > I think I'd like a similar test where there is no 4. I don't think it works
> > (there's nothing to finally cancel 1), but it needs to. Maybe when
> > processed_num_ becomes equal to unprocessed_num_, we need to release all
> > OrderFence?
> 
> If there is no #4, then this test is the same as the previous case where the
> release was expected to be in #1 but it didn't exist. This test is really
> testing that the release should have been after 1 but before 3 (non existent
> order number in the release stream).
> 
> Basically, the EndProcessing of #1 will release the fence. I can add a test
for
> this though, since it does seem a little bit different from the previous test
> although internally it is handled the same.

I've now added a new test "NonExistentRelease2" which is the same as this test
but removes the order number 4. I've also tried to add better descriptions of
how these tests work.

[David Yen, 2015-10-01 18:56:32]

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/1331843005/diff/560001/gpu/command_buf...
gpu/command_buffer/service/sync_point_manager.cc:101: while
(!order_fence_queue_.empty()) {
On 2015/10/01 18:12:00, David Yen wrote:
> On 2015/10/01 17:58:16, piman (slow to review) wrote:
> > Do we need this any more?
> > The only case not handled by BeginProcessingOrderNumber is order_num ==
> > wait_order_num, which means a client is waiting on itself.
> > 
> > This case actually needs to be made invalid, which would only happen (in
> > ValidateReleaseOrderNumber) if the previous processed order number was
> > order_num-1, so I think we have a missing bit here too (add a test? ;) )
> > 
> > 
> > I think there's 2 ways of handling this:
> > - either explicitly refuse self-waits (in SyncPointClient::Wait, if
> > client_state_->order_data() == release_state->order_data())
> > - or actually set processed_order_num_ in BeginProcessingOrderNumber. I
think
> > that makes a lot of sense (though we may want to rename it, in that we
haven't
> > "processed" it, but we have released up-to that order). It also means we can
> > completely remove FinishProcessingOrderNumber, which seems like an
interesting
> > property in itself.
> 
> The way I have come to understand these 2 cases is if a release is missing
from
> an order number, then the FinishProcessingOrderNumber() will catch it. If the
> release is missing and the order number is also missing, then the
> BeginProcessingOrderNumber() will catch it (hence the 2 tests I have:
> NonExistentRelease, and NonExistentOrderNumRelease.
> 
> The order number in the "order fence" is not necessarily the wait order
number,
> it is the something more like "highest order number the release should be
> released by". Calculated during enqueue time by: "min(unprocessed_order_num_,
> wait_order_num)".

I also added a check if a client is waiting on itself in Wait(). I realized that
in the original design, we do not call the callback on failures. But in all of
our current implementation, it is actually more convenient if the callback was
always guaranteed to be called. What do you think? This can be changed if some
new implementation requires it to not be called for some reason.

[David Yen, 2015-10-05 19:33:52]

friendly ping. I also added GL_SYNC_TOKEN_SIZE_CHROMIUM and GLES2Implementation
tests

[piman, 2015-10-05 23:00:36]

LGTM. Thanks, I had missed the min(), I think I convinced myself that this is
right. Thanks for the additional tests!

[David Yen, 2015-10-05 23:11:16]

On 2015/10/05 23:00:36, piman (slow to review) wrote:
> LGTM. Thanks, I had missed the min(), I think I convinced myself that this is
> right. Thanks for the additional tests!

Thanks for the review!

I just need an LGTM for mojo and gpu_message.h now.

jam@, could you take a look at the mojo changes? I already have an LGTM from
rjkroege@ and peng@. The presubmit check wanted me to add an entry in
"third_party/mojo/README.chromium", do I need to put anything else in there?

dcheng@, could you take a look at gpu_messages.h? I only added an NOP IPC
message.

[jam, 2015-10-06 14:55:47]

On 2015/10/05 23:11:16, David Yen wrote:
> On 2015/10/05 23:00:36, piman (slow to review) wrote:
> > LGTM. Thanks, I had missed the min(), I think I convinced myself that this
is
> > right. Thanks for the additional tests!
> 
> Thanks for the review!
> 
> I just need an LGTM for mojo and gpu_message.h now.
> 
> jam@, could you take a look at the mojo changes? I already have an LGTM from
> rjkroege@ and peng@. The presubmit check wanted me to add an entry in
> "third_party/mojo/README.chromium", do I need to put anything else in there?

that's just a presubmit hint, not an error afaik. don't edit that file. the
autogenerated file you changed actually lives in the chrome repo

> 
> dcheng@, could you take a look at gpu_messages.h? I only added an NOP IPC
> message.

[dcheng, 2015-10-06 16:00:58]

ipc changes lgtm, but two random thoughts

https://codereview.chromium.org/1331843005/diff/700001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/700001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:525: return (release <=
verified_fence_sync_release_);
No (), to be consistent with the other return statements (like the one on line
504). Also see
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Return_Values

https://codereview.chromium.org/1331843005/diff/700001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1331843005/diff/700001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.cc:247: bool
SyncPointClient::Wait(scoped_refptr<SyncPointClientState> release_state,
Here and below, why not just pass a raw pointer to SyncPointClientState? No
one's actually taking a reference to it, so using the raw pointer will make it
clear there's no ownership sharing.

[David Yen, 2015-10-06 17:09:03]

https://codereview.chromium.org/1331843005/diff/700001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1331843005/diff/700001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:525: return (release <=
verified_fence_sync_release_);
On 2015/10/06 16:00:58, dcheng wrote:
> No (), to be consistent with the other return statements (like the one on line
> 504). Also see
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Return_Values

Done.

https://codereview.chromium.org/1331843005/diff/700001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1331843005/diff/700001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.cc:247: bool
SyncPointClient::Wait(scoped_refptr<SyncPointClientState> release_state,
On 2015/10/06 16:00:58, dcheng wrote:
> Here and below, why not just pass a raw pointer to SyncPointClientState? No
> one's actually taking a reference to it, so using the raw pointer will make it
> clear there's no ownership sharing.

Done.

[David Yen, 2015-10-06 17:09:28]

On 2015/10/06 14:55:47, jam wrote:
> On 2015/10/05 23:11:16, David Yen wrote:
> > On 2015/10/05 23:00:36, piman (slow to review) wrote:
> > > LGTM. Thanks, I had missed the min(), I think I convinced myself that this
> is
> > > right. Thanks for the additional tests!
> > 
> > Thanks for the review!
> > 
> > I just need an LGTM for mojo and gpu_message.h now.
> > 
> > jam@, could you take a look at the mojo changes? I already have an LGTM from
> > rjkroege@ and peng@. The presubmit check wanted me to add an entry in
> > "third_party/mojo/README.chromium", do I need to put anything else in there?
> 
> that's just a presubmit hint, not an error afaik. don't edit that file. the
> autogenerated file you changed actually lives in the chrome repo
> 
> > 
> > dcheng@, could you take a look at gpu_messages.h? I only added an NOP IPC
> > message.

Ok, I reverted the file.

[dcheng, 2015-10-06 17:10:34]

lgtm

[jam, 2015-10-07 17:33:29]

lgtm

[David Yen, 2015-10-07 17:41:35]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-10-07 17:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, penghuang@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1331843005/#ps720001
(title: "Reverted mojo readme, changed wait() to take a pointer")

[David Yen, 2015-10-07 17:46:05]

The CQ bit was unchecked by dyen@chromium.org

[David Yen, 2015-10-07 17:46:06]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-10-07 17:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1331843005/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1331843005/720001

[commit-bot: I haz the power, 2015-10-07 18:58:46]

Committed patchset #38 (id:720001)

[commit-bot: I haz the power, 2015-10-07 18:59:23]

Patchset 38 (id:??) landed as
https://crrev.com/5c38a030b88f43b57f404dc60d777572b33ea3c4
Cr-Commit-Position: refs/heads/master@{#352901}