Move Insert/SignalSyncPoint() to GpuControl

Move Insert/SignalSyncPoint() to GpuControl

This moves InsertSyncPoint() and SignalSyncPoint() to the gpu::GpuControl
interface to get a step closer to removing the WebGraphicsContext3D interface.

R=piman,sievers

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226412

[jamesr, 2013-09-27 01:10:44]

This is a patch on top of https://codereview.chromium.org/24925002.  The idea is
to then expose this via gpu::ContextSupport and depend on that from places like
cc that need to use sync points but don't want to rely on WebGraphicsContext3D
for everything.

I don't fully understand WaitSyncPoint() yet or the *Query functions, but will
try to do something similar with them next if this is the right direction.

[no sievers, 2013-09-27 01:24:18]

On 2013/09/27 01:10:44, jamesr wrote:
> This is a patch on top of https://codereview.chromium.org/24925002.  The idea
is
> to then expose this via gpu::ContextSupport and depend on that from places
like
> cc that need to use sync points but don't want to rely on WebGraphicsContext3D
> for everything.
> 

Can't we leave InsertSyncPoint and WaitSyncPoint be exposed form
GLES2Implementation?

I think the only ones that don't have a good place yet are the ones with the
callbacks: SignalSyncPoint() and SignalQuery().
Not sure if they could also maybe just be exposed in GLES2Implementation (and
could then be implemented in GpuControl or CommandBuffer).

wdyt?


> I don't fully understand WaitSyncPoint() yet or the *Query functions, but will
> try to do something similar with them next if this is the right direction.

[piman, 2013-09-27 01:35:47]

LGTM overall, though I think we should fix the wgc3d implementations as we can
with these patches - that way we kinda know when we're done :)

About WaitSyncPoint, you definitely want to leave it in the command buffers,
which makes it virtually free.

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/service/in...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/service/in...
gpu/command_buffer/service/in_process_command_buffer.cc:669: NOTREACHED();
If you want to merge the WGC3D implementations and have them delegate to the
GLES2Implementation, this would be reached (though it can still return 0).

It'd be nice to do with this patch, I think, just fixing
WebGraphicsContext3DInProcessCommandBufferImpl::insertSyncPoint to
DELEGATE_TO_GL instead.

[piman, 2013-09-27 01:38:53]

On 2013/09/27 01:24:18, sievers wrote:
> On 2013/09/27 01:10:44, jamesr wrote:
> > This is a patch on top of https://codereview.chromium.org/24925002.  The
idea
> is
> > to then expose this via gpu::ContextSupport and depend on that from places
> like
> > cc that need to use sync points but don't want to rely on
WebGraphicsContext3D
> > for everything.
> > 
> 
> Can't we leave InsertSyncPoint and WaitSyncPoint be exposed form
> GLES2Implementation?

Well, I think the plan we settled on is to have ContextSupport be implemented by
GLES2Implementation anyway. Then we can decide if it should be in GLES2Interface
or ContextSupport.

> 
> I think the only ones that don't have a good place yet are the ones with the
> callbacks: SignalSyncPoint() and SignalQuery().
> Not sure if they could also maybe just be exposed in GLES2Implementation (and
> could then be implemented in GpuControl or CommandBuffer).

If InsertSyncPoint moves to GpuControl, then it makes complete sense for
SignalSyncPoint to go there too. SignalQuery should definitely go to GpuControl.

> 
> wdyt?
> 
> 
> > I don't fully understand WaitSyncPoint() yet or the *Query functions, but
will
> > try to do something similar with them next if this is the right direction.

[jamesr, 2013-09-27 04:36:47]

On 2013/09/27 01:38:53, piman wrote:
> On 2013/09/27 01:24:18, sievers wrote:
> > On 2013/09/27 01:10:44, jamesr wrote:
> > > This is a patch on top of https://codereview.chromium.org/24925002.  The
> idea
> > is
> > > to then expose this via gpu::ContextSupport and depend on that from places
> > like
> > > cc that need to use sync points but don't want to rely on
> WebGraphicsContext3D
> > > for everything.
> > > 
> > 
> > Can't we leave InsertSyncPoint and WaitSyncPoint be exposed form
> > GLES2Implementation?
> 
> Well, I think the plan we settled on is to have ContextSupport be implemented
by
> GLES2Implementation anyway. Then we can decide if it should be in
GLES2Interface
> or ContextSupport.

Exactly.  I'll definitely leave the implementation in GLES2Implementation.

