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

gpu/command_buffer/client/gles2_implementation.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // A class to emulate GLES2 over command buffers.
 
 #include "gpu/command_buffer/client/gles2_implementation.h"
 
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
@@ skipping 2303 matching lines @@
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
       bound_pixel_unpack_transfer_buffer_id_ = buffer_id;
       break;
     default:
       changed = true;
       break;
   }
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used even though it's marked it as used here.
   GetIdHandler(id_namespaces::kBuffers)->MarkAsUsedForBind(buffer_id);
+
+  // In Pepper, BindBuffer() may need generate resource in GPU process, so we
+  // have to flush command buffer to make the BindBuffer() being executed in
+  // GPU process before any DeleteBuffer() which are issused in future from
+  // other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
 bool GLES2Implementation::BindFramebufferHelper(
     GLenum target, GLuint framebuffer) {
   // TODO(gman): See note #1 above.
   bool changed = false;
   switch (target) {
     case GL_FRAMEBUFFER:
       if (bound_framebuffer_ != framebuffer ||
@@ skipping 20 matching lines @@
       }
       if (bound_framebuffer_ != framebuffer) {
         bound_framebuffer_ = framebuffer;
         changed = true;
       }
       break;
     default:
       SetGLErrorInvalidEnum("glBindFramebuffer", target, "target");
       return false;
   }
+
   GetIdHandler(id_namespaces::kFramebuffers)->MarkAsUsedForBind(framebuffer);
+
+  // In Pepper, BindFramebuffer() may need generate resource in GPU process,
+  // so we have to flush command buffer to make the BindFramebuffer() being
+  // executed in GPU process before any DeleteFramebuffer() which are issused in
+  // future from other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
 bool GLES2Implementation::BindRenderbufferHelper(
     GLenum target, GLuint renderbuffer) {
   // TODO(gman): See note #1 above.
   bool changed = false;
   switch (target) {
     case GL_RENDERBUFFER:
       if (bound_renderbuffer_ != renderbuffer) {
         bound_renderbuffer_ = renderbuffer;
         changed = true;
       }
       break;
     default:
       changed = true;
       break;
   }
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used even though it's marked it as used here.
   GetIdHandler(id_namespaces::kRenderbuffers)->MarkAsUsedForBind(renderbuffer);
+
+  // In Pepper, BindRenderbuffer() may need generate resource in GPU process,
+  // so we have to flush command buffer to make the BindRenderbuffer() being
+  // executed in GPU process before any DeleteRenderbuffer() which are issused
+  // in future from other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
 bool GLES2Implementation::BindTextureHelper(GLenum target, GLuint texture) {
   // TODO(gman): See note #1 above.
   // TODO(gman): Change this to false once we figure out why it's failing
   //     on daisy.
   bool changed = true;
   TextureUnit& unit = texture_units_[active_texture_unit_];
   switch (target) {
@@ skipping 15 matching lines @@
         changed = true;
       }
       break;
     default:
       changed = true;
       break;
   }
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used. even though it's marked it as used here.
   GetIdHandler(id_namespaces::kTextures)->MarkAsUsedForBind(texture);
+
+  // In Pepper, BindTexture() may need generate resource in GPU process, so we
+  // have to flush command buffer to make the BindTexture() being executed in
+  // GPU process before any DeleteTexture() which are issused in future from
+  // other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();

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

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]

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?

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

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

+
   return changed;
 }
 
 bool GLES2Implementation::BindVertexArrayOESHelper(GLuint array) {
   // TODO(gman): See note #1 above.
   bool changed = false;
   if (!vertex_array_object_manager_->BindVertexArray(array, &changed)) {
     SetGLError(
         GL_INVALID_OPERATION, "glBindVertexArrayOES",
         "id was not generated with glGenVertexArrayOES");
@@ skipping 1474 matching lines @@
   return true;
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "gpu/command_buffer/client/gles2_implementation_impl_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu