Improve sync object related command handling.

Improve sync object related command handling.

BUG=429053, 604794
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/22abf1da88a9ef1fb1d800bc8c58a6d3aeec7082
Cr-Commit-Position: refs/heads/master@{#401755}

[Zhenyao Mo, 2016-06-22 23:46:45]

Description was changed from

==========
Improve sync object related command handling.

BUG=429053,604794
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Improve sync object related command handling.

BUG=429053,604794
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-06-22 23:47:43]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2016-06-22 23:59:28]

piman: PTAL
kbr, cwallez: FYI

This should fix the random failures of negativefragmentapi.html

[Zhenyao Mo, 2016-06-22 23:59:34]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 23:59:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095493002/40001

[piman, 2016-06-23 00:09:34]

Thanks, one thing...

https://codereview.chromium.org/2095493002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2095493002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:15394: }
Should we force GL_SYNC_FLUSH_COMMANDS_BIT? Otherwise glClientWaitSync could
hang forever.

[Zhenyao Mo, 2016-06-23 00:22:33]

https://codereview.chromium.org/2095493002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2095493002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:15394: }
On 2016/06/23 00:09:34, piman wrote:
> Should we force GL_SYNC_FLUSH_COMMANDS_BIT? Otherwise glClientWaitSync could
> hang forever.

I thought about this, but didn't do it because I also want to add a limit to
timeout, so decided to leave that to a separate CL. I'll force flags here then
with a TODO for timeout.

[Zhenyao Mo, 2016-06-23 00:26:13]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[Zhenyao Mo, 2016-06-23 00:26:56]

Revised. Please take another look.

[commit-bot: I haz the power, 2016-06-23 00:27:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095493002/60001

[piman, 2016-06-23 00:58:32]

https://codereview.chromium.org/2095493002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2095493002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:15392: if (flags !=
GL_SYNC_FLUSH_COMMANDS_BIT) {
Oh, what I was thinking is, add GL_SYNC_FLUSH_COMMANDS_BIT even if the user
didn't specify it.
It is very possible that the user did things right and is allowed to not force a
flush (technically we /could/ track and verify this), and so we shouldn't fail,
but if we don't trust the user to do it right, we should force a flush to make
sure we don't block forever.

Then, I don't think we necessarily need a limit on timeout, in the sense that
it'll wait no longer than glFinish, and we already allow glFinish.

[Zhenyao Mo, 2016-06-23 01:07:46]

Please review again. Thanks.

https://codereview.chromium.org/2095493002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2095493002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:15392: if (flags !=
GL_SYNC_FLUSH_COMMANDS_BIT) {
On 2016/06/23 00:58:32, piman wrote:
> Oh, what I was thinking is, add GL_SYNC_FLUSH_COMMANDS_BIT even if the user
> didn't specify it.
> It is very possible that the user did things right and is allowed to not force
a
> flush (technically we /could/ track and verify this), and so we shouldn't
fail,
> but if we don't trust the user to do it right, we should force a flush to make
> sure we don't block forever.
> 
> Then, I don't think we necessarily need a limit on timeout, in the sense that
> it'll wait no longer than glFinish, and we already allow glFinish.

Sounds good. Done.

[piman, 2016-06-23 02:55:12]

LGTM, thanks!

[Corentin Wallez, 2016-06-23 14:15:09]

On 2016/06/23 at 02:55:12, piman wrote:
> LGTM, thanks!

LGTM too!

[Zhenyao Mo, 2016-06-23 20:09:46]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 20:10:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095493002/80001

[commit-bot: I haz the power, 2016-06-23 21:49:49]

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

[commit-bot: I haz the power, 2016-06-23 21:49:50]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-06-23 23:26:24]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2016-06-23 23:26:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095493002/80001

[commit-bot: I haz the power, 2016-06-23 23:33:48]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-06-23 23:35:25]

Description was changed from

==========
Improve sync object related command handling.

BUG=429053,604794
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Improve sync object related command handling.

BUG=429053,604794
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/22abf1da88a9ef1fb1d800bc8c58a6d3aeec7082
Cr-Commit-Position: refs/heads/master@{#401755}
==========

[commit-bot: I haz the power, 2016-06-23 23:35:27]

Patchset 4 (id:??) landed as
https://crrev.com/22abf1da88a9ef1fb1d800bc8c58a6d3aeec7082
Cr-Commit-Position: refs/heads/master@{#401755}