Introduce command buffer control thread

Introduce command buffer control thread

This CL introduce a control thread for the GPU command buffer. We listen on the
command buffer pipe on a control thread so that we can respond to messages with
low latency. Currently, we don't have any special low-latency processing on the
control thread, but the plan is to implement sync points on the control thread.
Instead, we just forward all the messages to the driver thread by re-wrapping
the arguments in base::Callbacks.

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/1658f1188749714b0dc601f33702c6fb242d8256

[abarth-chromium, 2014-11-18 20:10:14]

PTAL

[jamesr, 2014-11-19 06:43:02]

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:12: void
DestoryDriver(scoped_ptr<CommandBufferDriver> driver) {
typo 'Destory'

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:31: base::Passed(request.Pass())));
base::Passed(xxx.Pass()) is redundant, you should be able to just say
base::Passed(&request) here. it does the rest

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:37: FROM_HERE,
base::Bind(&DestoryDriver, base::Passed(driver_.Pass())));
same re: Passed+Pass

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:46:
base::Passed(shared_state.Pass())));
ditto

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:74:
base::Passed(transfer_buffer.Pass()), size));
ditto re base::Passed(...Pass())

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.h (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.h:17: class CommandBufferImpl : public
CommandBuffer {
could use a class level comment

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.h:43:
scoped_ptr<StrongBinding<CommandBuffer>> binding_;
hmm, no inherent need to have a scoped_ptr<> to the binding.  with Binding<> you
can allocate a binding and associate it with an interface without binding it to
a pipe+thread, then later call .Bind() to do the actual hookup. I didn't add the
Bind() calls to StrongBinding<> (although the interface-only c'tor is there)
because i couldn't think of a case where it was needed but i clearly wasn't
creative enough.  could you add void Bind(InterfaceRequest<Interface>) to
StrongBinding<> and use that instead of the extra heap allocation here?

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/gpu_...
File mojo/services/gles2/gpu_impl.h (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/gpu_...
mojo/services/gles2/gpu_impl.h:35: return control_thread_.task_runner();
the threading rules for these objects is not clear to me - are they only safe
for use on the driver thread? could we make that more explicit if so (i.e. add a
threadchecker member to this, perhaps?)

[abarth-chromium, 2014-11-19 15:42:11]

Thanks!

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:31: base::Passed(request.Pass())));
On 2014/11/19 at 06:43:02, jamesr wrote:
> base::Passed(xxx.Pass()) is redundant, you should be able to just say
base::Passed(&request) here. it does the rest

Ah, it was the & that I was missing.  Thanks!

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.h (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.h:43:
scoped_ptr<StrongBinding<CommandBuffer>> binding_;
On 2014/11/19 at 06:43:02, jamesr wrote:
> hmm, no inherent need to have a scoped_ptr<> to the binding.  with Binding<>
you can allocate a binding and associate it with an interface without binding it
to a pipe+thread, then later call .Bind() to do the actual hookup. I didn't add
the Bind() calls to StrongBinding<> (although the interface-only c'tor is there)
because i couldn't think of a case where it was needed but i clearly wasn't
creative enough.  could you add void Bind(InterfaceRequest<Interface>) to
StrongBinding<> and use that instead of the extra heap allocation here?

Sure thing.

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/gpu_...
File mojo/services/gles2/gpu_impl.h (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/gpu_...
mojo/services/gles2/gpu_impl.h:35: return control_thread_.task_runner();
On 2014/11/19 at 06:43:02, jamesr wrote:
> the threading rules for these objects is not clear to me - are they only safe
for use on the driver thread? could we make that more explicit if so (i.e. add a
threadchecker member to this, perhaps?)

I'm happy to add comments about which thread these objects should be used on,
but I'm not sure adding a threadchecker to this state object buys us much.  We
take references to these objects directly in CommandBufferDriver rather than
using State as a bundle.

I just made them into a bundle to make it easier for code that allocates GpuImpl
objects not to have to worry about what all state the GpuImpl object needs (and
because that code is duplicate for Desktop and Android).

[abarth-chromium, 2014-11-19 16:54:10]

PTAL

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.h (right):

https://codereview.chromium.org/739493002/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.h:17: class CommandBufferImpl : public
CommandBuffer {
On 2014/11/19 at 06:43:02, jamesr wrote:
> could use a class level comment

Done.

[jamesr, 2014-11-19 20:57:18]

lgtm if you shimmy the echo callback back to the control thread.  I can try to
dig into whether that works or not but I'm not super optimistic.  I'd say either
land this with a shimmy back, or wait until we figure out if it's going to work.
 If we do something thread-unsafe here I think it'll cause lots of sadness with
flaky crashes and crap

https://codereview.chromium.org/739493002/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/strong_binding.h (right):

https://codereview.chromium.org/739493002/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/strong_binding.h:73: binding_.Bind(handle.Pass(),
waiter);
add an assert() that we aren't already bound since if someone tries to re-bind
to a new pipe it'll operator delete the old impl and there's no way to
re-associate a binding

https://codereview.chromium.org/739493002/diff/60001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/739493002/diff/60001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_driver.cc:185: callback.Run();
i'm not sure if this works (i.e. if the callback binds to something that makes a
thread-unsafe call or not).  will try to investigate

[abarth-chromium, 2014-11-20 04:09:19]

Committed patchset #5 (id:80001) manually as
1658f1188749714b0dc601f33702c6fb242d8256 (presubmit successful).