gpu: Remove Echo and SwapCompletion GL interfacess

gpu: Remove Echo and SwapCompletion GL interfaces

This functionality can be implemented with sync point callbacks.

Now that swap completion is entirely an implementation detail
of the OutputSurface this also allows for eliminating some
IPC message redundancy. Ideally SwapCompletion would be unsolicited
and also include LatencyInfo (where needed/supported).

Also remove the message pattern matching logic in gpu_channel.cc
for Flush/Echo. This kind of matching does not work reliably,
since OnMessageReceived races with HandleMessage
(i.e. the next msg might or might not be in deferred_messages_).
Also, the original motivation in bug 407529 is described
as making sure SwapBuffers gets handled immediately,
which was separately fixed by merging the LatencyInfo with
the Flush IPC.

BUG=417945
Committed: https://crrev.com/42243047e7418892929a8e5b19fe12b9db4b9c4c
Cr-Commit-Position: refs/heads/master@{#297869}

[no sievers, 2014-09-30 01:36:48]

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

[no sievers, 2014-09-30 01:39:56]

ptal

[no sievers, 2014-09-30 01:45:36]

Ah wait.. I do have to keep the peeking logic, but apply it to RetireSyncPoint.
Which will be better in general.

[no sievers, 2014-09-30 21:51:01]

sievers@chromium.org changed reviewers:
+ jamesr@chromium.org, jbauman@chromium.org, tsepez@chromium.org

[no sievers, 2014-09-30 21:53:12]

piman: everything
jamesr: mojo stamp, or everything

jbauman: for double-checking the generalization of
MatchSwapBuffersMessagesPattern() in gpu_channel.cc

tsepez: gpu_messages.h stamp for msg removal

[Tom Sepez, 2014-09-30 22:50:13]

Rubberstamp LGTM on deleting messages. Thanks!

[jamesr, 2014-10-01 00:06:29]

https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
File cc/test/test_gles2_interface.h (right):

https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
cc/test/test_gles2_interface.h:193: virtual void SwapBuffers() OVERRIDE;
sucks to encode these on GLES2Interface.  I don't think they belong here -
https://www.khronos.org/opengles/sdk/docs/man/ has no 'SwapBuffers' or
'PostSubBuffer'.  I think of GpuControl as being the place to put things that
aren't GLES2 entry points but need ordering or other guarantees and
GLES2Interface as being just true GL entry points.  but maybe that distinction
isn't useful

[no sievers, 2014-10-01 00:12:53]

https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
File cc/test/test_gles2_interface.h (right):

https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
cc/test/test_gles2_interface.h:193: virtual void SwapBuffers() OVERRIDE;
On 2014/10/01 00:06:28, jamesr wrote:
> sucks to encode these on GLES2Interface.  I don't think they belong here -
> https://www.khronos.org/opengles/sdk/docs/man/ has no 'SwapBuffers' or
> 'PostSubBuffer'.  I think of GpuControl as being the place to put things that
> aren't GLES2 entry points but need ordering or other guarantees and
> GLES2Interface as being just true GL entry points.  but maybe that distinction
> isn't useful

In this patch I'm just removing a layer of indirection (GpuControl::Swap()
calling GLES2Impl::Swap). It might have to stay in GLES2Impl at least since it
inserts a token into the cmdbuffer. (Not sure if it is needed in
GLES2Interface.)

[jamesr, 2014-10-01 00:14:02]

On 2014/10/01 00:12:53, sievers wrote:
>
https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
> File cc/test/test_gles2_interface.h (right):
> 
>
https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
> cc/test/test_gles2_interface.h:193: virtual void SwapBuffers() OVERRIDE;
> On 2014/10/01 00:06:28, jamesr wrote:
> > sucks to encode these on GLES2Interface.  I don't think they belong here -
> > https://www.khronos.org/opengles/sdk/docs/man/ has no 'SwapBuffers' or
> > 'PostSubBuffer'.  I think of GpuControl as being the place to put things
that
> > aren't GLES2 entry points but need ordering or other guarantees and
> > GLES2Interface as being just true GL entry points.  but maybe that
distinction
> > isn't useful
> 
> In this patch I'm just removing a layer of indirection (GpuControl::Swap()
> calling GLES2Impl::Swap). It might have to stay in GLES2Impl at least since it
> inserts a token into the cmdbuffer. (Not sure if it is needed in
> GLES2Interface.)

GLES2Impl is both GLES2Interface and GpuControl, I think.  My point was about
which 'public' thing exposes it.

[no sievers, 2014-10-01 00:51:12]

