gpu: Finish after EGLImage creation on Qualcomm

gpu: Finish after EGLImage creation on Qualcomm

Qualcomm devices (e.g., Nexus 4) run into texture corruption problems
if the same texture is uploaded to with both async and normal uploads.
Synchronize after EGLImage creation on the main thread as a work-around.

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

[Sami, 2013-02-28 19:08:09]

This works around the sync/async upload issue on Qualcomm. According to traces
we generally spend 1.5 ms blocking in the glFinish on the Nexus 4, so the perf
impact seems acceptable.

[reveman, 2013-02-28 20:04:56]

great! this seems ok to me but lets wait for eric to review if it's not a
problem with existing compositor code.

[epenner, 2013-03-07 21:51:19]

On 2013/02/28 20:04:56, David Reveman wrote:
> great! this seems ok to me but lets wait for eric to review if it's not a
> problem with existing compositor code.

Thanks for tracking this down! I ironically had a glFinish for Qualcomm before,
but I removed it in favor of image-preserved=true. I guess there is still a
problem though :( Fences (and blocking the upload thread on that fence) actually
worked for this on Nexus 4, but not on older Qualcomm devices :(

However, I'm now questioning if we can even do this correctly without a
fence/glFinish after *all* main thread uploads. If you have sync immediately
followed by an async, there seems to be a race even though the API implies the
sync happens first.

LGTM for now. But I'm debating if we should force textures to be sync or async,
with API enforcement.

[Sami, 2013-03-08 11:09:56]

On 2013/03/07 21:51:19, epenner wrote:
> Thanks for tracking this down! I ironically had a glFinish for Qualcomm
before,
> but I removed it in favor of image-preserved=true. I guess there is still a
> problem though :( Fences (and blocking the upload thread on that fence)
actually
> worked for this on Nexus 4, but not on older Qualcomm devices :(

Ah, too bad. Fences would be just the mechanism we need but I guess they're
generally still too buggy to be used.

> However, I'm now questioning if we can even do this correctly without a
> fence/glFinish after *all* main thread uploads. If you have sync immediately
> followed by an async, there seems to be a race even though the API implies the
> sync happens first.
> 
> LGTM for now. But I'm debating if we should force textures to be sync or
async,
> with API enforcement.

I think making a clearer separation between sync and async is a good idea. I
don't think there's a reason to mix them now that we can do waits on async
uploads, and given that the throughput for async uploads is better on Android we
should always use them for latency sensitive stuff.

[commit-bot: I haz the power, 2013-03-08 11:10:11]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sami, 2013-03-08 11:11:35]

+gman for owner's review, please.

[greggman, 2013-03-08 21:08:27]

lgtm

[commit-bot: I haz the power, 2013-03-11 11:09:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12390020/1

[commit-bot: I haz the power, 2013-03-11 11:09:59]

Failed to apply patch for ui/gl/async_pixel_transfer_delegate_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/gl/async_pixel_transfer_delegate_android.cc
  Hunk #1 FAILED at 209.
  Hunk #2 succeeded at 254 (offset -24 lines).
  Hunk #3 succeeded at 328 (offset -20 lines).
  Hunk #4 succeeded at 338 (offset -20 lines).
  Hunk #5 succeeded at 462 (offset -18 lines).
  Hunk #6 succeeded at 477 (offset -18 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
ui/gl/async_pixel_transfer_delegate_android.cc.rej

Patch:       ui/gl/async_pixel_transfer_delegate_android.cc
Index: ui/gl/async_pixel_transfer_delegate_android.cc
diff --git a/ui/gl/async_pixel_transfer_delegate_android.cc
b/ui/gl/async_pixel_transfer_delegate_android.cc
index
a594c9468e341848f22f4bd2c7a1c5326d2a7247..5b42d4a4640dda192ac7f82fa819a033f5c61537
100644
--- a/ui/gl/async_pixel_transfer_delegate_android.cc
+++ b/ui/gl/async_pixel_transfer_delegate_android.cc
@@ -209,12 +209,14 @@ class TransferStateInternal
  public:
   explicit TransferStateInternal(GLuint texture_id,
                                  bool wait_for_uploads,
+                                 bool wait_for_creation,
                                  bool use_image_preserved)
       : texture_id_(texture_id),
         thread_texture_id_(0),
         needs_late_bind_(false),
         egl_image_(EGL_NO_IMAGE_KHR),
         wait_for_uploads_(wait_for_uploads),
+        wait_for_creation_(wait_for_creation),
         use_image_preserved_(use_image_preserved) {
     static const AsyncTexImage2DParams zero_params = {0, 0, 0, 0, 0, 0, 0, 0};
     late_bind_define_params_ = zero_params;
@@ -278,8 +280,13 @@ class TransferStateInternal
   }
 
   void CreateEglImageOnMainThreadIfNeeded() {
-    if (egl_image_ == EGL_NO_IMAGE_KHR)
+    if (egl_image_ == EGL_NO_IMAGE_KHR) {
       CreateEglImage(texture_id_);
+      if (wait_for_creation_) {
+        TRACE_EVENT0("gpu", "glFinish creation");
+        glFinish();
+      }
+    }
   }
 
   void WaitForLastUpload() {
@@ -343,6 +350,7 @@ class TransferStateInternal
 
   // Customize when we block on fences (these are work-arounds).
   bool wait_for_uploads_;
+  bool wait_for_creation_;
   bool use_image_preserved_;
 };
 
@@ -352,9 +360,11 @@ class AsyncTransferStateAndroid : public
AsyncPixelTransferState {
  public:
   explicit AsyncTransferStateAndroid(GLuint texture_id,
                                      bool wait_for_uploads,
+                                     bool wait_for_creation,
                                      bool use_image_preserved)
       : internal_(new TransferStateInternal(texture_id,
                                             wait_for_uploads,
+                                            wait_for_creation,
                                             use_image_preserved)) {
   }
   virtual ~AsyncTransferStateAndroid() {}
@@ -472,6 +482,11 @@ AsyncPixelTransferState*
   // In practice, they are complete when the CPU glTexSubImage2D completes.
   bool wait_for_uploads = !is_imagination_;
 
+  // Qualcomm runs into texture corruption problems if the same texture is
+  // uploaded to with both async and normal uploads. Synchronize after EGLImage
+  // creation on the main thread as a work-around.
+  bool wait_for_creation = is_qualcomm_;
+
   // Qualcomm has a race when using image_preserved=FALSE,
   // which can result in black textures even after the first upload.
   // Since using FALSE is mainly for performance (to avoid layout changes),
@@ -482,6 +497,7 @@ AsyncPixelTransferState*
   return static_cast<AsyncPixelTransferState*>(
       new AsyncTransferStateAndroid(texture_id,
                                     wait_for_uploads,
+                                    wait_for_creation,
                                     use_image_preserved));
 }

[commit-bot: I haz the power, 2013-03-11 11:15:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12390020/10001

[commit-bot: I haz the power, 2013-03-11 13:44:58]

Change committed as 187288