gpu: Separate GpuControlService from GpuControl

gpu: Separate GpuControlService from GpuControl

Orignal goal of this CL is to make
InProcessCommandBuffer::CreateGpuMemoryBuffer and
DestroyGpuMemoryBuffer thread safe. Before this, Create runs
on the client thread and Destroy runs on the service thread
without any kind of synchronization. This change makes the
division closer to the cross-process implementation, moving
parts of the implementation from GpuControlService to
GpuControl/InProcessViewRenderer.

As a result, GpuControlService no longer needs to inherit
GpuControl. And GLES2Decoder has enough information to
decide on all gpu::Capabilities.

Need to implement the bare minimum client GpuControl for
gl_tests and gles2_conform_test. This currently involves
some boilerplate and duplication.

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

[boliu, 2014-04-21 23:09:05]

Juicy code review for you after you come back :)

[no sievers, 2014-04-23 23:57:28]

Looks like a good idea to split the service portion out of GpuControl. 

revaman@ to double check, also wondering where we want to do with the
GLImage/GpuMemoryBuffer id namespace. Should we maybe just treat these like any
other GL resource and have separate client and service side IDs from the id
generator?

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2692: caps.map_image =
!!image_manager();
Shouldn't we always have an ImageManager (see ContextGroup constructor)?

reveman@ should comment on whether we need this capability to be passed from
service to client.

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.cc:303: NOTIMPLEMENTED();
Looks like gl_tests do (I mean did, even before your patch) not support sync
points. We should really switch those over to use GLInProcessContext and have at
least one less implementation.

[boliu, 2014-04-24 00:04:02]

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2692: caps.map_image =
!!image_manager();
On 2014/04/23 23:57:29, sievers wrote:
> Shouldn't we always have an ImageManager (see ContextGroup constructor)?
> 
> reveman@ should comment on whether we need this capability to be passed from
> service to client.

I was going to ask reveman too. The condition before this CL was
(image_manager() && gpu_memory_buffer_factory_), but factory is now a client
side concept.

But this change also doesn't matter for the cross-process context:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

Either way, I don't want to be accidentally turning on zero-copy path for other
platforms.

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.cc:303: NOTIMPLEMENTED();
On 2014/04/23 23:57:29, sievers wrote:
> Looks like gl_tests do (I mean did, even before your patch) not support sync
> points. We should really switch those over to use GLInProcessContext and have
at
> least one less implementation.

Can I do this in follow up?

[reveman, 2014-04-24 00:39:10]

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2692: caps.map_image =
!!image_manager();
On 2014/04/23 23:57:29, sievers wrote:
> Shouldn't we always have an ImageManager (see ContextGroup constructor)?
> 
> reveman@ should comment on whether we need this capability to be passed from
> service to client.

I would like to remove this cap completely and require the service to always
implement it. I did this in https://codereview.chromium.org/205193006/ by
considering an implementation that always fails to allocate buffers a valid
implementation but piman wasn't a fan of that. I think we should just implement
this everywhere (pepper, mojo?) and remove the map_image capability.

What happens in the case of pepper with this change?

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:29: void
DestroyGpuMemoryBuffer(int32 id);
A Destroy function but no Create function is weird. Do we still need this?

[boliu, 2014-04-24 01:30:56]

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2692: caps.map_image =
!!image_manager();
On 2014/04/24 00:39:11, reveman wrote:
> On 2014/04/23 23:57:29, sievers wrote:
> > Shouldn't we always have an ImageManager (see ContextGroup constructor)?
> > 
> > reveman@ should comment on whether we need this capability to be passed from
> > service to client.
> 
> I would like to remove this cap completely and require the service to always
> implement it. I did this in https://codereview.chromium.org/205193006/ by
> considering an implementation that always fails to allocate buffers a valid
> implementation but piman wasn't a fan of that. I think we should just
implement
> this everywhere (pepper, mojo?) and remove the map_image capability.
> 
> What happens in the case of pepper with this change?

Pepper and mojo are not affected. They don't implement gpu memory buffers and
just returns the default Capabilities object, so not affected by this change.

