Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1460833002:
gpu: Implement the new fence syncs in mojo command buffer. (Closed)
|
Created:
5 years, 1 month ago by Peng Modified:
5 years ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, piman+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: Implement the new fence syncs in mojo command buffer.
BUG=514815
Committed: https://crrev.com/3af1e58f871a6fd10efe23105fd20b495ab09d6c
Cr-Commit-Position: refs/heads/master@{#361517}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address issues #
Total comments: 11
Patch Set 3 : Update #
Total comments: 4
Patch Set 4 : Update #
Messages
Total messages: 26 (5 generated)
penghuang@chromium.org changed reviewers: + dyen@chromium.org
Hi David, could please take a look? Thanks.
https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:48: : command_buffer_id_(g_next_command_buffer_id.GetNext()), This needs to be globally unique across all processes. Are all of these drivers only created on a single process? From what I heard isn't there some sort of mojo unique client id we can use? https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:129: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); So this order data is used to order all commands, when a command is received and before it is enqueued an order number should be assigned to the command using "GenerateUnprocessedOrderNumber()". Upon processing the command that order number should be passed into "BeginProcessingOrderNumber()" and when it is done it should be passed into "FinishProcessingOrderNumber()". https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_local.cc:32: base::StaticAtomicSequenceNumber g_next_command_buffer_id; Same question as above. https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_local.cc:101: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); Same comment about order data as above.
https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:48: : command_buffer_id_(g_next_command_buffer_id.GetNext()), On 2015/11/19 18:12:52, David Yen wrote: > This needs to be globally unique across all processes. Are all of these drivers > only created on a single process? > > From what I heard isn't there some sort of mojo unique client id we can use? After reviewing the other CL this looks okay to me now as long as all the interacting command buffers will always be in the same process. https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.h (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.h:97: scoped_refptr<gpu::SyncPointOrderData> sync_point_order_data_; This would make it depend on the other CL, but the sync point order data probably needs to be assigned in the command task runner since that is the central location of queuing the tasks.
penghuang@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:48: : command_buffer_id_(g_next_command_buffer_id.GetNext()), On 2015/11/20 00:47:13, David Yen wrote: > On 2015/11/19 18:12:52, David Yen wrote: > > This needs to be globally unique across all processes. Are all of these > drivers > > only created on a single process? > > > > From what I heard isn't there some sort of mojo unique client id we can use? > > After reviewing the other CL this looks okay to me now as long as all the > interacting command buffers will always be in the same process. Yes. All drivers are in the same process (mus process). https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:129: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); On 2015/11/19 18:12:52, David Yen wrote: > So this order data is used to order all commands, when a command is received and > before it is enqueued an order number should be assigned to the command using > "GenerateUnprocessedOrderNumber()". > > Upon processing the command that order number should be passed into > "BeginProcessingOrderNumber()" and when it is done it should be passed into > "FinishProcessingOrderNumber()". Seems the SyncPointOrderData is used for verifying fence release. But for tasks submitted to CommandBufferTaskRunner, we don't guarantee those tasks will be executed in the submitted order. So I am not sure if the OrderData is still useful for us. piman, could you please give me some suggestion? https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_local.cc:32: base::StaticAtomicSequenceNumber g_next_command_buffer_id; On 2015/11/19 18:12:52, David Yen wrote: > Same question as above. All instances will be in the same process (mus process).
https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:129: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); On 2015/11/20 17:41:15, Peng wrote: > On 2015/11/19 18:12:52, David Yen wrote: > > So this order data is used to order all commands, when a command is received > and > > before it is enqueued an order number should be assigned to the command using > > "GenerateUnprocessedOrderNumber()". > > > > Upon processing the command that order number should be passed into > > "BeginProcessingOrderNumber()" and when it is done it should be passed into > > "FinishProcessingOrderNumber()". > > Seems the SyncPointOrderData is used for verifying fence release. But for tasks > submitted to CommandBufferTaskRunner, we don't guarantee those tasks will be > executed in the submitted order. So I am not sure if the OrderData is still > useful for us. > > piman, could you please give me some suggestion? The OrderData must live per driver, but the generation should be done on the CommandBufferTaskRunner. I would recommend adding it in "PostTask()". So while you have the lock, you do something such as: driver->sync_order_data()->AllocateOrderNum() then you attach that order number to the TaskCallback. TaskCallback can be something such as std::pair<callback, uint32>. Then in RunOneTaskInternalLocked before callback.Run() you would do: it->sync_order_data()->BeginProcessingOrderNumber(order_num); and after if(complete) you would do: it->sync_order_data()->FinishProcessingOrderNumber(order_num);
https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.cc:129: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); On 2015/11/20 18:20:06, David Yen wrote: > On 2015/11/20 17:41:15, Peng wrote: > > On 2015/11/19 18:12:52, David Yen wrote: > > > So this order data is used to order all commands, when a command is received > > and > > > before it is enqueued an order number should be assigned to the command > using > > > "GenerateUnprocessedOrderNumber()". > > > > > > Upon processing the command that order number should be passed into > > > "BeginProcessingOrderNumber()" and when it is done it should be passed into > > > "FinishProcessingOrderNumber()". > > > > Seems the SyncPointOrderData is used for verifying fence release. But for > tasks > > submitted to CommandBufferTaskRunner, we don't guarantee those tasks will be > > executed in the submitted order. So I am not sure if the OrderData is still > > useful for us. > > > > piman, could you please give me some suggestion? > > The OrderData must live per driver, but the generation should be done on the > CommandBufferTaskRunner. I would recommend adding it in "PostTask()". > > So while you have the lock, you do something such as: > driver->sync_order_data()->AllocateOrderNum() > > then you attach that order number to the TaskCallback. TaskCallback can be > something such as std::pair<callback, uint32>. Then in RunOneTaskInternalLocked > before callback.Run() you would do: > it->sync_order_data()->BeginProcessingOrderNumber(order_num); > > and after if(complete) you would do: > it->sync_order_data()->FinishProcessingOrderNumber(order_num); Done. https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_driver.h (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_driver.h:97: scoped_refptr<gpu::SyncPointOrderData> sync_point_order_data_; On 2015/11/20 00:47:14, David Yen wrote: > This would make it depend on the other CL, but the sync point order data > probably needs to be assigned in the command task runner since that is the > central location of queuing the tasks. Done. https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/1/components/mus/gles2/comman... components/mus/gles2/command_buffer_local.cc:101: sync_point_order_data_ = gpu::SyncPointOrderData::Create(); On 2015/11/19 18:12:52, David Yen wrote: > Same comment about order data as above. Done.
Mostly looks good, a couple of comments. https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_driver.cc:41: base::StaticAtomicSequenceNumber g_next_command_buffer_id; Could this be an atomic sequence in gpu_state instead? They can all be in the mojo namespace and we wouldn't need to add another namespace. Although eventually when we add out of process mojo command buffers we will probably need to add another one for that case. https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. This can probably just return true if mojo never needs to synchronize with any other process. Otherwise you can return true if sync_token->GetNamespace() is a MOJO namespace. Once we support out of process commands this function will have to check if the sync token is coming from the same process or not. https://codereview.chromium.org/1460833002/diff/20001/mojo/gles2/command_buff... File mojo/gles2/command_buffer_client_impl.cc (right): https://codereview.chromium.org/1460833002/diff/20001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:443: // All sync tokens must be flushed before being waited on. Same comment as above. Either return True if other types of command buffer synchronization will never happen, or check if the namespace is mojo.
https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_driver.cc:41: base::StaticAtomicSequenceNumber g_next_command_buffer_id; On 2015/11/23 22:06:03, David Yen wrote: > Could this be an atomic sequence in gpu_state instead? They can all be in the > mojo namespace and we wouldn't need to add another namespace. Although > eventually when we add out of process mojo command buffers we will probably need > to add another one for that case. CommadnBufferDriver/CommandBufferImpl are for out-of-process command buffers. The difference with chrome IPC is that the id here is genererated service-side instead of client-side, so that's why it looks like it could be the same namespace. I think in the future we may decide to create the id client-side, so I think we will want to separate MOJO_LOCAL from MOJO then - so why not now? https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:06:03, David Yen wrote: > This can probably just return true if mojo never needs to synchronize with any > other process. Otherwise you can return true if sync_token->GetNamespace() is a > MOJO namespace. Once we support out of process commands this function will have > to check if the sync token is coming from the same process or not. The only sync tokens that are valid to wait unverified are the CommandBufferLocal ones. The CommandBufferDriver ones come from out-of-process untrusted clients. At the same time, though, sync tokens on CommandBufferLocal are conceptually automatically verified as soon as they're Flush'ed. Indeed, the fence is already released.
https://codereview.chromium.org/1460833002/diff/20001/mojo/gles2/command_buff... File mojo/gles2/command_buffer_client_impl.cc (right): https://codereview.chromium.org/1460833002/diff/20001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:443: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:06:03, David Yen wrote: > Same comment as above. Either return True if other types of command buffer > synchronization will never happen, or check if the namespace is mojo. Hi piman, So returning false is OK here too, right?
https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_driver.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_driver.cc:41: base::StaticAtomicSequenceNumber g_next_command_buffer_id; On 2015/11/23 22:22:52, piman (slow to review) wrote: > On 2015/11/23 22:06:03, David Yen wrote: > > Could this be an atomic sequence in gpu_state instead? They can all be in the > > mojo namespace and we wouldn't need to add another namespace. Although > > eventually when we add out of process mojo command buffers we will probably > need > > to add another one for that case. > > CommadnBufferDriver/CommandBufferImpl are for out-of-process command buffers. > The difference with chrome IPC is that the id here is genererated service-side > instead of client-side, so that's why it looks like it could be the same > namespace. > I think in the future we may decide to create the id client-side, so I think we > will want to separate MOJO_LOCAL from MOJO then - so why not now? If the IDs are always going to be generated service side then this will work then. In that case let's keep this. https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:22:52, piman (slow to review) wrote: > On 2015/11/23 22:06:03, David Yen wrote: > > This can probably just return true if mojo never needs to synchronize with any > > other process. Otherwise you can return true if sync_token->GetNamespace() is > a > > MOJO namespace. Once we support out of process commands this function will > have > > to check if the sync token is coming from the same process or not. > > The only sync tokens that are valid to wait unverified are the > CommandBufferLocal ones. The CommandBufferDriver ones come from out-of-process > untrusted clients. > > At the same time, though, sync tokens on CommandBufferLocal are conceptually > automatically verified as soon as they're Flush'ed. Indeed, the fence is already > released. They would be automatically verified in that if the caller used "GenSyncToken()" it would always return immediately without doing anything, but a caller could still use "GenUnverifiedSyncToken()" and still expect it to work. In that case, it would be marked as "unverified". This should probably be "return sync_token->GetNamespaceID() == GetNamespaceID()".
https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:40:32, David Yen wrote: > On 2015/11/23 22:22:52, piman (slow to review) wrote: > > On 2015/11/23 22:06:03, David Yen wrote: > > > This can probably just return true if mojo never needs to synchronize with > any > > > other process. Otherwise you can return true if sync_token->GetNamespace() > is > > a > > > MOJO namespace. Once we support out of process commands this function will > > have > > > to check if the sync token is coming from the same process or not. > > > > The only sync tokens that are valid to wait unverified are the > > CommandBufferLocal ones. The CommandBufferDriver ones come from out-of-process > > untrusted clients. > > > > At the same time, though, sync tokens on CommandBufferLocal are conceptually > > automatically verified as soon as they're Flush'ed. Indeed, the fence is > already > > released. > > They would be automatically verified in that if the caller used "GenSyncToken()" > it would always return immediately without doing anything, but a caller could > still use "GenUnverifiedSyncToken()" and still expect it to work. In that case, > it would be marked as "unverified". > > This should probably be "return sync_token->GetNamespaceID() == > GetNamespaceID()". Ah, right, that sg.
https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:40:32, David Yen wrote: > On 2015/11/23 22:22:52, piman (slow to review) wrote: > > On 2015/11/23 22:06:03, David Yen wrote: > > > This can probably just return true if mojo never needs to synchronize with > any > > > other process. Otherwise you can return true if sync_token->GetNamespace() > is > > a > > > MOJO namespace. Once we support out of process commands this function will > > have > > > to check if the sync token is coming from the same process or not. > > > > The only sync tokens that are valid to wait unverified are the > > CommandBufferLocal ones. The CommandBufferDriver ones come from out-of-process > > untrusted clients. > > > > At the same time, though, sync tokens on CommandBufferLocal are conceptually > > automatically verified as soon as they're Flush'ed. Indeed, the fence is > already > > released. > > They would be automatically verified in that if the caller used "GenSyncToken()" > it would always return immediately without doing anything, but a caller could > still use "GenUnverifiedSyncToken()" and still expect it to work. In that case, > it would be marked as "unverified". > > This should probably be "return sync_token->GetNamespaceID() == > GetNamespaceID()". Actually, even then it would depend on if there could be multiple command buffers per process. If there is always only 1 command buffer per process then this should just return false. There is no point for a command buffer to wait on itself.
https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... File components/mus/gles2/command_buffer_local.cc (right): https://codereview.chromium.org/1460833002/diff/20001/components/mus/gles2/co... components/mus/gles2/command_buffer_local.cc:259: // All sync tokens must be flushed before being waited on. On 2015/11/23 22:42:18, David Yen wrote: > On 2015/11/23 22:40:32, David Yen wrote: > > On 2015/11/23 22:22:52, piman (slow to review) wrote: > > > On 2015/11/23 22:06:03, David Yen wrote: > > > > This can probably just return true if mojo never needs to synchronize with > > any > > > > other process. Otherwise you can return true if sync_token->GetNamespace() > > is > > > a > > > > MOJO namespace. Once we support out of process commands this function will > > > have > > > > to check if the sync token is coming from the same process or not. > > > > > > The only sync tokens that are valid to wait unverified are the > > > CommandBufferLocal ones. The CommandBufferDriver ones come from > out-of-process > > > untrusted clients. > > > > > > At the same time, though, sync tokens on CommandBufferLocal are conceptually > > > automatically verified as soon as they're Flush'ed. Indeed, the fence is > > already > > > released. > > > > They would be automatically verified in that if the caller used > "GenSyncToken()" > > it would always return immediately without doing anything, but a caller could > > still use "GenUnverifiedSyncToken()" and still expect it to work. In that > case, > > it would be marked as "unverified". > > > > This should probably be "return sync_token->GetNamespaceID() == > > GetNamespaceID()". > > Actually, even then it would depend on if there could be multiple command > buffers per process. If there is always only 1 command buffer per process then > this should just return false. There is no point for a command buffer to wait on > itself. Done.
LGTM when David is happy. https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... File mojo/gles2/command_buffer_client_impl.cc (right): https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:91: CHECK_EQ(command_buffer_namespace, gpu::CommandBufferNamespace::MOJO); nit: DCHECK_EQ https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:446: return sync_token->namespace_id() == gpu::CommandBufferNamespace::MOJO_LOCAL; It is also safe to wait on the same context (i.e. if sync_token->namespace_id() == gpu::CommandBufferNamespace::MOJO && sync_token->command_buffer_id() == GetCommandBufferID()
LGTM although you will need mojo reviewers, thanks for doing this.
https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... File mojo/gles2/command_buffer_client_impl.cc (right): https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:91: CHECK_EQ(command_buffer_namespace, gpu::CommandBufferNamespace::MOJO); On 2015/11/24 21:49:45, piman (slow to review) wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/1460833002/diff/40001/mojo/gles2/command_buff... mojo/gles2/command_buffer_client_impl.cc:446: return sync_token->namespace_id() == gpu::CommandBufferNamespace::MOJO_LOCAL; On 2015/11/24 21:49:45, piman (slow to review) wrote: > It is also safe to wait on the same context (i.e. if sync_token->namespace_id() > == gpu::CommandBufferNamespace::MOJO && sync_token->command_buffer_id() == > GetCommandBufferID() Done.
penghuang@chromium.org changed reviewers: + ben@chromium.org
Hi Ben, PTAL. Thanks.
rubber stamp lgtm
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dyen@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1460833002/#ps60001 (title: "Update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460833002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3af1e58f871a6fd10efe23105fd20b495ab09d6c Cr-Commit-Position: refs/heads/master@{#361517} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
