GPU: Flush in glBind* to avoid being executed after future glDelete*

GPU: Flush in glBind* to avoid being executed after future glDelete*

In Pepper, glBind*() may generate resource in GPU process, so we have to
flush command buffer to make sure the Bind* command is executed by GPU before
any future Delete* commands.

BUG=437432
Committed: https://crrev.com/68b55cd3b2f4bb22b5ad4beb3a543d71269fde4a
Cr-Commit-Position: refs/heads/master@{#307101}

[Peng, 2014-11-28 21:04:58]

penghuang@chromium.org changed reviewers:
+ bajones@chromium.org, piman@chromium.org

[Peng, 2014-11-28 21:05:17]

On 2014/11/28 21:04:58, Peng wrote:
> mailto:penghuang@chromium.org changed reviewers:
> + mailto:bajones@chromium.org, mailto:piman@chromium.org

PTAL, Thanks.

[Nike, 2014-12-01 12:09:16]

olonho@chromium.org changed reviewers:
+ olonho@chromium.org

[Nike, 2014-12-01 12:09:17]

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:2329: // TODO(penghuang): Get
ride of the Flush().
Get rid

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:2330: if (changed &&
share_group_->bind_generates_resource())
Would it be possible to create common FlushIfChanged() with above comment?

[Peng, 2014-12-01 14:38:12]

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:2329: // TODO(penghuang): Get
ride of the Flush().
On 2014/12/01 12:09:16, Nike wrote:
> Get rid

Done

https://codereview.chromium.org/763383002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:2330: if (changed &&
share_group_->bind_generates_resource())
On 2014/12/01 12:09:16, Nike wrote:
> Would it be possible to create common FlushIfChanged() with above comment?

The code for deciding if changed is different for BindTexture(), BindBuffer(),
etc. So the only common code is:

if (share_group_->bind_generates_resource())
  Flush().

I think it is not necessary to make a function for it.

[bajones, 2014-12-01 21:28:27]

lgtm

[piman, 2014-12-01 21:41:53]

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:2456:
helper_->CommandBufferHelper::Flush();
This is racy, it needs to be done under the lock in IdHandler. Otherwise a
DeleteTexture on another thread could happen after the MarkAsUsedForBind
(therefore clearing the "in use" bit for the id), but before the Flush here,
therefore causing a mismatch between client and service.

[Peng, 2014-12-02 19:32:36]

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:2456:
helper_->CommandBufferHelper::Flush();
On 2014/12/01 21:41:53, piman (Very slow to review) wrote:
> This is racy, it needs to be done under the lock in IdHandler. Otherwise a
> DeleteTexture on another thread could happen after the MarkAsUsedForBind
> (therefore clearing the "in use" bit for the id), but before the Flush here,
> therefore causing a mismatch between client and service.

For your mentioned case, it should be fine, unless other threads call
DeleteTextures() and Flush() together, and DeleteTextures() & Flush() are both
happen between MarkAsUsedForBind() & Flush(). It is unlikely, almost impossible.
So for short term, this change should be OK. At least, the change does not make
it worse.

I also looked code of GenTextures, GenBuffers, etc. They have the same issue.
They use Flush() without holding the lock in the id allocator too. 

For long term, I think a solution could be to add the 'sync point' concept for
context shared group. All GL commands need read the status of a shared group
will attach a 'sync point' with it. GPU process will not execute those commands
until the sync point being passed. For GL commands which modify status, a sync
point will be attached with it too (The sync points are generated by the shared
group in client side without any IPC). When the GPU process completes the
command, the GPU process will update the shared group sync point with the
attached sync point, and executes any pending commands which are waiting for
that sync point. WDYT?

[piman, 2014-12-02 21:24:52]

On Tue, Dec 2, 2014 at 11:32 AM, <penghuang@chromium.org> wrote:

