GPU: Adding sync points for cross-channel synchronization

GPU: Adding sync points for cross-channel synchronization

Theory of operation: command buffer 1 calls InsertSyncPoint, it returns an ID,
command buffer 2 calls WaitSyncPoint on that ID (even if on another channel).
The wait is pipelined.

InsertSyncPoint is handled on the IO thread in the GPU process, so it's
presumably fast, but its effect is ordered wrt the other messages.
Some benefits of the approach:
- once InsertSyncPoint returns the ID, the sync point is already enqueued to be
eventually retired, so it makes it very hard to cause deadlocks by incorrect
operation on the client side.
- the wait will return if the command buffer that inserted the sync point gets
destroyed.

This primitive should be enough for guaranteeing browser->renderer ordering.
Additional changes are needed to safely handle renderer->browser ordering
(especially in case of buggy/malicious renderer).

BUG=112299
TEST=manual (With other patches)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140653

[piman, 2012-06-05 02:20:33]

apatrick: please review
brettw: OWNERS for content/content_common.gypi and  ppapi/proxy

[Fady Samuel, 2012-06-05 02:26:20]

On 2012/06/05 02:20:33, piman wrote:
> apatrick: please review
> brettw: OWNERS for content/content_common.gypi and  ppapi/proxy

Interesting. This is pretty cool. Could this be used to synchronize a guest and
embedder if a guest grabs its context from the GPUChannelHost instead of from
the pepper channel?

[piman, 2012-06-05 02:43:07]

On Mon, Jun 4, 2012 at 7:26 PM, <fsamuel@chromium.org> wrote:

> On 2012/06/05 02:20:33, piman wrote:
>
>> apatrick: please review
>> brettw: OWNERS for content/content_common.gypi and  ppapi/proxy
>>
>
> Interesting. This is pretty cool. Could this be used to synchronize a
> guest and
> embedder if a guest grabs its context from the GPUChannelHost instead of
> from
> the pepper channel?
>

At this point, only if you have enough "trust" between the renderers. A
malicious renderer can put itself into a state where the GPU process will
keep the channel unscheduled, and so generate sync point ID that will never
be reached, and send that to the other renderer. If that other one calls
WaitSyncPoint on that id, the channel will never execute commands after
that (i.e. stop rendering). I don't know if other malicious uses could lead
to data leaks, etc.
Also, this doesn't do anything to solve cross-channel resource sharing.


>
>
https://chromiumcodereview.**appspot.com/10510013/<https://chromiumcodereview...
>

[piman, 2012-06-05 02:46:11]

cc: backer FYI

[Fady Samuel, 2012-06-05 04:20:23]

