Add a transform feedback workaround for Intel GPUs on MacOSX

Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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/0494f634cad9c20494a458e79120f10cca315582
Cr-Commit-Position: refs/heads/master@{#413412}

[jchen10, 2016-08-18 06:51:04]

Description was changed from

==========
Add a MacOSX workaround for Intel GPUs

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
==========

to

==========
Add a MacOSX workaround for Intel GPUs

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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
==========

[jchen10, 2016-08-18 07:02:05]

Description was changed from

==========
Add a MacOSX workaround for Intel GPUs

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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

==========
Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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
==========

[jchen10, 2016-08-18 07:02:05]

jie.a.chen@intel.com changed reviewers:
+ kbr@chromium.org, qiankun.miao@intel.com, yunchao.he@intel.com,
zmo@chromium.org

[jchen10, 2016-08-18 07:08:58]

Please have a look, Thanks!
This flaw has only been found on Intel GPUs so far.

[jchen10, 2016-08-18 07:53:53]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 07:54:19]

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-08-18 07:54:20]

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

[commit-bot: I haz the power, 2016-08-18 07:54:21]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Zhenyao Mo, 2016-08-18 17:39:31]

Mostly looks good

Are there failed tests passing because of this? If yes, can we also update the
test expectations?

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5718:
state_.bound_transform_feedback->DoBindTransformFeedback(
I think you should just do glBindTransformFeedback(GL_TRANSFORM_FEEDBACK,
state_.bound_transform_feedback->service_id()) here to avoid all the unnecessary
steps.

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_w...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_w...
gpu/config/gpu_driver_bug_workaround_type.h:160:
GPU_OP(USE_TRANSFORM_FEEDBACK_TEMP_UNBIND,                 \
Maybe TRANSFORM_FEEDBACK_REBIND_BEFORE_RESUME?

[Zhenyao Mo, 2016-08-18 18:49:05]

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5718:
state_.bound_transform_feedback->DoBindTransformFeedback(
On 2016/08/18 17:39:31, Zhenyao Mo wrote:
> I think you should just do glBindTransformFeedback(GL_TRANSFORM_FEEDBACK,
> state_.bound_transform_feedback->service_id()) here to avoid all the
unnecessary
> steps.

1) I am still worried that this is caused by Chrome command buffer, so can you
do a glGetIntegerv(GL_TRANSFORM_FEEDBACK_BINDING) and see if the ID is lost? If
not, 2)

2) Do we have to Bind to 0 first then the currently bound transformfeedback?  If
we just bind it, will that work around the issue?

[Ken Russell (switch to Gerrit), 2016-08-19 00:13:36]

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5718:
state_.bound_transform_feedback->DoBindTransformFeedback(
On 2016/08/18 18:49:05, Zhenyao Mo wrote:
> On 2016/08/18 17:39:31, Zhenyao Mo wrote:
> > I think you should just do glBindTransformFeedback(GL_TRANSFORM_FEEDBACK,
> > state_.bound_transform_feedback->service_id()) here to avoid all the
> unnecessary
> > steps.
> 
> 1) I am still worried that this is caused by Chrome command buffer, so can you
> do a glGetIntegerv(GL_TRANSFORM_FEEDBACK_BINDING) and see if the ID is lost?
If
> not, 2)
> 
> 2) Do we have to Bind to 0 first then the currently bound transformfeedback? 
If
> we just bind it, will that work around the issue?

I think this is very likely a driver bug. These tests pass on my MacBook Pro
Retina with NVIDIA GPU.

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
gpu/config/gpu_driver_bug_list_json.cc:1923:
"use_transform_feedback_temp_unbind"
How about "rebind_before_resume_transform_feedback"?

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_w...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_w...
gpu/config/gpu_driver_bug_workaround_type.h:160:
GPU_OP(USE_TRANSFORM_FEEDBACK_TEMP_UNBIND,                 \
On 2016/08/18 17:39:31, Zhenyao Mo wrote:
> Maybe TRANSFORM_FEEDBACK_REBIND_BEFORE_RESUME?

Or that too.

[jchen10, 2016-08-19 01:01:23]

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5718:
state_.bound_transform_feedback->DoBindTransformFeedback(
On 2016/08/19 00:13:35, Ken Russell wrote:
> On 2016/08/18 18:49:05, Zhenyao Mo wrote:
> > On 2016/08/18 17:39:31, Zhenyao Mo wrote:
> > > I think you should just do glBindTransformFeedback(GL_TRANSFORM_FEEDBACK,
> > > state_.bound_transform_feedback->service_id()) here to avoid all the
> > unnecessary
> > > steps.
> > 
> > 1) I am still worried that this is caused by Chrome command buffer, so can
you
> > do a glGetIntegerv(GL_TRANSFORM_FEEDBACK_BINDING) and see if the ID is lost?
> If
> > not, 2)
> > 
> > 2) Do we have to Bind to 0 first then the currently bound transformfeedback?

> If
> > we just bind it, will that work around the issue?
> 
> I think this is very likely a driver bug. These tests pass on my MacBook Pro
> Retina with NVIDIA GPU.

Anyway the worry makes sense to me. I have tried them. It turned out that 
    ID queried from GL_TRANSFORM_FEEDBACK_BINDING was same as
