GLContextGLX: try all GL versions from the highest.

GLContextGLX: try all GL versions from the highest.

Also try all ES versions from the highest. This change will allow
Chrome to create a GL context higher than 3.0 on Mesa and will in turn
allow WebGL2 to run on Intel Linux when using the native GL driver.

This patch is a direct port of the logic in ANGLE's DisplayGLX

BUG=598902
Committed: https://crrev.com/3464004765597c914d8499ee9792cf81e106a4ae
Cr-Commit-Position: refs/heads/master@{#396243}

[Corentin Wallez, 2016-05-20 19:18:26]

Description was changed from

==========
GLContextGLX: try all GL versions from the highest.

Also try all ES versions from the highest. This change will allow
Chrome to create a GL context higher than 3.0 on Mesa and will in turn
allow WebGL2 to run on Linux when using the native GL driver.

This patch is a direct port of the logic in ANGLE's DisplayGLX

BUG=598902
==========

to

==========
GLContextGLX: try all GL versions from the highest.

Also try all ES versions from the highest. This change will allow
Chrome to create a GL context higher than 3.0 on Mesa and will in turn
allow WebGL2 to run on Intel Linux when using the native GL driver.

This patch is a direct port of the logic in ANGLE's DisplayGLX

BUG=598902
==========

[Zhenyao Mo, 2016-05-20 19:20:12]

zmo@chromium.org changed reviewers:
+ qiankun.miao@intel.com, zmo@chromium.org

[Zhenyao Mo, 2016-05-20 19:20:57]

qiankun: can you help testing this CL on Intel GPU with mesa driver?

[qiankun, 2016-05-23 09:02:05]

On 2016/05/20 19:20:57, Zhenyao Mo wrote:
> qiankun: can you help testing this CL on Intel GPU with mesa driver?

I verified this CL on Linux desktop with mesa driver. With this CL, chrome
creates a 3.3 GL context. But this context is not sufficient to run WebGL 2 for
Linux (Maybe it's sufficient for Mac). Please refer the following info in
about://gpu with and without this CL:
without:
GL_VENDOR       Intel Open Source Technology Center
GL_RENDERER     Mesa DRI Intel(R) Skylake DT GT2
GL_VERSION      3.0 Mesa 11.0.2

with:
GL_VENDOR       Intel Open Source Technology Center
GL_RENDERER     Mesa DRI Intel(R) Skylake DT GT2
GL_VERSION      3.3 (Core Profile) Mesa 11.0.2

[Zhenyao Mo, 2016-05-23 18:24:14]

On 2016/05/23 09:02:05, qiankun wrote:
> On 2016/05/20 19:20:57, Zhenyao Mo wrote:
> > qiankun: can you help testing this CL on Intel GPU with mesa driver?
> 
> I verified this CL on Linux desktop with mesa driver. With this CL, chrome
> creates a 3.3 GL context. But this context is not sufficient to run WebGL 2
for
> Linux (Maybe it's sufficient for Mac). Please refer the following info in
> about://gpu with and without this CL:
> without:
> GL_VENDOR       Intel Open Source Technology Center
> GL_RENDERER     Mesa DRI Intel(R) Skylake DT GT2
> GL_VERSION      3.0 Mesa 11.0.2
> 
> with:
> GL_VENDOR       Intel Open Source Technology Center
> GL_RENDERER     Mesa DRI Intel(R) Skylake DT GT2
> GL_VERSION      3.3 (Core Profile) Mesa 11.0.2

Thanks @qiankun.

[Zhenyao Mo, 2016-05-23 18:24:44]

The CL lgtm, but I am not an owner.

[Corentin Wallez, 2016-05-24 14:55:20]

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

[Corentin Wallez, 2016-05-24 14:58:32]

On 2016/05/23 at 18:24:44, zmo wrote:
> The CL lgtm, but I am not an owner.

+(Ken, Antoine): PTAL and note that this will create a core profile context for
any version >= 3.2 whereas previously a compatibility profile context would have
been created.

@qiankun: thanks for the verification, getting a 3.3 context was the expected
behavior, and we should be able to massage Chrome into allowing WebGL2 on a 3.3
core profile (ANGLE is able to run the WebGL2 CTS fine on the same driver)

[piman, 2016-05-24 15:22:15]

lgtm overall. Do you know if it affects context creation time significantly?
E.g. if it has to do a round-trip to the server to check the version. It may be
worth caching the result and/or only doing this on mesa (as a workaround)

[Corentin Wallez, 2016-05-24 15:31:24]

On 2016/05/24 at 15:22:15, piman wrote:
> lgtm overall. Do you know if it affects context creation time significantly?
E.g. if it has to do a round-trip to the server to check the version. It may be
worth caching the result and/or only doing this on mesa (as a workaround)

Thanks for the review. I did not measure the context creation time but assuming
the GL server is local (i.e. not ssh -X) I don't see how it could cause a
significant slowdown as the check for the context version will be very early (I
assume NVIDIA's driver won't have gone multithreaded at that point yet). Do we
have a perftest for this?

[piman, 2016-05-24 16:00:02]

On 2016/05/24 15:31:24, Corentin Wallez wrote:
> On 2016/05/24 at 15:22:15, piman wrote:
> > lgtm overall. Do you know if it affects context creation time significantly?
> E.g. if it has to do a round-trip to the server to check the version. It may
be
> worth caching the result and/or only doing this on mesa (as a workaround)
> 
> Thanks for the review. I did not measure the context creation time but
assuming
> the GL server is local (i.e. not ssh -X) I don't see how it could cause a
> significant slowdown as the check for the context version will be very early
(I
> assume NVIDIA's driver won't have gone multithreaded at that point yet). Do we
> have a perftest for this?

I don't think we do. Where it would show up typically I think is in the startup
time UMA.

[Corentin Wallez, 2016-05-24 17:12:11]

On 2016/05/24 at 16:00:02, piman wrote:
> On 2016/05/24 15:31:24, Corentin Wallez wrote:
> > On 2016/05/24 at 15:22:15, piman wrote:
> > > lgtm overall. Do you know if it affects context creation time
significantly?
> > E.g. if it has to do a round-trip to the server to check the version. It may
be
> > worth caching the result and/or only doing this on mesa (as a workaround)
> > 
> > Thanks for the review. I did not measure the context creation time but
assuming
> > the GL server is local (i.e. not ssh -X) I don't see how it could cause a
> > significant slowdown as the check for the context version will be very early
(I
> > assume NVIDIA's driver won't have gone multithreaded at that point yet). Do
we
> > have a perftest for this?
> 
> I don't think we do. Where it would show up typically I think is in the
startup time UMA.

Local benchmarking shows that the NVIDIA driver takes 2.5ms per failed context
creation and Mesa less than 0.1ms. The additional latency is probably not
acceptable on NVIDIA so I'll make the workaround Mesa-specific.
 
I'll do the respective ANGLE change before modifying this CL.

[piman, 2016-05-24 17:16:18]

On 2016/05/24 17:12:11, Corentin Wallez wrote:
> On 2016/05/24 at 16:00:02, piman wrote:
> > On 2016/05/24 15:31:24, Corentin Wallez wrote:
> > > On 2016/05/24 at 15:22:15, piman wrote:
> > > > lgtm overall. Do you know if it affects context creation time
> significantly?
> > > E.g. if it has to do a round-trip to the server to check the version. It
may
> be
> > > worth caching the result and/or only doing this on mesa (as a workaround)
> > > 
> > > Thanks for the review. I did not measure the context creation time but
> assuming
> > > the GL server is local (i.e. not ssh -X) I don't see how it could cause a
> > > significant slowdown as the check for the context version will be very
early
> (I
> > > assume NVIDIA's driver won't have gone multithreaded at that point yet).
Do
> we
> > > have a perftest for this?
> > 
> > I don't think we do. Where it would show up typically I think is in the
> startup time UMA.
> 
> Local benchmarking shows that the NVIDIA driver takes 2.5ms per failed context
> creation and Mesa less than 0.1ms. The additional latency is probably not
> acceptable on NVIDIA so I'll make the workaround Mesa-specific.
>  
> I'll do the respective ANGLE change before modifying this CL.

Sounds good, thanks!

[Ken Russell (switch to Gerrit), 2016-05-24 17:43:54]

Thanks for doing this port. It LGTM overall. Is it needed on more GPU types than
just Intel/Mesa? I thought AMD's fglrx driver also had similar behavior to this.

[Corentin Wallez, 2016-05-24 17:59:24]

On 2016/05/24 at 17:43:54, kbr wrote:
> Thanks for doing this port. It LGTM overall. Is it needed on more GPU types
than just Intel/Mesa? I thought AMD's fglrx driver also had similar behavior to
this.

I don't remember it being needed for AMD, I'll check with the respective ANGLE
patch. Mesa drivers other than Intel's will also need the workaround (even
though it is a smaller population)

[Corentin Wallez, 2016-05-25 22:01:31]

On 2016/05/24 at 17:59:24, Corentin Wallez wrote:
> On 2016/05/24 at 17:43:54, kbr wrote:
> > Thanks for doing this port. It LGTM overall. Is it needed on more GPU types
than just Intel/Mesa? I thought AMD's fglrx driver also had similar behavior to
this.
> 
> I don't remember it being needed for AMD, I'll check with the respective ANGLE
patch. Mesa drivers other than Intel's will also need the workaround (even
though it is a smaller population)

PTAL again, addressed the performance concern. The workaround isn't needed on
AMD (webgl_conformance_angle_tests on Linux ATI Release correctly uses a 4.4
context after the respective ANGLE patch landed).

[piman, 2016-05-25 22:35:17]

LGTM+nits, thanks!

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc
File ui/gl/gl_context_glx.cc (right):

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:90: const ContextCreationInfo contextsToTry[] = {
nit: chrome style
contexts_to_try

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:98: const ContextCreationInfo mesaContextsToTry[] = {
nit: mesa_contexts_to_try

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:123: std::string clientVendor =
glXGetClientString(display, GLX_VENDOR);
nit: client_vendor

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:124: bool isMesa = clientVendor.find("Mesa") !=
std::string::npos;
nit: is_mesa

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:126: const ContextCreationInfo* toTry = contextsToTry;
nit: to_try

https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
ui/gl/gl_context_glx.cc:127: size_t toTryLength = arraysize(contextsToTry);
nit: to_try_length

[Corentin Wallez, 2016-05-25 22:41:18]

On 2016/05/25 at 22:35:17, piman wrote:
> LGTM+nits, thanks!
> 
> https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc
> File ui/gl/gl_context_glx.cc (right):
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:90: const ContextCreationInfo contextsToTry[] = {
> nit: chrome style
> contexts_to_try
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:98: const ContextCreationInfo mesaContextsToTry[] = {
> nit: mesa_contexts_to_try
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:123: std::string clientVendor =
glXGetClientString(display, GLX_VENDOR);
> nit: client_vendor
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:124: bool isMesa = clientVendor.find("Mesa") !=
std::string::npos;
> nit: is_mesa
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:126: const ContextCreationInfo* toTry = contextsToTry;
> nit: to_try
> 
>
https://codereview.chromium.org/1999543003/diff/20001/ui/gl/gl_context_glx.cc...
> ui/gl/gl_context_glx.cc:127: size_t toTryLength = arraysize(contextsToTry);
> nit: to_try_length

Done

Thanks for the reviews!

[Corentin Wallez, 2016-05-25 22:41:19]

The CQ bit was checked by cwallez@chromium.org

[Corentin Wallez, 2016-05-25 22:41:20]

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

[commit-bot: I haz the power, 2016-05-25 22:42:26]

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

[commit-bot: I haz the power, 2016-05-26 00:14:59]

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

[commit-bot: I haz the power, 2016-05-26 00:15:00]

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

[Corentin Wallez, 2016-05-26 17:24:01]

On 2016/05/26 at 00:15:00, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

CQing again after fixing the version (0, 0) condition (trivial fix).

[Corentin Wallez, 2016-05-26 17:24:03]

The CQ bit was checked by cwallez@chromium.org

[Corentin Wallez, 2016-05-26 17:24:04]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
zmo@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1999543003/#ps60001
(title: "Fix silly mistake")

[commit-bot: I haz the power, 2016-05-26 17:24:29]

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

[commit-bot: I haz the power, 2016-05-26 18:48:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-26 18:50:50]

Description was changed from

==========
GLContextGLX: try all GL versions from the highest.

Also try all ES versions from the highest. This change will allow
Chrome to create a GL context higher than 3.0 on Mesa and will in turn
allow WebGL2 to run on Intel Linux when using the native GL driver.

This patch is a direct port of the logic in ANGLE's DisplayGLX

BUG=598902
==========

to

==========
GLContextGLX: try all GL versions from the highest.

Also try all ES versions from the highest. This change will allow
Chrome to create a GL context higher than 3.0 on Mesa and will in turn
allow WebGL2 to run on Intel Linux when using the native GL driver.

This patch is a direct port of the logic in ANGLE's DisplayGLX

BUG=598902

Committed: https://crrev.com/3464004765597c914d8499ee9792cf81e106a4ae
Cr-Commit-Position: refs/heads/master@{#396243}
==========

[commit-bot: I haz the power, 2016-05-26 18:50:51]

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

[Zhenyao Mo, 2016-05-26 22:12:27]

This triggered a bunch of failures on Linux AMD and Linux Intel.

[Corentin Wallez, 2016-05-26 22:17:50]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2012413002/ by cwallez@chromium.org.

The reason for reverting is: Causes a lot of failures on the GPU FYI waterfall..