Thinking out loud (please don't let this block this cl): Would a timeout
solve this problem?

Thanks,

Fady

Would a timeout solve this issue [we may occasionally miss frames
though..I'm not sure]?



On Mon, Jun 4, 2012 at 10:42 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Jun 4, 2012 at 7:26 PM, <fsamuel@chromium.org> wrote:
>
>> On 2012/06/05 02:20:33, piman wrote:
>>
>>> apatrick: please review
>>> brettw: OWNERS for content/content_common.gypi and  ppapi/proxy
>>>
>>
>> Interesting. This is pretty cool. Could this be used to synchronize a
>> guest and
>> embedder if a guest grabs its context from the GPUChannelHost instead of
>> from
>> the pepper channel?
>>
>
> At this point, only if you have enough "trust" between the renderers. A
> malicious renderer can put itself into a state where the GPU process will
> keep the channel unscheduled, and so generate sync point ID that will never
> be reached, and send that to the other renderer. If that other one calls
> WaitSyncPoint on that id, the channel will never execute commands after
> that (i.e. stop rendering). I don't know if other malicious uses could lead
> to data leaks, etc.
> Also, this doesn't do anything to solve cross-channel resource sharing.
>
>
>>
>>
https://chromiumcodereview.**appspot.com/10510013/<https://chromiumcodereview...
>>
>
>

[piman, 2012-06-05 06:46:41]

On Mon, Jun 4, 2012 at 9:20 PM, Fady Samuel <fsamuel@chromium.org> wrote:

> Thinking out loud (please don't let this block this cl): Would a timeout
> solve this problem?
>
> Thanks,
>
> Fady
>
> Would a timeout solve this issue [we may occasionally miss frames
> though..I'm not sure]?
>

A timeout never correctly solves any issue... What if it legitimately took
that long? What if there was an IO hitch?

My current plan is to add something like a SignalSyncPoint with a callback,
so that if the renderer sends an update-pending-sync-point, the browser
would not take that update until the callback is called. If it doesn't get
called, the renderer (assumed buggy/malicious) won't update anymore,
without compromising anything else.
There's other reasons why we don't want to unschedule the browser's context
anyway - it could case a deadlock.

>
>
>
> On Mon, Jun 4, 2012 at 10:42 PM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>> On Mon, Jun 4, 2012 at 7:26 PM, <fsamuel@chromium.org> wrote:
>>
>>> On 2012/06/05 02:20:33, piman wrote:
>>>
>>>> apatrick: please review
>>>> brettw: OWNERS for content/content_common.gypi and  ppapi/proxy
>>>>
>>>
>>> Interesting. This is pretty cool. Could this be used to synchronize a
>>> guest and
>>> embedder if a guest grabs its context from the GPUChannelHost instead of
>>> from
>>> the pepper channel?
>>>
>>
>> At this point, only if you have enough "trust" between the renderers. A
>> malicious renderer can put itself into a state where the GPU process will
>> keep the channel unscheduled, and so generate sync point ID that will never
>> be reached, and send that to the other renderer. If that other one calls
>> WaitSyncPoint on that id, the channel will never execute commands after
>> that (i.e. stop rendering). I don't know if other malicious uses could lead
>> to data leaks, etc.
>> Also, this doesn't do anything to solve cross-channel resource sharing.
>>
>>
>>>
>>>
https://chromiumcodereview.**appspot.com/10510013/<https://chromiumcodereview...
>>>
>>
>>
>

[apatrick_chromium, 2012-06-05 18:44:15]

I gave this three passes and I think it's right. Just nits.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/gpu_channel.cc:24: #include "ipc/ipc_channel.h"
and ipc_channel_proxy.h

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/gpu_command_buffer_stub.h:10: 
#include <deque>

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.cc:37: std::swap(list, it->second);
nit: list.swap(it->second)

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/sync_point_manager.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:8: 
#include <vector>

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:39: typedef base::hash_map<uint32,
std::vector<base::Closure> > SyncPointMap;
Did you mean to use ClosureList here?

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:47: uint32 next_sync_point_;
nit: DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/content_com...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/content_com...
content/content_common.gypi:204: 'common/gpu/sync_point_manager.h',
nit: not in alphabetic order

https://chromiumcodereview.appspot.com/10510013/diff/7001/gpu/ipc/command_buf...
File gpu/ipc/command_buffer_proxy.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/gpu/ipc/command_buf...
gpu/ipc/command_buffer_proxy.h:40: // Inserts a sync point, returning its ID.
This is handled on the IO thread,
nit: IO thread of the GPU process

[brettw, 2012-06-05 19:38:57]

LGTM (I assume you needed an owners review from me, this is rubberstamp based on
Al's review)

[piman, 2012-06-05 20:09:50]

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/gpu_channel.cc:24: #include "ipc/ipc_channel.h"
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> and ipc_channel_proxy.h

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/gpu_command_buffer_stub.h:10: 
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> #include <deque>

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/sync_point_manager.cc (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.cc:37: std::swap(list, it->second);
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> nit: list.swap(it->second)

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
File content/common/gpu/sync_point_manager.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:8: 
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> #include <vector>

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:39: typedef base::hash_map<uint32,
std::vector<base::Closure> > SyncPointMap;
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> Did you mean to use ClosureList here?

Yes, thanks! Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/common/gpu/...
content/common/gpu/sync_point_manager.h:47: uint32 next_sync_point_;
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/content_com...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/content/content_com...
content/content_common.gypi:204: 'common/gpu/sync_point_manager.h',
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> nit: not in alphabetic order

Done.

https://chromiumcodereview.appspot.com/10510013/diff/7001/gpu/ipc/command_buf...
File gpu/ipc/command_buffer_proxy.h (right):

https://chromiumcodereview.appspot.com/10510013/diff/7001/gpu/ipc/command_buf...
gpu/ipc/command_buffer_proxy.h:40: // Inserts a sync point, returning its ID.
This is handled on the IO thread,
On 2012/06/05 18:44:15, apatrick_chromium wrote:
> nit: IO thread of the GPU process

Done.

[apatrick_chromium, 2012-06-05 22:16:02]

lgtm

[commit-bot: I haz the power, 2012-06-05 22:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/10510013/15001

[commit-bot: I haz the power, 2012-06-05 23:35:26]

Change committed as 140653