>
> https://codereview.chromium.org/763383002/diff/20001/gpu/
> command_buffer/client/gles2_implementation.cc
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
> https://codereview.chromium.org/763383002/diff/20001/gpu/
> command_buffer/client/gles2_implementation.cc#newcode2456
> gpu/command_buffer/client/gles2_implementation.cc:2456:
> helper_->CommandBufferHelper::Flush();
> On 2014/12/01 21:41:53, piman (Very slow to review) wrote:
>
>> This is racy, it needs to be done under the lock in IdHandler.
>>
> Otherwise a
>
>> DeleteTexture on another thread could happen after the
>>
> MarkAsUsedForBind
>
>> (therefore clearing the "in use" bit for the id), but before the Flush
>>
> here,
>
>> therefore causing a mismatch between client and service.
>>
>
> For your mentioned case, it should be fine, unless other threads call
> DeleteTextures() and Flush() together, and DeleteTextures() & Flush()
> are both happen between MarkAsUsedForBind() & Flush(). It is unlikely,
> almost impossible. So for short term, this change should be OK. At

least, the change does not make it worse.
>

What's wrong with my suggested change?
I don't like half-hearted fixes. There's no such thing as "unlikely" when
you multiply by several hundreds of millions of users.


>
> I also looked code of GenTextures, GenBuffers, etc. They have the same
> issue. They use Flush() without holding the lock in the id allocator
> too.
>

Right. This is a problem too.


>
> For long term, I think a solution could be to add the 'sync point'
> concept for context shared group. All GL commands need read the status
> of a shared group will attach a 'sync point' with it. GPU process will
> not execute those commands until the sync point being passed. For GL
> commands which modify status, a sync point will be attached with it too
> (The sync points are generated by the shared group in client side
> without any IPC). When the GPU process completes the command, the GPU
> process will update the shared group sync point with the attached sync
> point, and executes any pending commands which are waiting for that sync
> point. WDYT?
>

I don't understand the need for this. Inserting a sync point implies a
Flush. Share groups don't exist across channels, this has nothing to do
with the GPU process ordering - GPU commands coming from a single process
are all ordered based on the Flush ordering.
This is purely a race on the client side, I don't see how a service-side
concept (sync point) would do anything.


>
> https://codereview.chromium.org/763383002/
>

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

[Peng, 2014-12-04 17:45:01]

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/763383002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:2456:
helper_->CommandBufferHelper::Flush();
On 2014/12/01 21:41:53, piman (Very slow to review) wrote:
> This is racy, it needs to be done under the lock in IdHandler. Otherwise a
> DeleteTexture on another thread could happen after the MarkAsUsedForBind
> (therefore clearing the "in use" bit for the id), but before the Flush here,
> therefore causing a mismatch between client and service.

Done. Flush() the command buffer under the lock in IdHandler.

[piman, 2014-12-05 20:31:13]

https://codereview.chromium.org/763383002/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/763383002/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/share_group.cc:157: base::AutoLock auto_lock(lock_);
You don't need to take the lock in the release path. This was optimized in a
previous patch, because it was "hot" on compositor workloads.

[Peng, 2014-12-05 21:03:55]

Patchset #4 (id:60001) has been deleted

[Peng, 2014-12-05 21:04:39]

https://codereview.chromium.org/763383002/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/763383002/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/share_group.cc:157: base::AutoLock auto_lock(lock_);
On 2014/12/05 20:31:13, piman (Very slow to review) wrote:
> You don't need to take the lock in the release path. This was optimized in a
> previous patch, because it was "hot" on compositor workloads.

Done.

[piman, 2014-12-05 21:14:53]

lgtm

[Peng, 2014-12-05 21:20:14]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2014-12-05 21:20:35]

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

[commit-bot: I haz the power, 2014-12-05 23:03:24]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-12-05 23:04:57]

Patchset 4 (id:??) landed as
https://crrev.com/68b55cd3b2f4bb22b5ad4beb3a543d71269fde4a
Cr-Commit-Position: refs/heads/master@{#307101}