Separate out resource mailbox creation and fix synchronization issue.

Separate out resource mailbox creation and fix synchronization issue.

The resource transfer function has been split up so it does the lazy
resource mailbox creation in a separate pass. This allows us to setup
the sync token directly on the resource itself before the transfer.

The texture binding code is also done on the first pass, this fixes
a subtle synchronization issue where previously the bound textures
could potentially not have a sync token attached to it if no mailboxes
were lazily created.

BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
Cr-Commit-Position: refs/heads/master@{#373847}

[David Yen, 2016-02-04 19:05:44]

Description was changed from

==========
Separate out resource mailbox creation and fix synchronization issue.

The resource transfer function has been split up so it does the lazy
resource mailbox creation in a separate pass. This allows us to setup
the sync token directly on the resource itself before the transfer.

The texture binding code is also done on the first pass, this fixes
a subtle synchronization issue where previously the bound textures
could potentially not have a sync token attached to it if no mailboxes
were lazily created.

BUG=584381
==========

to

==========
Separate out resource mailbox creation and fix synchronization issue.

The resource transfer function has been split up so it does the lazy
resource mailbox creation in a separate pass. This allows us to setup
the sync token directly on the resource itself before the transfer.

The texture binding code is also done on the first pass, this fixes
a subtle synchronization issue where previously the bound textures
could potentially not have a sync token attached to it if no mailboxes
were lazily created.

BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[David Yen, 2016-02-04 19:09:27]

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.cc:1170:
need_synchronization_indexes.push_back(it);
From piman@:
We don't need to create sync tokens if delegated_sync_points_required is not
set. We could either skip the 2 code paths below, or I think simply not add it
to created_mailbox_indexes here.

The other option, I think, is to call CreateResourceMailbox from LazyCreate. I
don't think there's any point in delaying it any more - it used to require a
sync round-trip to the GPU process, so we needed to batch, but now it's all
client-side and a lot cheaper compared to everything else. Also with surfaces
used for the UI, there's very few cases left, if any, where we ever create a
resource that will not be sent to a parent, so I think delaying the mailbox
creation is a false optimization at this point.

[David Yen, 2016-02-04 19:13:28]

dyen@chromium.org changed reviewers:
+ piman@chromium.org, reveman@chromium.org

[David Yen, 2016-02-04 19:13:29]

This splits up the TransferResource calls into 2 passes, it also fixes the
subtle zero copy synchronization bug we discussed earlier.

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.cc:1170:
need_synchronization_indexes.push_back(it);
On 2016/02/04 19:09:27, David Yen wrote:
> From piman@:
> We don't need to create sync tokens if delegated_sync_points_required is not
> set. We could either skip the 2 code paths below, or I think simply not add it
> to created_mailbox_indexes here.
> 
> The other option, I think, is to call CreateResourceMailbox from LazyCreate. I
> don't think there's any point in delaying it any more - it used to require a
> sync round-trip to the GPU process, so we needed to batch, but now it's all
> client-side and a lot cheaper compared to everything else. Also with surfaces
> used for the UI, there's very few cases left, if any, where we ever create a
> resource that will not be sent to a parent, so I think delaying the mailbox
> creation is a false optimization at this point.

Done. Added delegated_sync_points_required check.

I'm fine with leaving the lazy mailbox creation here, because we still need to
synchronize the lazy texture binding code which was not previously synchronized.

[piman, 2016-02-04 19:27:02]

lgtm

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:540: bool
CreateMailboxAndBindResource(gpu::gles2::GLES2Interface* gl,
nit: maybe document what the return value is meant to represent.

[David Yen, 2016-02-04 19:40:16]

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1666203003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:540: bool
CreateMailboxAndBindResource(gpu::gles2::GLES2Interface* gl,
On 2016/02/04 19:27:02, piman wrote:
> nit: maybe document what the return value is meant to represent.

Done.

[reveman, 2016-02-04 20:57:35]

lgtm, thanks

[David Yen, 2016-02-04 23:02:19]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-02-04 23:02:19]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1666203003/#ps40001
(title: "Removed assumption that sync token is cleared")

[commit-bot: I haz the power, 2016-02-04 23:04:39]

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

[commit-bot: I haz the power, 2016-02-05 00:09:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-05 00:09:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Yen, 2016-02-05 17:35:56]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2016-02-05 17:36:28]

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

[commit-bot: I haz the power, 2016-02-05 18:12:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-05 18:14:10]

Description was changed from

==========
Separate out resource mailbox creation and fix synchronization issue.

The resource transfer function has been split up so it does the lazy
resource mailbox creation in a separate pass. This allows us to setup
the sync token directly on the resource itself before the transfer.

The texture binding code is also done on the first pass, this fixes
a subtle synchronization issue where previously the bound textures
could potentially not have a sync token attached to it if no mailboxes
were lazily created.

BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Separate out resource mailbox creation and fix synchronization issue.

The resource transfer function has been split up so it does the lazy
resource mailbox creation in a separate pass. This allows us to setup
the sync token directly on the resource itself before the transfer.

The texture binding code is also done on the first pass, this fixes
a subtle synchronization issue where previously the bound textures
could potentially not have a sync token attached to it if no mailboxes
were lazily created.

BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
Cr-Commit-Position: refs/heads/master@{#373847}
==========

[commit-bot: I haz the power, 2016-02-05 18:14:11]

Patchset 3 (id:??) landed as
https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
Cr-Commit-Position: refs/heads/master@{#373847}

[David Yen, 2016-02-06 00:41:09]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1678613002/ by dyen@chromium.org.

The reason for reverting is: Causing renderer synchronization issues.