To me, SignalSyncPoint() probably belongs on ContextSupport since it depends on
callbacks and I think GLES2Interface conceptually should stay closer to OpenGL
which doesn't have any notion of callbacks or event loops or such.  If you
accept that reasoning, Insert|Wait SyncPoint could probably go on GLES2Interface
or ContextSupport but having all 3 related functions be together seems
reasonable to me.

[danakj1, 2013-09-27 15:05:28]

On Thu, Sep 26, 2013 at 9:10 PM, <jamesr@chromium.org> wrote:

> Reviewers: piman, sievers,
>
> Message:
> This is a patch on top of
https://codereview.chromium.**org/24925002<https://codereview.chromium.org/24....
>  The idea is
> to then expose this via gpu::ContextSupport and depend on that from places
> like
> cc that need to use sync points but don't want to rely on
> WebGraphicsContext3D
> for everything.
>

When we move the SignalQuery/SyncPoint stuff into ContextSupport, do you
think we could get rid of cc/resources/sync_point_helper at the same time?

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

[no sievers, 2013-09-27 18:13:04]

LGTM, but don't you have to update
WebGraphicsContext3DCommandBufferImpl::signalSyncPoint() which calls
CommandBufferProxyImpl::SignalSyncPoint() directly?

https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
content/common/gpu/client/command_buffer_proxy_impl.cc:445: return;
We might want to revisit the behavior and always guarantee signal callbacks to
be run. Seems a bit hard otherwise for the client not to potentially deadlock,
esp. if you don't know whether it will run or not (no error returned). But I
guess it's consistent for now (we don't run them in the destructor).

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:24: #include
"gpu/command_buffer/common/gpu_control.h"
nit: already included in line 26

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
File gpu/command_buffer/common/gpu_control.h (right):

https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
gpu/command_buffer/common/gpu_control.h:41: // Runs |callback| when a sync point
is reached or the context is destroyed.
Does it really currently really run it when the context is destroyed?

For context lost, there is actually a test that defines the current behavior:
GLRendererTestSyncPoint.SignalSyncPointOnLostContext, see '// The sync point
shouldn't have happened since the context was lost.'

Although that check if before the context is destroyed.

[jamesr, 2013-10-01 01:36:25]

On 2013/09/27 18:13:04, sievers wrote:
> LGTM, but don't you have to update
> WebGraphicsContext3DCommandBufferImpl::signalSyncPoint() which calls
> CommandBufferProxyImpl::SignalSyncPoint() directly?

CommandBufferProxyImpl implements gpu::GpuControl, so it's calling the same
function.  When I switch cc to calling SignalSyncPoint() via cc::ContextSupport
the WebGraphicsContext3DCommandBufferImpl implementation and the
WebGraphicsContext3D entry points can go away completely (unless/until we expose
these to WebGL).

> 
>
https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
> content/common/gpu/client/command_buffer_proxy_impl.cc:445: return;
> We might want to revisit the behavior and always guarantee signal callbacks to
> be run. Seems a bit hard otherwise for the client not to potentially deadlock,
> esp. if you don't know whether it will run or not (no error returned). But I
> guess it's consistent for now (we don't run them in the destructor).

I agree this deserves some thought, but I don't think there is any action to
take with this patch.

> 
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
> gpu/command_buffer/client/gles2_implementation.cc:24: #include
> "gpu/command_buffer/common/gpu_control.h"
> nit: already included in line 26

Fixed

> 
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
> File gpu/command_buffer/common/gpu_control.h (right):
> 
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
> gpu/command_buffer/common/gpu_control.h:41: // Runs |callback| when a sync
point
> is reached or the context is destroyed.
> Does it really currently really run it when the context is destroyed?

Hmm, it doesn't.  Fixed the comment.

> 
> For context lost, there is actually a test that defines the current behavior:
> GLRendererTestSyncPoint.SignalSyncPointOnLostContext, see '// The sync point
> shouldn't have happened since the context was lost.'
> 
> Although that check if before the context is destroyed.

Right.  Do you think it's worth calling the lost context behavior out
specifically in this patch?