gpu::Capabilities was added to help cc so it doesn't have to parse extension
strings. If cc has another mechanism to correct detect whether gpu memory
buffers are supported, then we can remove it now? For android, kEnableMapImage
is added only for webview and not chrome, so that should be ok already.

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:29: void
DestroyGpuMemoryBuffer(int32 id);
On 2014/04/24 00:39:11, reveman wrote:
> A Destroy function but no Create function is weird. Do we still need this?

Renamed to UnregisterGpuMemoryBuffer to match Register below.

Also removed comment on Register. The "get an ID" part is outdated and the rest
is obvious.

[no sievers, 2014-04-25 01:16:35]

https://codereview.chromium.org/235563002/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (left):

https://codereview.chromium.org/235563002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:23: class GPU_EXPORT
GpuControlService : public GpuControl {
Is it possible to move GpuControl to command_buffer/client/gpu_control.h now?

[boliu, 2014-04-25 02:23:46]

On 2014/04/25 01:16:35, sievers wrote:
>
https://codereview.chromium.org/235563002/diff/140001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gpu_control_service.h (left):
> 
>
https://codereview.chromium.org/235563002/diff/140001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gpu_control_service.h:23: class GPU_EXPORT
> GpuControlService : public GpuControl {
> Is it possible to move GpuControl to command_buffer/client/gpu_control.h now?

Yep. Used ./tools/git/move_source_file.py and looks like it did 99% of the work.
Only had to manually edit the gypi files.

[boliu, 2014-04-25 19:45:24]

Ping? Is this good to go?

[no sievers, 2014-04-25 19:51:48]

LGTM is reveman is happy.

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:29: void
DestroyGpuMemoryBuffer(int32 id);
On 2014/04/24 01:30:56, boliu wrote:
> On 2014/04/24 00:39:11, reveman wrote:
> > A Destroy function but no Create function is weird. Do we still need this?
> 
> Renamed to UnregisterGpuMemoryBuffer to match Register below.
> 
> Also removed comment on Register. The "get an ID" part is outdated and the
rest
> is obvious.

Actually the mismatch is already there in gpu_memory_buffer_manager.h.

It's Register and then Destroy.

[boliu, 2014-04-25 20:27:15]

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:29: void
DestroyGpuMemoryBuffer(int32 id);
On 2014/04/25 19:51:48, sievers wrote:
> On 2014/04/24 01:30:56, boliu wrote:
> > On 2014/04/24 00:39:11, reveman wrote:
> > > A Destroy function but no Create function is weird. Do we still need this?
> > 
> > Renamed to UnregisterGpuMemoryBuffer to match Register below.
> > 
> > Also removed comment on Register. The "get an ID" part is outdated and the
> rest
> > is obvious.
> 
> Actually the mismatch is already there in gpu_memory_buffer_manager.h.
> 
> It's Register and then Destroy.

Noticed. I was thinking maybe I should do that as a follow up and avoid changing
too many things in one go. But this is already doing n things, one more won't
hurt. So done.

[boliu, 2014-04-28 22:36:54]

reveman: ping?

[boliu, 2014-04-30 22:57:28]

On 2014/04/28 22:36:54, boliu wrote:
> reveman: ping?

ping again?

[reveman, 2014-05-01 18:47:07]

sorry for the delay. lgtm.

[boliu, 2014-05-01 18:51:53]

+viettrungluu for mojo
+piman for ppapi/proxy (and maybe the whole thing)

[piman, 2014-05-01 19:28:04]

Not sure it's a net win to make GpuControlService not a GpuControl any more.

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:22: class GPU_EXPORT
GpuControlService {
nit: not sure this should still be called GpuControlService.

Should it be merged with CommandBufferService?

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:25: gles2::MailboxManager*
mailbox_manager,
You're not using the MailboxManager. remove?

[boliu, 2014-05-01 22:10:45]

About not having GpuControlService inherit GpuControl. The new code duplication
are in the test harnesses, and Daniel suggested to have tests directly use
InProcessCommandBuffer instead of rolling their own.

For production code, I think it's a win.

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:22: class GPU_EXPORT
GpuControlService {
On 2014/05/01 19:28:05, piman wrote:
> nit: not sure this should still be called GpuControlService.
> 
> Should it be merged with CommandBufferService?

Looked this a bit. But didn't do anything.

A straight forward merging introduces circular dependency. Right now dependency
goes GpuControlService -> decoder -> scheduler -> command buffer. So control and
command buffer are merged, then it needs setters for memory buffer and query
managers, which makes init and tear down brittle.

Maybe it makes sense for decoder to set the two managers directly, but it looks
like a unnecessary layering violation.

Thoughts?

https://codereview.chromium.org/235563002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:25: gles2::MailboxManager*
mailbox_manager,
On 2014/05/01 19:28:05, piman wrote:
> You're not using the MailboxManager. remove?

Done.

[piman, 2014-05-01 22:30:23]

On Thu, May 1, 2014 at 3:10 PM, <boliu@chromium.org> wrote:

> About not having GpuControlService inherit GpuControl. The new code
> duplication
> are in the test harnesses, and Daniel suggested to have tests directly use
> InProcessCommandBuffer instead of rolling their own.
>
> For production code, I think it's a win.
>
>
>
> https://codereview.chromium.org/235563002/diff/180001/gpu/
> command_buffer/service/gpu_control_service.h
> File gpu/command_buffer/service/gpu_control_service.h (right):
>
> https://codereview.chromium.org/235563002/diff/180001/gpu/
> command_buffer/service/gpu_control_service.h#newcode22
> gpu/command_buffer/service/gpu_control_service.h:22: class GPU_EXPORT
> GpuControlService {
> On 2014/05/01 19:28:05, piman wrote:
>
>> nit: not sure this should still be called GpuControlService.
>>
>
>  Should it be merged with CommandBufferService?
>>
>
> Looked this a bit. But didn't do anything.
>
> A straight forward merging introduces circular dependency. Right now
> dependency goes GpuControlService -> decoder -> scheduler -> command
> buffer.

So control and command buffer are merged, then it needs setters
> for memory buffer and query managers, which makes init and tear down
> brittle.
>
> Maybe it makes sense for decoder to set the two managers directly, but
> it looks like a unnecessary layering violation.
>
> Thoughts?


Ok, if it's hard let's not do it now. I think we'd benefit from fewer
classes, but maybe this is not the right axis.

>
>
> https://codereview.chromium.org/235563002/diff/180001/gpu/
> command_buffer/service/gpu_control_service.h#newcode25
> gpu/command_buffer/service/gpu_control_service.h:25:
> gles2::MailboxManager* mailbox_manager,
> On 2014/05/01 19:28:05, piman wrote:
>
>> You're not using the MailboxManager. remove?
>>
>
> Done.
>
> https://codereview.chromium.org/235563002/
>

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

[piman, 2014-05-01 22:30:35]

lgtm

[viettrungluu, 2014-05-01 23:03:29]

mojo lgtm

[boliu, 2014-05-01 23:21:14]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-01 23:22:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/235563002/200001

[commit-bot: I haz the power, 2014-05-02 05:22:06]

Change committed as 267753

[vmiura, 2014-05-08 20:21:22]

https://codereview.chromium.org/235563002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2358:
state_.default_vertex_attrib_manager = new VertexAttribManager();
Was there a reason for this change?

Using non zero default VAO was an optimization.

[boliu, 2014-05-08 20:23:24]

https://codereview.chromium.org/235563002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/235563002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2358:
state_.default_vertex_attrib_manager = new VertexAttribManager();
On 2014/05/08 20:21:22, vmiura wrote:
> Was there a reason for this change?
> 
> Using non zero default VAO was an optimization.

Doh! Really sorry! I had your CL locally reverted for something else.

Let me fix that now.

[boliu, 2014-05-08 20:34:39]

Reland of VAO change in https://codereview.chromium.org/278653002/

Sorry again!