state_.bound_transform_feedback->service_id(), and
    the walk around didn't work without binding to 0 first.
So I think it's more likely a driver bug.

[jchen10, 2016-08-19 01:34:43]

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
gpu/config/gpu_driver_bug_list_json.cc:1923:
"use_transform_feedback_temp_unbind"
On 2016/08/19 00:13:36, Ken Russell wrote:
> How about "rebind_before_resume_transform_feedback"?

Or "rebind_transform_feedback_before_resume"?

[Ken Russell (switch to Gerrit), 2016-08-19 01:39:25]

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
gpu/config/gpu_driver_bug_list_json.cc:1923:
"use_transform_feedback_temp_unbind"
On 2016/08/19 01:34:43, jchen10 wrote:
> On 2016/08/19 00:13:36, Ken Russell wrote:
> > How about "rebind_before_resume_transform_feedback"?
> 
> Or "rebind_transform_feedback_before_resume"?

Sounds fine.

[jchen10, 2016-08-19 01:58:30]

On 2016/08/19 01:39:25, Ken Russell wrote:
>
https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/2252153003/diff/1/gpu/config/gpu_driver_bug_l...
> gpu/config/gpu_driver_bug_list_json.cc:1923:
> "use_transform_feedback_temp_unbind"
> On 2016/08/19 01:34:43, jchen10 wrote:
> > On 2016/08/19 00:13:36, Ken Russell wrote:
> > > How about "rebind_before_resume_transform_feedback"?
> > 
> > Or "rebind_transform_feedback_before_resume"?
> 
> Sounds fine.

Fixes uploaded! Please take a look, Thanks a lot!

[Ken Russell (switch to Gerrit), 2016-08-19 02:05:09]

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

[Ken Russell (switch to Gerrit), 2016-08-19 02:05:11]

Awesome. LGTM, but I'm not an owner.

[jchen10, 2016-08-19 02:06:40]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 02:07:06]

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-19 02:19:25]

lgtm

[commit-bot: I haz the power, 2016-08-19 05:24:35]

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

[commit-bot: I haz the power, 2016-08-19 05:24:36]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[jchen10, 2016-08-19 06:11:32]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 06:11:44]

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-08-19 08:14:21]

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

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

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[jchen10, 2016-08-19 08:19:56]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 08:20:16]

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-08-19 09:06:13]

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

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

Dry run: This issue passed the CQ dry run.

[jchen10, 2016-08-22 02:16:21]

The CQ bit was checked by jie.a.chen@intel.com

[commit-bot: I haz the power, 2016-08-22 02:16:36]

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-08-22 02:18:05]

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

[commit-bot: I haz the power, 2016-08-22 02:18:06]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jchen10, 2016-08-22 02:22:23]

The CQ bit was checked by jie.a.chen@intel.com

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

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-08-22 02:24:08]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jchen10, 2016-08-22 02:55:30]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 02:55:45]

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-08-22 03:25:33]

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

[commit-bot: I haz the power, 2016-08-22 03:25:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhenyao Mo, 2016-08-22 03:27:14]

https://codereview.chromium.org/2252153003/diff/40001/gpu/config/gpu_driver_b...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/2252153003/diff/40001/gpu/config/gpu_driver_b...
gpu/config/gpu_driver_bug_list_json.cc:1930: }
Here you miss ",".

[jchen10, 2016-08-22 03:31:29]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 03:31:37]

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-08-22 05:48:10]

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

[commit-bot: I haz the power, 2016-08-22 05:48:11]

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_...)

[jchen10, 2016-08-22 05:51:34]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 05:51:49]

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-08-22 06:39:36]

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

[commit-bot: I haz the power, 2016-08-22 06:39:37]

Dry run: This issue passed the CQ dry run.

[jchen10, 2016-08-22 06:40:45]

The CQ bit was checked by jie.a.chen@intel.com

[jchen10, 2016-08-22 06:40:46]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2252153003/#ps60001
(title: "fix the missing comma")

[commit-bot: I haz the power, 2016-08-22 06:41:11]

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-08-22 06:45:12]

Description was changed from

==========
Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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

==========
Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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-08-22 06:45:13]

Committed patchset #4 (id:60001)

[jchen10, 2016-08-22 06:46:34]

On 2016/08/22 03:27:14, Zhenyao Mo wrote:
>
https://codereview.chromium.org/2252153003/diff/40001/gpu/config/gpu_driver_b...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/2252153003/diff/40001/gpu/config/gpu_driver_b...
> gpu/config/gpu_driver_bug_list_json.cc:1930: }
> Here you miss ",".

Thanks for helping!

[commit-bot: I haz the power, 2016-08-22 06:47:17]

Description was changed from

==========
Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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

==========
Add a transform feedback workaround for Intel GPUs on MacOSX

Temporarily unbind current transform feedback object to make
glResumeTransformFeedback actually work correctly.

BUG=638514
TEST=webgl2_conformance_test/deqp/functional/gles3/transformfeedback on mac
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/0494f634cad9c20494a458e79120f10cca315582
Cr-Commit-Position: refs/heads/master@{#413412}
==========

[commit-bot: I haz the power, 2016-08-22 06:47:18]

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