[commit-bot: I haz the power, 2013-10-01 01:41:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/24925002/17001

[commit-bot: I haz the power, 2013-10-01 05:17:39]

Failed to apply patch for content/common/gpu/client/command_buffer_proxy_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/common/gpu/client/command_buffer_proxy_impl.h
  Hunk #2 FAILED at 106.
  Hunk #3 succeeded at 124 (offset 2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/common/gpu/client/command_buffer_proxy_impl.h.rej

Patch:       content/common/gpu/client/command_buffer_proxy_impl.h
Index: content/common/gpu/client/command_buffer_proxy_impl.h
diff --git a/content/common/gpu/client/command_buffer_proxy_impl.h
b/content/common/gpu/client/command_buffer_proxy_impl.h
index
05c8f09b8b2150c37de5ca0289327cc4bf25a02e..bfa0d9cbd9cdab241bd96ed793ea4af8d941dd17
100644
--- a/content/common/gpu/client/command_buffer_proxy_impl.h
+++ b/content/common/gpu/client/command_buffer_proxy_impl.h
@@ -97,7 +97,6 @@ class CommandBufferProxyImpl
   virtual void SetParseError(gpu::error::Error error) OVERRIDE;
   virtual void SetContextLostReason(
       gpu::error::ContextLostReason reason) OVERRIDE;
-  virtual uint32 InsertSyncPoint() OVERRIDE;
 
   // gpu::GpuControl implementation:
   virtual bool SupportsGpuMemoryBuffer() OVERRIDE;
@@ -107,6 +106,9 @@ class CommandBufferProxyImpl
       unsigned internalformat,
       int32* id) OVERRIDE;
   virtual void DestroyGpuMemoryBuffer(int32 id) OVERRIDE;
+  virtual uint32 InsertSyncPoint() OVERRIDE;
+  virtual void SignalSyncPoint(uint32 sync_point,
+                               const base::Closure& callback) OVERRIDE;
 
   int GetRouteID() const;
   bool Echo(const base::Closure& callback);
@@ -123,11 +125,6 @@ class CommandBufferProxyImpl
   bool DiscardBackbuffer();
   bool EnsureBackbuffer();
 
-  // Makes this command buffer invoke a task when a sync point is reached, or
-  // the command buffer that inserted that sync point is destroyed.
-  bool SignalSyncPoint(uint32 sync_point,
-                       const base::Closure& callback);
-
   // Makes this command buffer invoke a task when a query is completed, or
   // the command buffer that inserted that sync point is destroyed or the
   // query was deleted. Should be invoked after endQuery.

[no sievers, 2013-10-01 16:57:13]

On 2013/10/01 01:36:25, jamesr wrote:
> On 2013/09/27 18:13:04, sievers wrote:
> > LGTM, but don't you have to update
> > WebGraphicsContext3DCommandBufferImpl::signalSyncPoint() which calls
> > CommandBufferProxyImpl::SignalSyncPoint() directly?
> 
> CommandBufferProxyImpl implements gpu::GpuControl, so it's calling the same
> function.  When I switch cc to calling SignalSyncPoint() via
cc::ContextSupport
> the WebGraphicsContext3DCommandBufferImpl implementation and the
> WebGraphicsContext3D entry points can go away completely (unless/until we
expose
> these to WebGL).
> 

Argh, right, nevermind.

> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
> > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/content/common/gpu/client/com...
> > content/common/gpu/client/command_buffer_proxy_impl.cc:445: return;
> > We might want to revisit the behavior and always guarantee signal callbacks
to
> > be run. Seems a bit hard otherwise for the client not to potentially
deadlock,
> > esp. if you don't know whether it will run or not (no error returned). But I
> > guess it's consistent for now (we don't run them in the destructor).
> 
> I agree this deserves some thought, but I don't think there is any action to
> take with this patch.
> 
> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
> > File gpu/command_buffer/client/gles2_implementation.cc (right):
> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/client/gle...
> > gpu/command_buffer/client/gles2_implementation.cc:24: #include
> > "gpu/command_buffer/common/gpu_control.h"
> > nit: already included in line 26
> 
> Fixed
> 
> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
> > File gpu/command_buffer/common/gpu_control.h (right):
> > 
> >
>
https://codereview.chromium.org/24925002/diff/1/gpu/command_buffer/common/gpu...
> > gpu/command_buffer/common/gpu_control.h:41: // Runs |callback| when a sync
> point
> > is reached or the context is destroyed.
> > Does it really currently really run it when the context is destroyed?
> 
> Hmm, it doesn't.  Fixed the comment.
> 
> > 
> > For context lost, there is actually a test that defines the current
behavior:
> > GLRendererTestSyncPoint.SignalSyncPointOnLostContext, see '// The sync point
> > shouldn't have happened since the context was lost.'
> > 
> > Although that check if before the context is destroyed.
> 
> Right.  Do you think it's worth calling the lost context behavior out
> specifically in this patch?

Nope. LGTM.

[commit-bot: I haz the power, 2013-10-02 00:25:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/24925002/49001

[commit-bot: I haz the power, 2013-10-02 01:49:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 03:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/24925002/49001

[commit-bot: I haz the power, 2013-10-02 07:07:38]

Change committed as 226412