|
|
DescriptionVerify resource provider sync tokens before sending to parent.
R=piman@chromium.org
BUG=514815
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/882d69d97b5c43b7579bc610722dbadd328ac63e
Cr-Commit-Position: refs/heads/master@{#367727}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Verify new sync token in mailbox holder #Patch Set 3 : Insert sync token after transfer all resources again #Patch Set 4 : rebase #Patch Set 5 : Fix verify sync token chromium order #Patch Set 6 : No longer insert dummy fence syncs, just use EnsureWorkVisible() #Patch Set 7 : Fixed merge error #Patch Set 8 : Fixed unit test so it doesn't expect dummy fence sync #
Messages
Total messages: 35 (11 generated)
Description was changed from ========== Verify resource provider sync tokens before sending to parent. R=piman@chromium.org BUG=514815 ========== to ========== Verify resource provider sync tokens before sending to parent. R=piman@chromium.org BUG=514815 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/1479673003/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1479673003/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.cc:1152: unverified_sync_tokens.push_back(new_sync_token.GetData()); Actually, the VerifySyncTokensCHROMIUM below will tag the local copy of the SyncToken, new_sync_token, not the copy in the mailbox_holder. I think you want to move this line after l.1154 and be unverified_sync_tokens.push_back(resource.mailbox_holder.sync_token.GetData()); Or maybe even combine that with the code on l.1156, i.e. remove the else.
https://codereview.chromium.org/1479673003/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1479673003/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.cc:1152: unverified_sync_tokens.push_back(new_sync_token.GetData()); On 2015/11/26 02:32:44, piman (slow to review) wrote: > Actually, the VerifySyncTokensCHROMIUM below will tag the local copy of the > SyncToken, new_sync_token, not the copy in the mailbox_holder. > > I think you want to move this line after l.1154 and be > unverified_sync_tokens.push_back(resource.mailbox_holder.sync_token.GetData()); > > Or maybe even combine that with the code on l.1156, i.e. remove the else. Done.
lgtm
On 2015/12/08 02:27:33, piman (OOO back 12-11) wrote: > lgtm In fixing the unit tests, one of the validations is that the new sync point was inserted after all of the glGenMailboxCHROMIUM() and glProduceTextureDirectCHROMIUM() calls which is done in ResourceProvider::TransferResource() for each item in the loop. That seems to make sense, it also means we can't create the new sync token within the loop. I moved the new sync token generation outside of the loop again and assigned it in a separate loop afterwards (similar to the structure before). I'll let you review this again before landing it. PTAL.
Good point. LGTM, and thanks for the test.
The CQ bit was checked by dyen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1479673003/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479673003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479673003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479673003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): 1. This is most likely command buffer implementation dependent (it was the case for mojo) but I had to actually generate a fence sync command in order to be sure a flush actually does something. For that reason I've changed VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. 2. I also thought of a subtle race condition that could happen, since we are ensuring that ordering barriers are flushed within CanWaitUnverifiedSyncToken(), we have to do the synchronization validation after all the CanWaitUnverifiedSyncToken() calls. For that reason I have restructured the function to loop through the sync tokens twice. This requires an extra memcpy call for the first loop, but I think it is still faster to do the memcpy on the stack rather than allocate a vector of sync tokens on the heap. I also added a test to be sure the order is done correctly (VerifySyncTokensCHROMIUM_Sequence). GLES2Implementation::VerifySyncTokensCHROMIUM() had changed quite a bit so I'll let you re-review this again. PTAL.
On Thu, Dec 17, 2015 at 6:13 AM, <dyen@chromium.org> wrote: > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > >> Try jobs failed on following builders: >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, >> > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > ) > > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): > > 1. This is most likely command buffer implementation dependent (it was the > case > for mojo) but I had to actually generate a fence sync command in order to > be > sure a flush actually does something. For that reason I've changed > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. > I'd like to understand this "flush actually does something". Is it because of the last_put_offset_ == put_offset early exit? That shouldn't be a problem - it would only trigger if no commands were inserted since the last flush. For mojo, it may trigger even without an actual Flush, because OrderingBarrier isn't optimized and behaves exactly like a Flush, but the result is the same - when Flush returns, it's guaranteed that an IPC has been sent that makes the commands up to put_offset visible to the service side; it doesn't matter whether that IPC was sent from the Flush itself, or some time in the past. In either case it's valid to do the round-trip, and that will satisfy the guarantee that the service side has seen that IPC. Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, which forces an extra flush, but I don't think it's needed because of the above - or maybe I missed something, but I would like to understand it (and it probably needs to be documented). 2. I also thought of a subtle race condition that could happen, since we are > ensuring that ordering barriers are flushed within > CanWaitUnverifiedSyncToken(), > we have to do the synchronization validation after all the > CanWaitUnverifiedSyncToken() calls. For that reason I have restructured the > function to loop through the sync tokens twice. This requires an extra > memcpy > call for the first loop, but I think it is still faster to do the memcpy > on the > stack rather than allocate a vector of sync tokens on the heap. I also > added a > test to be sure the order is done correctly > (VerifySyncTokensCHROMIUM_Sequence). > Ok. I wonder if it could make sense to set the verified bit directly on the byte array - it's a bit of a hack but would save those copies. We can make a function that sets the bit directly (reinterpret_cast the byte array to bool*), and defend it with a static_assert(offsetof(SyncToken, verified_flush_)==0) to make sure we're touching the correct field. > > GLES2Implementation::VerifySyncTokensCHROMIUM() had changed quite a bit so > I'll > let you re-review this again. PTAL. > > https://codereview.chromium.org/1479673003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/17 06:16:39, piman (in KST timezone) wrote: > On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: > > > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > > > >> Try jobs failed on following builders: > >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > >> > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > ) > > > > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): > > > > 1. This is most likely command buffer implementation dependent (it was the > > case > > for mojo) but I had to actually generate a fence sync command in order to > > be > > sure a flush actually does something. For that reason I've changed > > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and > > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. > > > > I'd like to understand this "flush actually does something". Is it because > of the last_put_offset_ == put_offset early exit? That shouldn't be a > problem - it would only trigger if no commands were inserted since the last > flush. For mojo, it may trigger even without an actual Flush, because > OrderingBarrier isn't optimized and behaves exactly like a Flush, but the > result is the same - when Flush returns, it's guaranteed that an IPC has > been sent that makes the commands up to put_offset visible to the service > side; it doesn't matter whether that IPC was sent from the Flush itself, or > some time in the past. In either case it's valid to do the round-trip, and > that will satisfy the guarantee that the service side has seen that IPC. > > Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, > which forces an extra flush, but I don't think it's needed because of the > above - or maybe I missed something, but I would like to understand it (and > it probably needs to be documented). > Yeah the extra command is not really needed, the only thing that is needed is to flush any pending ordering barriers and issue a synchronous IPC to verify the sync token. Guaranteeing a command is in the buffer to be flushed allows the rest of the system do the validation. This seems like the easiest way apart from writing yet another gpu_control function to handle this case. > 2. I also thought of a subtle race condition that could happen, since we are > > ensuring that ordering barriers are flushed within > > CanWaitUnverifiedSyncToken(), > > we have to do the synchronization validation after all the > > CanWaitUnverifiedSyncToken() calls. For that reason I have restructured the > > function to loop through the sync tokens twice. This requires an extra > > memcpy > > call for the first loop, but I think it is still faster to do the memcpy > > on the > > stack rather than allocate a vector of sync tokens on the heap. I also > > added a > > test to be sure the order is done correctly > > (VerifySyncTokensCHROMIUM_Sequence). > > > > Ok. I wonder if it could make sense to set the verified bit directly on the > byte array - it's a bit of a hack but would save those copies. We can make > a function that sets the bit directly (reinterpret_cast the byte array to > bool*), and defend it with a static_assert(offsetof(SyncToken, > verified_flush_)==0) to make sure we're touching the correct field. > > That could work, I'm not too concerned with the performance of this part yet since the sync_token is not that big and it's all done on the stack (IE in cache). > > > > GLES2Implementation::VerifySyncTokensCHROMIUM() had changed quite a bit so > > I'll > > let you re-review this again. PTAL. > > > > https://codereview.chromium.org/1479673003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Dec 17, 2015 at 3:31 PM, <dyen@chromium.org> wrote: > On 2015/12/17 06:16:39, piman (in KST timezone) wrote: > > On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: >> > > > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: >> > >> >> Try jobs failed on following builders: >> >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, >> >> >> > >> > >> > >> > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > >> > ) >> > >> > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): >> > >> > 1. This is most likely command buffer implementation dependent (it was >> the >> > case >> > for mojo) but I had to actually generate a fence sync command in order >> to >> > be >> > sure a flush actually does something. For that reason I've changed >> > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and >> > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. >> > >> > > I'd like to understand this "flush actually does something". Is it because >> of the last_put_offset_ == put_offset early exit? That shouldn't be a >> problem - it would only trigger if no commands were inserted since the >> last >> flush. For mojo, it may trigger even without an actual Flush, because >> OrderingBarrier isn't optimized and behaves exactly like a Flush, but the >> result is the same - when Flush returns, it's guaranteed that an IPC has >> been sent that makes the commands up to put_offset visible to the service >> side; it doesn't matter whether that IPC was sent from the Flush itself, >> or >> some time in the past. In either case it's valid to do the round-trip, and >> that will satisfy the guarantee that the service side has seen that IPC. >> > > Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, >> which forces an extra flush, but I don't think it's needed because of the >> above - or maybe I missed something, but I would like to understand it >> (and >> it probably needs to be documented). >> > > > Yeah the extra command is not really needed, the only thing that is needed > is to > flush any pending ordering barriers and issue a synchronous IPC to verify > the > sync token. Guaranteeing a command is in the buffer to be flushed allows > the > rest of the system do the validation. This seems like the easiest way > apart from > writing yet another gpu_control function to handle this case. But why is Flush() not enough? > > > 2. I also thought of a subtle race condition that could happen, since we >> are >> > ensuring that ordering barriers are flushed within >> > CanWaitUnverifiedSyncToken(), >> > we have to do the synchronization validation after all the >> > CanWaitUnverifiedSyncToken() calls. For that reason I have restructured >> the >> > function to loop through the sync tokens twice. This requires an extra >> > memcpy >> > call for the first loop, but I think it is still faster to do the memcpy >> > on the >> > stack rather than allocate a vector of sync tokens on the heap. I also >> > added a >> > test to be sure the order is done correctly >> > (VerifySyncTokensCHROMIUM_Sequence). >> > >> > > Ok. I wonder if it could make sense to set the verified bit directly on the >> byte array - it's a bit of a hack but would save those copies. We can make >> a function that sets the bit directly (reinterpret_cast the byte array to >> bool*), and defend it with a static_assert(offsetof(SyncToken, >> verified_flush_)==0) to make sure we're touching the correct field. >> > > > > That could work, I'm not too concerned with the performance of this part > yet > since the sync_token is not that big and it's all done on the stack (IE in > cache). > > > >> > GLES2Implementation::VerifySyncTokensCHROMIUM() had changed quite a bit >> so >> > I'll >> > let you re-review this again. PTAL. >> > >> > https://codereview.chromium.org/1479673003/ >> > >> > > -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> 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/1479673003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/17 06:44:47, piman (in KST timezone) wrote: > On Thu, Dec 17, 2015 at 3:31 PM, <mailto:dyen@chromium.org> wrote: > > > On 2015/12/17 06:16:39, piman (in KST timezone) wrote: > > > > On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: > >> > > > > > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > >> > > >> >> Try jobs failed on following builders: > >> >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > >> >> > >> > > >> > > >> > > >> > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > >> > ) > >> > > >> > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): > >> > > >> > 1. This is most likely command buffer implementation dependent (it was > >> the > >> > case > >> > for mojo) but I had to actually generate a fence sync command in order > >> to > >> > be > >> > sure a flush actually does something. For that reason I've changed > >> > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and > >> > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. > >> > > >> > > > > I'd like to understand this "flush actually does something". Is it because > >> of the last_put_offset_ == put_offset early exit? That shouldn't be a > >> problem - it would only trigger if no commands were inserted since the > >> last > >> flush. For mojo, it may trigger even without an actual Flush, because > >> OrderingBarrier isn't optimized and behaves exactly like a Flush, but the > >> result is the same - when Flush returns, it's guaranteed that an IPC has > >> been sent that makes the commands up to put_offset visible to the service > >> side; it doesn't matter whether that IPC was sent from the Flush itself, > >> or > >> some time in the past. In either case it's valid to do the round-trip, and > >> that will satisfy the guarantee that the service side has seen that IPC. > >> > > > > Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, > >> which forces an extra flush, but I don't think it's needed because of the > >> above - or maybe I missed something, but I would like to understand it > >> (and > >> it probably needs to be documented). > >> > > > > > > Yeah the extra command is not really needed, the only thing that is needed > > is to > > flush any pending ordering barriers and issue a synchronous IPC to verify > > the > > sync token. Guaranteeing a command is in the buffer to be flushed allows > > the > > rest of the system do the validation. This seems like the easiest way > > apart from > > writing yet another gpu_control function to handle this case. > > > But why is Flush() not enough? > So the flush just becomes a no-op since the put offset didn't change. Since no flush occurred the fence release is not set as flushed which causes the sync token verification to fail. On the other hand, generating a fence sync which is not a part of any command buffer seems like an error. Although in this case it can never be passed anywhere so there is no technical danger, but that is why the logic doesn't handle it.
On Thu, Dec 17, 2015 at 3:44 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Thu, Dec 17, 2015 at 3:31 PM, <dyen@chromium.org> wrote: > >> On 2015/12/17 06:16:39, piman (in KST timezone) wrote: >> >> On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: >>> >> >> > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: >>> > >>> >> Try jobs failed on following builders: >>> >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, >>> >> >>> > >>> > >>> > >>> >> >> >> http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... >> >>> > ) >>> > >>> > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): >>> > >>> > 1. This is most likely command buffer implementation dependent (it was >>> the >>> > case >>> > for mojo) but I had to actually generate a fence sync command in order >>> to >>> > be >>> > sure a flush actually does something. For that reason I've changed >>> > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and >>> > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. >>> > >>> >> >> I'd like to understand this "flush actually does something". Is it because >>> of the last_put_offset_ == put_offset early exit? That shouldn't be a >>> problem - it would only trigger if no commands were inserted since the >>> last >>> flush. For mojo, it may trigger even without an actual Flush, because >>> OrderingBarrier isn't optimized and behaves exactly like a Flush, but the >>> result is the same - when Flush returns, it's guaranteed that an IPC has >>> been sent that makes the commands up to put_offset visible to the service >>> side; it doesn't matter whether that IPC was sent from the Flush itself, >>> or >>> some time in the past. In either case it's valid to do the round-trip, >>> and >>> that will satisfy the guarantee that the service side has seen that IPC. >>> >> >> Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, >>> which forces an extra flush, but I don't think it's needed because of the >>> above - or maybe I missed something, but I would like to understand it >>> (and >>> it probably needs to be documented). >>> >> >> >> Yeah the extra command is not really needed, the only thing that is >> needed is to >> flush any pending ordering barriers and issue a synchronous IPC to verify >> the >> sync token. Guaranteeing a command is in the buffer to be flushed allows >> the >> rest of the system do the validation. This seems like the easiest way >> apart from >> writing yet another gpu_control function to handle this case. > > > But why is Flush() not enough? > To be clear: - on mojo, Flush() is enough to guarantee pending ordering barriers are flushed, because they're always flushed immediately anyway (OrderingBarrier == Flush there). - on CommandBufferProxyImpl/GpuChannelHost, Flush will also provide the guarantee: - there's only at most one OrderingBarrier pending - if the OrderingBarrier is from another command buffer, it will be flushed before doing anything - if the OrderingBarrier is from the same command buffer, then last_barrier_put_offset_ > last_put_offset_ (otherwise we wouldn't have kept it), so put_offset >= last_barrier_put_offset_ > last_put_offset_, so we will flush. Antoine > > > >> >> >> 2. I also thought of a subtle race condition that could happen, since we >>> are >>> > ensuring that ordering barriers are flushed within >>> > CanWaitUnverifiedSyncToken(), >>> > we have to do the synchronization validation after all the >>> > CanWaitUnverifiedSyncToken() calls. For that reason I have >>> restructured the >>> > function to loop through the sync tokens twice. This requires an extra >>> > memcpy >>> > call for the first loop, but I think it is still faster to do the >>> memcpy >>> > on the >>> > stack rather than allocate a vector of sync tokens on the heap. I also >>> > added a >>> > test to be sure the order is done correctly >>> > (VerifySyncTokensCHROMIUM_Sequence). >>> > >>> >> >> Ok. I wonder if it could make sense to set the verified bit directly on >>> the >>> byte array - it's a bit of a hack but would save those copies. We can >>> make >>> a function that sets the bit directly (reinterpret_cast the byte array to >>> bool*), and defend it with a static_assert(offsetof(SyncToken, >>> verified_flush_)==0) to make sure we're touching the correct field. >>> >> >> >> >> That could work, I'm not too concerned with the performance of this part >> yet >> since the sync_token is not that big and it's all done on the stack (IE in >> cache). >> >> > >>> > GLES2Implementation::VerifySyncTokensCHROMIUM() had changed quite a >>> bit so >>> > I'll >>> > let you re-review this again. PTAL. >>> > >>> > https://codereview.chromium.org/1479673003/ >>> > >>> >> >> -- >>> You received this message because you are subscribed to the Google Groups >>> "Chromium-reviews" group. >>> 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/1479673003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/17 06:57:00, piman (in KST timezone) wrote: > On Thu, Dec 17, 2015 at 3:44 PM, Antoine Labour <mailto:piman@chromium.org> wrote: > > > > > > > On Thu, Dec 17, 2015 at 3:31 PM, <mailto:dyen@chromium.org> wrote: > > > >> On 2015/12/17 06:16:39, piman (in KST timezone) wrote: > >> > >> On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: > >>> > >> > >> > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > >>> > > >>> >> Try jobs failed on following builders: > >>> >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > >>> >> > >>> > > >>> > > >>> > > >>> > >> > >> > >> > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > >> > >>> > ) > >>> > > >>> > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): > >>> > > >>> > 1. This is most likely command buffer implementation dependent (it was > >>> the > >>> > case > >>> > for mojo) but I had to actually generate a fence sync command in order > >>> to > >>> > be > >>> > sure a flush actually does something. For that reason I've changed > >>> > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and > >>> > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. > >>> > > >>> > >> > >> I'd like to understand this "flush actually does something". Is it because > >>> of the last_put_offset_ == put_offset early exit? That shouldn't be a > >>> problem - it would only trigger if no commands were inserted since the > >>> last > >>> flush. For mojo, it may trigger even without an actual Flush, because > >>> OrderingBarrier isn't optimized and behaves exactly like a Flush, but the > >>> result is the same - when Flush returns, it's guaranteed that an IPC has > >>> been sent that makes the commands up to put_offset visible to the service > >>> side; it doesn't matter whether that IPC was sent from the Flush itself, > >>> or > >>> some time in the past. In either case it's valid to do the round-trip, > >>> and > >>> that will satisfy the guarantee that the service side has seen that IPC. > >>> > >> > >> Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, > >>> which forces an extra flush, but I don't think it's needed because of the > >>> above - or maybe I missed something, but I would like to understand it > >>> (and > >>> it probably needs to be documented). > >>> > >> > >> > >> Yeah the extra command is not really needed, the only thing that is > >> needed is to > >> flush any pending ordering barriers and issue a synchronous IPC to verify > >> the > >> sync token. Guaranteeing a command is in the buffer to be flushed allows > >> the > >> rest of the system do the validation. This seems like the easiest way > >> apart from > >> writing yet another gpu_control function to handle this case. > > > > > > But why is Flush() not enough? > > > > To be clear: > - on mojo, Flush() is enough to guarantee pending ordering barriers are > flushed, because they're always flushed immediately anyway (OrderingBarrier > == Flush there). > - on CommandBufferProxyImpl/GpuChannelHost, Flush will also provide the > guarantee: > - there's only at most one OrderingBarrier pending > - if the OrderingBarrier is from another command buffer, it will be > flushed before doing anything > - if the OrderingBarrier is from the same command buffer, then > last_barrier_put_offset_ > last_put_offset_ (otherwise we wouldn't have > kept it), so put_offset >= last_barrier_put_offset_ > last_put_offset_, so > we will flush. > > Antoine > Yes the flush is enough to ensure there are no pending ordering barriers, that's not the issue here. The issue is forcing a synchronous IPC to happen even if there are no fence syncs to be validated on this stream. So in order to force the synchronous IPC the code inserts a dummy fence sync, flushes, and validates that dummy fence sync. The issue is if flush didn't do anything, it does not mark the dummy fence sync as being flushed, and the validation also does not do anything because there are no flushed fence syncs waiting to be validated. Another option we can do is to add a function in gpu_control which just issues a synchronous iPC, then here we would just call flush() and the new function. I am not sure if it's worth adding the new function just for this use case though, but I can understand if you don't want to cause an unnecessary flush.
On 2015/12/17 15:40:23, David Yen wrote: > On 2015/12/17 06:57:00, piman (in KST timezone) wrote: > > On Thu, Dec 17, 2015 at 3:44 PM, Antoine Labour <mailto:piman@chromium.org> > wrote: > > > > > > > > > > > On Thu, Dec 17, 2015 at 3:31 PM, <mailto:dyen@chromium.org> wrote: > > > > > >> On 2015/12/17 06:16:39, piman (in KST timezone) wrote: > > >> > > >> On Thu, Dec 17, 2015 at 6:13 AM, <mailto:dyen@chromium.org> wrote: > > >>> > > >> > > >> > On 2015/12/16 18:08:56, commit-bot: I haz the power wrote: > > >>> > > > >>> >> Try jobs failed on following builders: > > >>> >> linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > >>> >> > > >>> > > > >>> > > > >>> > > > >>> > > >> > > >> > > >> > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > >> > > >>> > ) > > >>> > > > >>> > So a couple of issues I've found here with VerifySyncTokensCHROMIUM(): > > >>> > > > >>> > 1. This is most likely command buffer implementation dependent (it was > > >>> the > > >>> > case > > >>> > for mojo) but I had to actually generate a fence sync command in order > > >>> to > > >>> > be > > >>> > sure a flush actually does something. For that reason I've changed > > >>> > VerifySyncTokensCHROMIUM() to call InsertFenceSyncCHROMIUM and > > >>> > GenSyncTokenCHROMIUM() instead of calling gpu_control calls directly. > > >>> > > > >>> > > >> > > >> I'd like to understand this "flush actually does something". Is it because > > >>> of the last_put_offset_ == put_offset early exit? That shouldn't be a > > >>> problem - it would only trigger if no commands were inserted since the > > >>> last > > >>> flush. For mojo, it may trigger even without an actual Flush, because > > >>> OrderingBarrier isn't optimized and behaves exactly like a Flush, but the > > >>> result is the same - when Flush returns, it's guaranteed that an IPC has > > >>> been sent that makes the commands up to put_offset visible to the service > > >>> side; it doesn't matter whether that IPC was sent from the Flush itself, > > >>> or > > >>> some time in the past. In either case it's valid to do the round-trip, > > >>> and > > >>> that will satisfy the guarantee that the service side has seen that IPC. > > >>> > > >> > > >> Doing an extra InsertFenceSyncCHROMIUM basically forces an extra command, > > >>> which forces an extra flush, but I don't think it's needed because of the > > >>> above - or maybe I missed something, but I would like to understand it > > >>> (and > > >>> it probably needs to be documented). > > >>> > > >> > > >> > > >> Yeah the extra command is not really needed, the only thing that is > > >> needed is to > > >> flush any pending ordering barriers and issue a synchronous IPC to verify > > >> the > > >> sync token. Guaranteeing a command is in the buffer to be flushed allows > > >> the > > >> rest of the system do the validation. This seems like the easiest way > > >> apart from > > >> writing yet another gpu_control function to handle this case. > > > > > > > > > But why is Flush() not enough? > > > > > > > To be clear: > > - on mojo, Flush() is enough to guarantee pending ordering barriers are > > flushed, because they're always flushed immediately anyway (OrderingBarrier > > == Flush there). > > - on CommandBufferProxyImpl/GpuChannelHost, Flush will also provide the > > guarantee: > > - there's only at most one OrderingBarrier pending > > - if the OrderingBarrier is from another command buffer, it will be > > flushed before doing anything > > - if the OrderingBarrier is from the same command buffer, then > > last_barrier_put_offset_ > last_put_offset_ (otherwise we wouldn't have > > kept it), so put_offset >= last_barrier_put_offset_ > last_put_offset_, so > > we will flush. > > > > Antoine > > > > Yes the flush is enough to ensure there are no pending ordering barriers, that's > not the issue here. The issue is forcing a synchronous IPC to happen even if > there are no fence syncs to be validated on this stream. > > So in order to force the synchronous IPC the code inserts a dummy fence sync, > flushes, and validates that dummy fence sync. The issue is if flush didn't do > anything, it does not mark the dummy fence sync as being flushed, and the > validation also does not do anything because there are no flushed fence syncs > waiting to be validated. > > Another option we can do is to add a function in gpu_control which just issues a > synchronous iPC, then here we would just call flush() and the new function. I am > not sure if it's worth adding the new function just for this use case though, > but I can understand if you don't want to cause an unnecessary flush. I've modified the function to use EnsureWorkVisible() now so we do not have to use a dummy fence sync anymore. This saves us a flush if nothing needs to be flushed. PTAL.
lgtm
The CQ bit was checked by dyen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1479673003/#ps140001 (title: "Fixed unit test so it doesn't expect dummy fence sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479673003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479673003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Verify resource provider sync tokens before sending to parent. R=piman@chromium.org BUG=514815 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Verify resource provider sync tokens before sending to parent. R=piman@chromium.org BUG=514815 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/882d69d97b5c43b7579bc610722dbadd328ac63e Cr-Commit-Position: refs/heads/master@{#367727} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/882d69d97b5c43b7579bc610722dbadd328ac63e Cr-Commit-Position: refs/heads/master@{#367727} |