On 2014/10/01 00:14:02, jamesr wrote:
> On 2014/10/01 00:12:53, sievers wrote:
> >
>
https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
> > File cc/test/test_gles2_interface.h (right):
> > 
> >
>
https://codereview.chromium.org/619453002/diff/40001/cc/test/test_gles2_inter...
> > cc/test/test_gles2_interface.h:193: virtual void SwapBuffers() OVERRIDE;
> > On 2014/10/01 00:06:28, jamesr wrote:
> > > sucks to encode these on GLES2Interface.  I don't think they belong here -
> > > https://www.khronos.org/opengles/sdk/docs/man/ has no 'SwapBuffers' or
> > > 'PostSubBuffer'.  I think of GpuControl as being the place to put things
> that
> > > aren't GLES2 entry points but need ordering or other guarantees and
> > > GLES2Interface as being just true GL entry points.  but maybe that
> distinction
> > > isn't useful
> > 
> > In this patch I'm just removing a layer of indirection (GpuControl::Swap()
> > calling GLES2Impl::Swap). It might have to stay in GLES2Impl at least since
it
> > inserts a token into the cmdbuffer. (Not sure if it is needed in
> > GLES2Interface.)
> 
> GLES2Impl is both GLES2Interface and GpuControl, I think.  My point was about
> which 'public' thing exposes it.


Yea I can leave Swap/PartialSwap in ContextSupport actually. But then we should
remove it from GLES2Interface (I'll have to see how to exclude it from there
since it's auto-generated).

Then I can add a ContextSupport::SwapAndAck(base::Closure callback) later which
will allow me to reduce the IPC messages involved.

[piman, 2014-10-01 02:03:31]

LGTM, thanks for cleanup!

We can move SwapBuffers/PostSubBuffers to ContextSupport if it's practical, but
I think command buffer commands are autogen'ed into GLES2Interface.

[jamesr, 2014-10-01 02:04:39]

It's not a super huge deal either way.  This cleanup is great either way.

mojo/ lgtm

[jbauman, 2014-10-01 02:15:38]

lgtm

https://codereview.chromium.org/619453002/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/619453002/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel.cc:680: // SwapBuffer should be AsyncFlush->Echo.
We only try to match Echo message.
Change this comment.

[jbauman, 2014-10-01 02:21:00]

https://codereview.chromium.org/619453002/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/619453002/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel.cc:680: // SwapBuffer should be AsyncFlush->Echo.
We only try to match Echo message.
On 2014/10/01 02:15:38, jbauman wrote:
> Change this comment.

Although, to keep the logic working in a similar way you should probably check
for the AsyncFlush, RetireSyncPoint, SignalSyncPoint sequence (and
subsequences). Look at https://codereview.chromium.org/564903002/ to see how
that might be handled.

[no sievers, 2014-10-01 21:54:49]

Ok so I have left (Partial)Swap in ContextSupport. I think it does make more
sense to have it there, since I'll be adding back a new type of swap completion
callback (that has LatencyInfo and is only available for certain onscreen
surfaces), and these callbacks fit better in that class.

I have also entirely removed the peeking-at-the-next message logic. It does not
work that well in practice because IPC message handling is very racy across all
the threads involved. When I tried this (admittedly on a debug Linux build) the
channel reader would only read out and post a task for a message at a time and
so forth. So I wouldn't really get to peek at the SignalSyncPoint because
deferred_messages_ was drained before it was put there.

How about I do this instead?
a) https://codereview.chromium.org/619993002/
b) send the put offset with InsertSyncPoint in a separate patch

[piman, 2014-10-02 17:42:52]

LGTM. Adding a Flush+InsertSyncPoint IPC seems like a good idea.

[no sievers, 2014-10-02 18:28:26]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-10-02 18:29:48]

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

[commit-bot: I haz the power, 2014-10-02 18:35:29]

Committed patchset #5 (id:80001) as da4b00b9cd2c89a5facd0d522b0435ce516a6da6

[commit-bot: I haz the power, 2014-10-02 18:36:19]

Patchset 5 (id:??) landed as
https://crrev.com/42243047e7418892929a8e5b19fe12b9db4b9c4c
Cr-Commit-Position: refs/heads/master@{#297869}

[no sievers, 2014-10-03 01:55:04]

On 2014/10/02 17:42:52, piman (Very slow to review) wrote:
> LGTM. Adding a Flush+InsertSyncPoint IPC seems like a good idea.

So I just realized that for a Flush+InsertSyncPoint-command to be very useful it
would also have to imply the deep flush/glFlush because the pattern is not
unlikely to be like this:

void CompositorOutputSurface::SwapBuffers(cc::CompositorFrame* frame) {
...

  context->Flush();
  uint32 sync_point = context->InsertSyncPointCHROMIUM(); // So this already
doesn't send a Flush IPC
}

That however might cause us to deep flush more often than desirable.

I wonder if it's better to move in a direction where the client uses less
synchronization primitives, ideally only sync points and doesn't use shallow or
deep flushes anymore. It's better for the service to use the best working
primitive available (glFlush or fence, or neither with virtual contexts). And
then you could optimize further, for example we might then be able to update
'put' through shared memory (like we do for get). And the service might also be
able to make better scheduling decisions unlike when flush is entirely
client-driven (it could decide to parse on its own when it has no work).