mustash: gpu: implement SignalQuery in CommandBufferLocal

mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus. GLRenderer::GetFramebufferPixelsAsync calls this function to respond to CopyOutputRequests.

BUG=557397
Committed: https://crrev.com/9fca1d27747026b7eb76f334477745d1b4ad9399
Cr-Commit-Position: refs/heads/master@{#370781}

[Hadi, 2016-01-20 19:38:46]

moshayedi@chromium.org changed reviewers:
+ penghuang@chromium.org

[Hadi, 2016-01-20 19:38:46]

PTAL.

[Peng, 2016-01-20 19:57:13]

https://codereview.chromium.org/1610783002/diff/1/components/mus/gles2/comman...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1610783002/diff/1/components/mus/gles2/comman...
components/mus/gles2/command_buffer_local.cc:338:
gpu::gles2::QueryManager::Query* query = query_manager->GetQuery(query_id);
CommandBufferLocal::SignalQuery() is called on the client thread (the mus main
thread), the QueryManager is used on the GPU thread. Seems GetQuery() &
AddCallback() are not thread safe (see [1]). So I think you have to post a task
to client GPU thread, and then execute those code. And I believe the callback
passed to query->AddCallback() will be invoked on the GPU thread, but the
callback of SignalQuery() should be called on the client thread, so you have to
some extra work for it (Like the code at line 327).


BTW, you may consider adding a SignalQuery() in CommandBufferDriver instead of
exposing decoder to CommandBufferLocal. And call it from
CommandBufferLocal::SignalQueryOnGpuThread()



[1]
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

[Hadi, 2016-01-20 21:33:30]

https://codereview.chromium.org/1610783002/diff/1/components/mus/gles2/comman...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1610783002/diff/1/components/mus/gles2/comman...
components/mus/gles2/command_buffer_local.cc:338:
gpu::gles2::QueryManager::Query* query = query_manager->GetQuery(query_id);
On 2016/01/20 19:57:13, Peng wrote:
> CommandBufferLocal::SignalQuery() is called on the client thread (the mus main
> thread), the QueryManager is used on the GPU thread. Seems GetQuery() &
> AddCallback() are not thread safe (see [1]). So I think you have to post a
task
> to client GPU thread, and then execute those code. And I believe the callback
> passed to query->AddCallback() will be invoked on the GPU thread, but the
> callback of SignalQuery() should be called on the client thread, so you have
to
> some extra work for it (Like the code at line 327).
> 
> 
> BTW, you may consider adding a SignalQuery() in CommandBufferDriver instead of
> exposing decoder to CommandBufferLocal. And call it from
> CommandBufferLocal::SignalQueryOnGpuThread()
> 
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Done. Used gpu_state_->command_buffer_task_runner()->PostTask() to send tasks to
GPU thread.

[Peng, 2016-01-20 22:27:29]

https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
components/mus/gles2/command_buffer_local.cc:558: driver_->SignalQuery(query_id,
callback);
You must make sure the callback is called on the client thread.

[Hadi, 2016-01-21 15:46:27]

https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
components/mus/gles2/command_buffer_local.cc:558: driver_->SignalQuery(query_id,
callback);
On 2016/01/20 22:27:29, Peng wrote:
> You must make sure the callback is called on the client thread.

Done.

[Peng, 2016-01-21 15:53:25]

On 2016/01/21 15:46:27, Hadi wrote:
>
https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
> File components/mus/gles2/command_buffer_local.cc (right):
> 
>
https://codereview.chromium.org/1610783002/diff/20001/components/mus/gles2/co...
> components/mus/gles2/command_buffer_local.cc:558:
driver_->SignalQuery(query_id,
> callback);
> On 2016/01/20 22:27:29, Peng wrote:
> > You must make sure the callback is called on the client thread.
> 
> Done.

lgtm. Please ask piman@ to take a look too.

[Hadi, 2016-01-21 15:56:48]

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

[Hadi, 2016-01-21 15:56:48]

piman@ can you please take a look at this?

[Hadi, 2016-01-21 16:00:54]

Description was changed from

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

BUG=NONE
==========

to

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus.
GLRenderer::GetFramebufferPixelsAsync calls this function to respond to
CopyOutputRequests.

BUG=557397
==========

[piman, 2016-01-21 20:13:46]

lgtm

[Hadi, 2016-01-21 20:14:52]

moshayedi@chromium.org changed reviewers:
+ fsamuel@google.com

[Hadi, 2016-01-21 20:14:53]

fsamuel@ can you please provide OWNERS review for this?

[Hadi, 2016-01-21 20:16:09]

moshayedi@chromium.org changed reviewers:
+ fsamuel@chromium.org
- fsamuel@google.com

[Hadi, 2016-01-21 20:16:09]

fsamuel@ can you please provide OWNERS review for this?

(I sent the previous message to @google.com account by mistake)

[Fady Samuel, 2016-01-21 20:17:30]

lgtm

[Hadi, 2016-01-21 20:18:50]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2016-01-21 20:19:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610783002/40001

[commit-bot: I haz the power, 2016-01-21 20:50:05]

Description was changed from

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus.
GLRenderer::GetFramebufferPixelsAsync calls this function to respond to
CopyOutputRequests.

BUG=557397
==========

to

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus.
GLRenderer::GetFramebufferPixelsAsync calls this function to respond to
CopyOutputRequests.

BUG=557397
==========

[commit-bot: I haz the power, 2016-01-21 20:50:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 20:52:28]

Description was changed from

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus.
GLRenderer::GetFramebufferPixelsAsync calls this function to respond to
CopyOutputRequests.

BUG=557397
==========

to

==========
mustash: gpu: implement SignalQuery in CommandBufferLocal

We need this to make snapshots work in mus.
GLRenderer::GetFramebufferPixelsAsync calls this function to respond to
CopyOutputRequests.

BUG=557397

Committed: https://crrev.com/9fca1d27747026b7eb76f334477745d1b4ad9399
Cr-Commit-Position: refs/heads/master@{#370781}
==========

[commit-bot: I haz the power, 2016-01-21 20:52:29]

Patchset 3 (id:??) landed as
https://crrev.com/9fca1d27747026b7eb76f334477745d1b4ad9399
Cr-Commit-Position: refs/heads/master@{#370781}