Implementing FlushMappedBufferRange in GPU command buffer.

Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

Committed: https://crrev.com/1f5a5a2f766dbaeb047e4a39e647be420f3530e9
Cr-Commit-Position: refs/heads/master@{#423035}

[Zhenyao Mo, 2016-08-08 20:05:53]

Description was changed from

==========
Implemeng FlushMappedBufferRange in GPU command buffer.

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

to

==========
Implemeng FlushMappedBufferRange in GPU command buffer.

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

[Zhenyao Mo, 2016-08-08 20:24:36]

Patchset #2 (id:20001) has been deleted

[Zhenyao Mo, 2016-08-08 20:24:47]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2016-08-08 21:14:52]

Patchset #2 (id:60001) has been deleted

[Zhenyao Mo, 2016-08-08 21:15:36]

Patchset #1 (id:40001) has been deleted

[Zhenyao Mo, 2016-08-08 21:16:23]

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

[commit-bot: I haz the power, 2016-08-08 21:16:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2016-08-08 21:18:56]

piman: PTAL

Can't believe I missed a command at this point

[commit-bot: I haz the power, 2016-08-08 22:14:08]

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

[commit-bot: I haz the power, 2016-08-08 22:14:09]

Dry run: This issue passed the CQ dry run.

[piman, 2016-08-08 22:55:23]

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

https://codereview.chromium.org/2221173002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16589:
glFlushMappedBufferRange(target, offset, size);
This in itself won't do anything - the client side wrote the data into the
shared memory, but we need to copy the shared memory into the mapped GL buffer
before doing the flush.

Separately, we also should do the maintenance of the buffer's range cache /
shadow buffer if appropriate (see also crbug.com/491002). Actually, that's also
true of UnmapBuffer if GL_MAP_FLUSH_EXPLICIT_BIT is not set.

[Zhenyao Mo, 2016-10-04 21:32:37]

Description was changed from

==========
Implemeng FlushMappedBufferRange in GPU command buffer.

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

to

==========
Implemeng FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

[Zhenyao Mo, 2016-10-04 21:32:37]

zmo@chromium.org changed reviewers:
+ kbr@chromium.org

[Zhenyao Mo, 2016-10-04 21:32:42]

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

[commit-bot: I haz the power, 2016-10-04 21:33:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2016-10-04 21:33:56]

kbr: Can you review since piman is out of office?
vmiura, kainino, bsalomon: FYI (feel free to review)

[vmiura, 2016-10-04 22:18:52]

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

[vmiura, 2016-10-04 22:18:53]

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16864: if (size == 0) {
I think size == 0 should not generate a GL error.  Perhaps just return kNoError,
after all other error checks are done?

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16881: }
Could you add check for "GL_INVALID_VALUE is generated if access has any bits
set other than those defined above."

if ( access & (~GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT
| GL_MAP_INVALIDATE_BUFFER_BIT | GL_MAP_FLUSH_EXPLICIT_BIT |
GL_MAP_UNSYNCHRONIZED_BIT)) {
  ...error
}

[Zhenyao Mo, 2016-10-04 22:30:28]

vmiura: revised. please take another look.

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16864: if (size == 0) {
On 2016/10/04 22:18:52, vmiura wrote:
> I think size == 0 should not generate a GL error.  Perhaps just return
kNoError,
> after all other error checks are done?

This is explicitly required by the spec, ES 3.04, page 39, the first
INVALID_OPERATION condition.

https://codereview.chromium.org/2221173002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16881: }
On 2016/10/04 22:18:52, vmiura wrote:
> Could you add check for "GL_INVALID_VALUE is generated if access has any bits
> set other than those defined above."
> 
> if ( access & (~GL_MAP_READ_BIT | GL_MAP_WRITE_BIT |
GL_MAP_INVALIDATE_RANGE_BIT
> | GL_MAP_INVALIDATE_BUFFER_BIT | GL_MAP_FLUSH_EXPLICIT_BIT |
> GL_MAP_UNSYNCHRONIZED_BIT)) {
>   ...error
> }

Done.

[vmiura, 2016-10-04 22:34:53]

LGTM

[Zhenyao Mo, 2016-10-04 22:41:24]

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

[commit-bot: I haz the power, 2016-10-04 22:42:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 23:17:31]

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

[commit-bot: I haz the power, 2016-10-04 23:17:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ken Russell (switch to Gerrit), 2016-10-04 23:40:17]

Description was changed from

==========
Implemeng FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

to

==========
Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

[Ken Russell (switch to Gerrit), 2016-10-04 23:41:32]

I defer to vmiura's review.

[Zhenyao Mo, 2016-10-05 00:23:43]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-10-05 00:23:43]

The patchset sent to the CQ was uploaded after l-g-t-m from vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/2221173002/#ps150001
(title: "attempt to fix win build")

[commit-bot: I haz the power, 2016-10-05 00:24:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 01:20:21]

Description was changed from

==========
Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

to

==========
Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

[commit-bot: I haz the power, 2016-10-05 01:20:22]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2016-10-05 01:23:44]

Description was changed from

==========
Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

to

==========
Implementing FlushMappedBufferRange in GPU command buffer.

Also adding some missing validation for MapBufferRange.

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

Committed: https://crrev.com/1f5a5a2f766dbaeb047e4a39e647be420f3530e9
Cr-Commit-Position: refs/heads/master@{#423035}
==========

[commit-bot: I haz the power, 2016-10-05 01:23:45]

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