GpuControlListEntry might need more info for gl_version as well

GpuControlListEntry might need more info for gl_version as well

BUG=651576, 634519
R=kbr@chromium.org, zmo@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/b53abd7b3c59d221641d07fd25ab26d2ca2f8a22
Cr-Commit-Position: refs/heads/master@{#422393}

[Julien Isorce Samsung, 2016-09-29 23:25:45]

Description was changed from

==========
GpuControlListEntry might need more info for gl_version as well

BUG=651576, 634519

R=kbr@chromium.org, zmo@chromium.org
==========

to

==========
GpuControlListEntry might need more info for gl_version as well

BUG=651576, 634519

R=kbr@chromium.org, zmo@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
==========

[Julien Isorce Samsung, 2016-09-29 23:26:27]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 23:26:50]

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

[Julien Isorce Samsung, 2016-09-29 23:41:22]

On 2016/09/29 23:26:50, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Hi, I have not spend enough time to think on corner cases since it is late here.
But I did run a few tests.
I can have more thought tomorrow if you have some doubts. In the mean time
please have
a first look as this is a small patch. Thx

[Zhenyao Mo, 2016-09-29 23:58:50]

On 2016/09/29 23:41:22, Julien Isorce wrote:
> On 2016/09/29 23:26:50, commit-bot: I haz the power wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Hi, I have not spend enough time to think on corner cases since it is late
here.
> But I did run a few tests.
> I can have more thought tomorrow if you have some doubts. In the mean time
> please have
> a first look as this is a small patch. Thx

lgtm

[commit-bot: I haz the power, 2016-09-30 00:15:18]

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

[commit-bot: I haz the power, 2016-09-30 00:15:19]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-30 00:43:59]

Thanks for putting this together. I'd like to see some testing of this change
before approving it.

[yizhou.jiang, 2016-09-30 03:11:47]

On 2016/09/29 23:41:22, Julien Isorce wrote:
> On 2016/09/29 23:26:50, commit-bot: I haz the power wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Hi, I have not spend enough time to think on corner cases since it is late
here.
> But I did run a few tests.
> I can have more thought tomorrow if you have some doubts. In the mean time
> please have
> a first look as this is a small patch. Thx

@Julien, I have applied this patch on
https://codereview.chromium.org/2318313004/ and run trybot again(see patchset
17).It has passed on win and linux compared to patchset 16, but fails on mac
with the same problem which passed on patchset 16.

[Julien Isorce Samsung, 2016-09-30 10:56:04]

On 2016/09/30 00:43:59, Ken Russell wrote:
> Thanks for putting this together. I'd like to see some testing of this change
> before approving it.

No problem. Patch Set 2 contains a unit test. Prior this CL this new test would
have failed.
The idea behind is that whether the "feature"(i.e. the workaround here) has to
be added as a preventive measure or not.
I think here it should just behave like gl_version_string so not as a preventive
measure.

[Julien Isorce Samsung, 2016-09-30 11:05:44]

On 2016/09/30 03:11:47, yizhou.jiang wrote:
> @Julien, I have applied this patch on
> https://codereview.chromium.org/2318313004/ and run trybot again(see patchset
> 17).It has passed on win and linux compared to patchset 16, 

Good news!

> but fails on mac with the same problem which passed on patchset 16.

I can see why, this is another issue. Prior it was just lucky that it enables
this workaround in the gpu process. (gpu_info.gl_version was empty and
the workaround was added as a preventive measure). But even if "lucky"
it would have been wrong in some cases because it would have added the
workaround
no matter the real gl version.

These later problem has not been spotted before because your new entry in
kGpuDriverBugListJson is the first one that relies on gl_version for macosx.
(all other entries for macosx only rely on os version or vendor/device ids)

I am going to submit a fix for it after that this CL lands. On gpu process
side it just needs to re-compute the workarounds on osx like it is done
on others platforms. (see GpuChildThread::OnCollectGraphicsInfo and
GpuInit's gpu::ApplyGpuDriverBugWorkarounds call)

[Julien Isorce Samsung, 2016-09-30 11:05:52]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 11:06:07]

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-09-30 11:26:23]

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

[commit-bot: I haz the power, 2016-09-30 11:26:24]

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

[Julien Isorce Samsung, 2016-09-30 13:03:43]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:03:57]

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

[Julien Isorce Samsung, 2016-09-30 13:36:30]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:36: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-09-30 14:39:20]

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

[commit-bot: I haz the power, 2016-09-30 14:39:21]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-30 23:23:00]

LGTM based on the new test and the green trybot runs.

[Zhenyao Mo, 2016-09-30 23:29:23]

https://codereview.chromium.org/2379153002/diff/80001/gpu/config/gpu_control_...
File gpu/config/gpu_control_list_entry_unittest.cc (right):

https://codereview.chromium.org/2379153002/diff/80001/gpu/config/gpu_control_...
gpu/config/gpu_control_list_entry_unittest.cc:837:
entry->Contains(GpuControlList::kOsUnknown, std::string(), gpu_info));
Can you try a "OpenGL ES 3.6 Mesa 20.0.0"? Just wanna make sure GL and ES work
as intended.

[Zhenyao Mo, 2016-09-30 23:30:14]

lgtm

https://codereview.chromium.org/2379153002/diff/80001/gpu/config/gpu_control_...
File gpu/config/gpu_control_list_entry_unittest.cc (right):

https://codereview.chromium.org/2379153002/diff/80001/gpu/config/gpu_control_...
gpu/config/gpu_control_list_entry_unittest.cc:837:
entry->Contains(GpuControlList::kOsUnknown, std::string(), gpu_info));
On 2016/09/30 23:29:22, Zhenyao Mo wrote:
> Can you try a "OpenGL ES 3.6 Mesa 20.0.0"? Just wanna make sure GL and ES work
> as intended.

Never mind.  Just realized it/s < 3.5 so this last case already covered it.

[yizhou.jiang, 2016-10-01 04:13:13]

Thanks a lot!

[Julien Isorce Samsung, 2016-10-03 07:43:39]

The CQ bit was checked by j.isorce@samsung.com

[Julien Isorce Samsung, 2016-10-03 07:43:39]

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

[commit-bot: I haz the power, 2016-10-03 07:43:48]

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-03 08:51:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-03 08:54:30]

Description was changed from

==========
GpuControlListEntry might need more info for gl_version as well

BUG=651576, 634519

R=kbr@chromium.org, zmo@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

==========
GpuControlListEntry might need more info for gl_version as well

BUG=651576, 634519

R=kbr@chromium.org, zmo@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/b53abd7b3c59d221641d07fd25ab26d2ca2f8a22
Cr-Commit-Position: refs/heads/master@{#422393}
==========

[commit-bot: I haz the power, 2016-10-03 08:54:31]

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