DCHECK if shader compilation fails that it's due to context loss.

DCHECK if shader compilation fails that it's due to context loss.

Add GetGraphicsResetStatusKHR() to GLES2Interface so GLHelper
code can verify this.

Adds IsGpuChannelLost() to GpuControl to tell if the
GpuChannelHost has lost its connection to the GPU process,
which is implemented in CommandBufferProxyImpl.

Uses this along with the GetLastState() on the CommandBuffer
(via CommandBufferHelper::IsContextLost()) to tell if the
context is lost from GLES2Implementation.

R=piman@chromium.org
TBR=sky
BUG=492447
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b7c7364c8783728bf33dc3edbef59cdce4aefb40
Cr-Commit-Position: refs/heads/master@{#333404}

[jamesr, 2015-05-28 21:35:28]

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

[jamesr, 2015-05-28 21:35:29]

We already have an entry pot to check for it the context is lost
(glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid growing the
contextsupport interface with gl entry points?

[danakj, 2015-05-28 21:37:08]

Yes I can use that then.

On Thu, May 28, 2015 at 2:35 PM, <jamesr@chromium.org> wrote:

> We already have an entry pot to check for it the context is lost
> (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid growing
> the
> contextsupport interface with gl entry points?
>
> https://codereview.chromium.org/1160863007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-28 21:39:15]

I don't see anything with "Reset" in a name in gles2_interface_autogen.h.
Help?

On Thu, May 28, 2015 at 2:36 PM, Dana Jansens <danakj@chromium.org> wrote:

> Yes I can use that then.
>
> On Thu, May 28, 2015 at 2:35 PM, <jamesr@chromium.org> wrote:
>
>> We already have an entry pot to check for it the context is lost
>> (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid
>> growing the
>> contextsupport interface with gl entry points?
>>
>> https://codereview.chromium.org/1160863007/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-28 22:00:27]

On 2015/05/28 21:35:29, jamesr wrote:
> We already have an entry pot to check for it the context is lost
> (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid growing the
> contextsupport interface with gl entry points?

Are you thinking of WebGraphicsContext3D API? WebGraphicsContext3DImpl doesn't
even override/implement that method (anymore?) ?

[danakj, 2015-05-28 22:02:42]

On 2015/05/28 22:00:27, danakj wrote:
> On 2015/05/28 21:35:29, jamesr wrote:
> > We already have an entry pot to check for it the context is lost
> > (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid growing
the
> > contextsupport interface with gl entry points?
> 
> Are you thinking of WebGraphicsContext3D API? WebGraphicsContext3DImpl doesn't
> even override/implement that method (anymore?) ?

OK so.. WebGraphicsContext3DInProcessCommandBufferImpl does implement that
method. But that's not GLES2Interface. I'm not sure where all the dark twisty
tunnels go but I don't think I can get to there from GLES2 stuff, can I?

[piman, 2015-05-28 23:35:36]

On 2015/05/28 22:02:42, danakj wrote:
> On 2015/05/28 22:00:27, danakj wrote:
> > On 2015/05/28 21:35:29, jamesr wrote:
> > > We already have an entry pot to check for it the context is lost
> > > (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid growing
> the
> > > contextsupport interface with gl entry points?
> > 
> > Are you thinking of WebGraphicsContext3D API? WebGraphicsContext3DImpl
doesn't
> > even override/implement that method (anymore?) ?
> 
> OK so.. WebGraphicsContext3DInProcessCommandBufferImpl does implement that
> method. But that's not GLES2Interface. I'm not sure where all the dark twisty
> tunnels go but I don't think I can get to there from GLES2 stuff, can I?

Implementing the equivalent of WebGraphicsContext3D::getGraphicsResetStatusARB
in GLES2Implementation is possible, but a bit of work, because among others we
need to add a GpuControl::IsLost to accurately represent
IsCommandBufferContextLost(). It'd be a nice cleanup though - together with
isContextLost, they're the last WebGraphicsContext3D method not implemented by
WebGraphicsContext3DImpl but by its derived classes instead.

It's not strictly needed for this though.

[Ken Russell (switch to Gerrit), 2015-05-29 17:47:04]

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

[Ken Russell (switch to Gerrit), 2015-05-29 17:47:05]

Thanks for doing this cleanup. LGTM from the standpoint of getting the DCHECK
back in.

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:882: // Unfortunately, the only
way to reliably detect context loss from
The sentence starting with "Unfortunately" should probably be removed now.

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:889:
DCHECK(context_support->IsContextLost());
To be closer to the previous code, should this be phrased as
DCHECK(Initialized() || context_support->IsContextLost())?

https://codereview.chromium.org/1160863007/diff/40001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1160863007/diff/40001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5375: return
helper_->IsContextLost();
Per piman's comment --
WebGraphicsContext3DCommandBufferImpl::IsCommandBufferContextLost checks the GPU
channel's state too. I wonder whether the propagation of context lost state from
the channel to all of the command buffers
(GpuCommandBufferStub::CheckContextLost and related code) will be enough to get
this bit set.

[danakj, 2015-05-29 18:30:15]

I'm way over my head here, but I'm not seeing why this belongs on GLES2Impl.
That's for methods that are implemented in the decoder on the gpu process.
IsLost is not going to do that, is it? It's just looking at state on the client
side.

Or maybe there's an example of sometihng else like this in the GLES2Impl stuff?

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:882: // Unfortunately, the only
way to reliably detect context loss from
On 2015/05/29 17:47:05, Ken Russell wrote:
> The sentence starting with "Unfortunately" should probably be removed now.

Done.

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:889:
DCHECK(context_support->IsContextLost());
On 2015/05/29 17:47:05, Ken Russell wrote:
> To be closer to the previous code, should this be phrased as
> DCHECK(Initialized() || context_support->IsContextLost())?

Sure ya.

[Ken Russell (switch to Gerrit), 2015-05-29 19:13:09]

This seems fine to me in its current form. We can refine it later.

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:882: // Unfortunately, the only
way to reliably detect context loss from
On 2015/05/29 18:30:14, danakj wrote:
> On 2015/05/29 17:47:05, Ken Russell wrote:
> > The sentence starting with "Unfortunately" should probably be removed now.
> 
> Done.

Was the current patch set updated to include this change?

[danakj, 2015-05-29 19:53:04]

Oops, I thought it did. Now it does.

[Ken Russell (switch to Gerrit), 2015-05-29 22:42:44]

Still LGTM

https://codereview.chromium.org/1160863007/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/1160863007/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_scaling.cc:882:
DCHECK_IMPLIES(!Initialized(), context_support->IsContextLost());
Nice.

[danakj, 2015-05-29 23:03:45]

WDYT piman@ looks like the gpu/ bits need an owners approval still.

[piman, 2015-06-01 21:09:07]

On Fri, May 29, 2015 at 10:47 AM, <kbr@chromium.org> wrote:

> Thanks for doing this cleanup. LGTM from the standpoint of getting the
> DCHECK
> back in.
>
>
>
>
>
https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
> File content/common/gpu/client/gl_helper_scaling.cc (right):
>
>
>
https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
> content/common/gpu/client/gl_helper_scaling.cc:882: // Unfortunately,
> the only way to reliably detect context loss from
> The sentence starting with "Unfortunately" should probably be removed
> now.
>
>
>
https://codereview.chromium.org/1160863007/diff/40001/content/common/gpu/clie...
> content/common/gpu/client/gl_helper_scaling.cc:889:
> DCHECK(context_support->IsContextLost());
> To be closer to the previous code, should this be phrased as
> DCHECK(Initialized() || context_support->IsContextLost())?
>
>
>
https://codereview.chromium.org/1160863007/diff/40001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
>
>
https://codereview.chromium.org/1160863007/diff/40001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:5375: return
> helper_->IsContextLost();
> Per piman's comment --
> WebGraphicsContext3DCommandBufferImpl::IsCommandBufferContextLost checks
> the GPU channel's state too. I wonder whether the propagation of context
> lost state from the channel to all of the command buffers
> (GpuCommandBufferStub::CheckContextLost and related code) will be enough
> to get this bit set.
>

It's not strictly needed in this case (GLHelperScaling), because the
initialization failure would have been caused by a round trip, at which
point the channel being lost guarantees the client state reflects it.


> https://codereview.chromium.org/1160863007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-06-01 21:12:16]

LGTM, but can you add a TODO/bug that ContextSupport::IsContextLost only
reflects that the current context is known to be lost due to a call on this
context, but may not reflect the fact that it may be lost due to other contexts
in the same group being lost (or the channel being lost).

[danakj, 2015-06-08 19:28:08]

On 2015/05/28 23:35:36, piman (Very slow to review) wrote:
> On 2015/05/28 22:02:42, danakj wrote:
> > On 2015/05/28 22:00:27, danakj wrote:
> > > On 2015/05/28 21:35:29, jamesr wrote:
> > > > We already have an entry pot to check for it the context is lost
> > > > (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid
growing
> > the
> > > > contextsupport interface with gl entry points?
> > > 
> > > Are you thinking of WebGraphicsContext3D API? WebGraphicsContext3DImpl
> doesn't
> > > even override/implement that method (anymore?) ?
> > 
> > OK so.. WebGraphicsContext3DInProcessCommandBufferImpl does implement that
> > method. But that's not GLES2Interface. I'm not sure where all the dark
twisty
> > tunnels go but I don't think I can get to there from GLES2 stuff, can I?
> 
> Implementing the equivalent of WebGraphicsContext3D::getGraphicsResetStatusARB
> in GLES2Implementation is possible, but a bit of work, because among others we
> need to add a GpuControl::IsLost to accurately represent
> IsCommandBufferContextLost(). It'd be a nice cleanup though - together with

I'm not sure why here. There is a CommandBufferHelper::IsContextLost() which
checks for errors. Do we need to ask GpuControl too? Do we need this to go to
the service side if there are no client side errors and handle this in the
decoder?

> isContextLost, they're the last WebGraphicsContext3D method not implemented by
> WebGraphicsContext3DImpl but by its derived classes instead.
> 
> It's not strictly needed for this though.

[danakj, 2015-06-08 19:31:48]

This patch adds a method to GLES2Interface that calls the helper. But it doesn't
add GpuControl and doesn't go to the decoder service side. Is this better than
the ContextSupport patch? Not sure.

[danakj, 2015-06-08 20:48:56]

Patchset #6 (id:100001) has been deleted

[danakj, 2015-06-08 20:49:05]

Patchset #6 (id:120001) has been deleted

[danakj, 2015-06-08 20:57:11]

https://codereview.chromium.org/1160863007/diff/140001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_interface.h (right):

https://codereview.chromium.org/1160863007/diff/140001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_interface.h:30: virtual GLboolean
IsContextLost() = 0;
I am pretty sure putting this method here is wrong. In fact mojo code is
including mojo_gles2_impl_autogen.h directly, so this just doesn't work unless I
add indirection like we use elsewhere.

But I didn't see how to add this via the autogen scripts without adding it to
the cmd_buffer_functions.txt file, and we don't need a glIsContextLost()
function, right?

[piman, 2015-06-08 20:58:05]

On Mon, Jun 8, 2015 at 12:28 PM, <danakj@chromium.org> wrote:

> On 2015/05/28 23:35:36, piman (Very slow to review) wrote:
>
>> On 2015/05/28 22:02:42, danakj wrote:
>> > On 2015/05/28 22:00:27, danakj wrote:
>> > > On 2015/05/28 21:35:29, jamesr wrote:
>> > > > We already have an entry pot to check for it the context is lost
>> > > > (glQueryContextResetWhateverARB) on GLES2Interface. Could we avoid
>>
> growing
>
>> > the
>> > > > contextsupport interface with gl entry points?
>> > >
>> > > Are you thinking of WebGraphicsContext3D API? WebGraphicsContext3DImpl
>> doesn't
>> > > even override/implement that method (anymore?) ?
>> >
>> > OK so.. WebGraphicsContext3DInProcessCommandBufferImpl does implement
>> that
>> > method. But that's not GLES2Interface. I'm not sure where all the dark
>>
> twisty
>
>> > tunnels go but I don't think I can get to there from GLES2 stuff, can I?
>>
>
>  Implementing the equivalent of
>> WebGraphicsContext3D::getGraphicsResetStatusARB
>> in GLES2Implementation is possible, but a bit of work, because among
>> others we
>> need to add a GpuControl::IsLost to accurately represent
>> IsCommandBufferContextLost(). It'd be a nice cleanup though - together
>> with
>>
>
> I'm not sure why here. There is a CommandBufferHelper::IsContextLost()
> which
> checks for errors. Do we need to ask GpuControl too? Do we need this to go
> to
> the service side if there are no client side errors and handle this in the
> decoder?


I think places that call CommandBufferHelper::IsContextLost are after a
round trip so they don't need the CommandBufferProxyImpl poll.


>
>  isContextLost, they're the last WebGraphicsContext3D method not
>> implemented by
>> WebGraphicsContext3DImpl but by its derived classes instead.
>>
>
>  It's not strictly needed for this though.
>>
>
>
> https://codereview.chromium.org/1160863007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-06-08 21:01:58]

https://codereview.chromium.org/1160863007/diff/140001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_interface.h (right):

https://codereview.chromium.org/1160863007/diff/140001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_interface.h:30: virtual GLboolean
IsContextLost() = 0;
On 2015/06/08 20:57:10, danakj wrote:
> I am pretty sure putting this method here is wrong. In fact mojo code is
> including mojo_gles2_impl_autogen.h directly, so this just doesn't work unless
I
> add indirection like we use elsewhere.
> 
> But I didn't see how to add this via the autogen scripts without adding it to
> the cmd_buffer_functions.txt file, and we don't need a glIsContextLost()
> function, right?

I suppose what we'd want to add to GLES2Interface is a
glGetGraphicsResetStatusKHR from
https://www.opengl.org/registry/specs/KHR/robustness.txt


It can be purely implemented without going to the GPU process, so that's why
adding it to cmd_buffer_functions.txt feels like overkill (we don't actually
need that command in the command buffer), but if not doing so requires massive
amounts of hackery, then maybe we can just add it and noop the decoder side.

[danakj, 2015-06-08 22:02:11]

PTAL

[danakj, 2015-06-08 22:02:52]

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:2705: 'gen_cmd': False,
This is the one weird trick to keep the function out of the command
stream/decoder.

[piman, 2015-06-08 22:21:16]

LGTM but for one thing.

https://codereview.chromium.org/1160863007/diff/160001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1160863007/diff/160001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:450: return
channel_->IsLost();
!channel_ || channel_->IsLost()

To my dismay we reset channel_ in some places (when lost).

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:311: GL_APICALL GLenum	GL_APIENTRY
glGetGraphicsResetStatusKHR (void);
nit: do you actually need this when gen_cmd == false?

[danakj, 2015-06-08 22:22:37]

https://codereview.chromium.org/1160863007/diff/160001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1160863007/diff/160001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:450: return
channel_->IsLost();
On 2015/06/08 22:21:16, piman (Very slow to review) wrote:
> !channel_ || channel_->IsLost()
> 
> To my dismay we reset channel_ in some places (when lost).

Done.

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1160863007/diff/160001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:311: GL_APICALL GLenum	GL_APIENTRY
glGetGraphicsResetStatusKHR (void);
On 2015/06/08 22:21:16, piman (Very slow to review) wrote:
> nit: do you actually need this when gen_cmd == false?

Ya without this the build script does nothing, it all comes from here, and the
python just changes it.

[piman, 2015-06-08 22:32:24]

lgtm

[danakj, 2015-06-08 22:42:54]

Patchset #10 (id:220001) has been deleted

[danakj, 2015-06-08 22:43:02]

Patchset #9 (id:200001) has been deleted

[danakj, 2015-06-08 22:43:28]

danakj@chromium.org changed reviewers:
+ sky@chromium.org

[danakj, 2015-06-08 22:43:29]

+sky for mojo/

[Ken Russell (switch to Gerrit), 2015-06-08 22:59:09]

LGTM again. Thanks for cleaning this up.

[Ken Russell (switch to Gerrit), 2015-06-08 23:08:22]

One small comment.

https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:4115: // the actual reason
here if we cared to.
It's a little unfortunate that this can't return the GUILTY status any more, but
realistically none of the drivers out there were reporting guilty/innocent
contexts with any precision.

[danakj, 2015-06-08 23:09:37]

https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:4115: // the actual reason
here if we cared to.
On 2015/06/08 23:08:22, Ken Russell wrote:
> It's a little unfortunate that this can't return the GUILTY status any more,
but
> realistically none of the drivers out there were reporting guilty/innocent
> contexts with any precision.

Ya, that's why I left a todo so someone can track it down and add it if they
care to. For now I think it'd just be extra plumbing/LOC without benefit?

[Ken Russell (switch to Gerrit), 2015-06-08 23:22:11]

On 2015/06/08 23:09:37, danakj wrote:
>
https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/1160863007/diff/260001/gpu/command_buffer/cli...
> gpu/command_buffer/client/gles2_implementation.cc:4115: // the actual reason
> here if we cared to.
> On 2015/06/08 23:08:22, Ken Russell wrote:
> > It's a little unfortunate that this can't return the GUILTY status any more,
> but
> > realistically none of the drivers out there were reporting guilty/innocent
> > contexts with any precision.
> 
> Ya, that's why I left a todo so someone can track it down and add it if they
> care to. For now I think it'd just be extra plumbing/LOC without benefit?

Agree, it's fine the way it is.

[danakj, 2015-06-08 23:40:19]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-06-08 23:40:21]

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

[commit-bot: I haz the power, 2015-06-08 23:40:42]

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

[commit-bot: I haz the power, 2015-06-08 23:49:39]

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

[commit-bot: I haz the power, 2015-06-08 23:49:40]

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

[danakj, 2015-06-09 00:33:57]

TBR=sky

[danakj, 2015-06-09 00:34:14]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-06-09 00:34:30]

On 2015/06/09 00:33:57, danakj wrote:
> TBR=sky

(adding a mojo method stub to a common interface)

[commit-bot: I haz the power, 2015-06-09 00:37:22]

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

[commit-bot: I haz the power, 2015-06-09 00:45:40]

Committed patchset #11 (id:270001)

[commit-bot: I haz the power, 2015-06-09 00:47:25]

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