Early wait on texture resource sync points in gl_renderer.

Early wait on texture resource sync points in gl_renderer.

Previously texture resource sync points were waited on
a just-in-time basis, just before drawing with the texture.

This change moves all sync point waits ahead of drawing a
frame, to ensure that we don't switch GLContexts in the
middle of drawing.

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

[vmiura, 2014-08-13 00:08:39]

I need some pointers for doing this patch properly.

- Does this look like the right approach?
- In DrawRenderPass we may skip quads if using_scissor_as_optimization is
enabled.  Should I skip locking those tile resources as well?

Thanks!

[enne (OOO), 2014-08-13 00:20:54]

I feel like I'm missing a lot of context (pun not intended) here.  Can you help
me understand what you mean by context switching here?  Where is it happening?

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:483: ResourceProvider::ScopedReadLockGL
lock(resource_provider_,
I'm not sure that I follow what this is trying to do.  A scoped read lock will
lock and unlock right here.  Is that intentional?

[vmiura, 2014-08-13 01:01:32]

On 2014/08/13 00:20:54, enne wrote:
> I feel like I'm missing a lot of context (pun not intended) here.  Can you
help
> me understand what you mean by context switching here?  Where is it happening?

Sure.

We have the Renderer process issuing commands on it's GL context (gl1) to
rasterize tiles (via Skia).
We have the Browser process issuing commands on it's GL context (gl2) for
compositing via (via gl_renderer).

gl1 commands:
 for each tile:
   render tile contents.
   post mailbox for tile.
 add InsertSyncPointCHROMIUM.
 

gl2 commands:
 clear backbuffer
 for each render pass:
   for each quad:
     lock texture resource, adds WaitSyncPointCHROMIUM matching above
InsertSyncPointCHROMIUM.
     draw quad.

On the GPU process side gl1 & gl2 contexts are processed
first-come-first-served.  In the best case, they're processed in order [gl1,
gl2], but frequently gl2 will go first.

E.g. process order:
  [gl2] clear backbuffer
  [gl2] draw solid quad
  [gl2] draw solid quad
  [gl2] WaitSyncPointCHROMIUM -> forces a wait until gl1 context processed
  [gl1] render tile
  [gl1] post mailbox
  [gl1] render tile
  [gl1] post mailbox
  [gl1] InsertSyncPointCHROMIUM -> allows gl2 to proceed
  [gl2] draw tile quad
  [gl2] draw tile quad
  ...

It's very costly on the GPU to start drawing to an FBO, switch to something
else, and then resume.

  [gl2] clear backbuffer
  [gl2] draw solid quad
  [gl2] draw solid quad
  ... do something on gl1 that changes FBOs ... (super bad news)
  [gl2] draw tile quad 
  [gl2] draw tile quad

There is an example in the bug:
https://code.google.com/p/chromium/issues/detail?id=394547 which is worse still.
In that example we have both contexts interrupting each-other: [gl1, gl2, gl1,
gl2]

The CL forces the WaitSyncPointCHROMIUM to be inserted on gl2 before the browser
begins any drawing, i.e.:

  [gl2] WaitSyncPointCHROMIUM -> forces a wait until gl1 context processed
  [gl1] render tile
  [gl1] post mailbox
  [gl1] render tile
  [gl1] post mailbox
  [gl1] InsertSyncPointCHROMIUM -> allows gl2 to proceed
  [gl2] clear backbuffer
  [gl2] draw solid quad
  [gl2] draw solid quad
  [gl2] draw tile quad
  [gl2] draw tile quad

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:483: ResourceProvider::ScopedReadLockGL
lock(resource_provider_,
On 2014/08/13 00:20:54, enne wrote:
> I'm not sure that I follow what this is trying to do.  A scoped read lock will
> lock and unlock right here.  Is that intentional?

Yes it's intentional, in this case taking the lock is taken temporarily to force
any pending mailbox sync point to be checked.

Here:

ResourceProvider::LockForRead(ResourceId id) {
...
    DCHECK(gl);
    if (resource->mailbox.sync_point()) {
      GLC(gl, gl->WaitSyncPointCHROMIUM(resource->mailbox.sync_point()));
      resource->mailbox.set_sync_point(0);
    }
}

[danakj, 2014-08-13 13:52:13]

https://codereview.chromium.org/411643002/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:344: ++it) {
You can IterateResources here to get each of the resources on each quad. Then
you don't need a switch on the quad material type.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/draw_quad...

Once you have all the resources, instead of doing a Lock/Unlock, how about we
just add an API to wait on any sync points. Something like:

ResourceProvider::WaitSyncPointIfNeeded(ResourceId id) {
  Resource* resource = GetResource(id);
  DCHECK_EQ(resource->exported_count, 0);
  DCHECK(resource->allocated);
  if (!resource is gl type with gl_id) return;
  if (!syncpoint) return;
  DCHECK(resource->mailbox.IsValid);
  WaitSyncPoint(syncpoint);
}

Then we don't need a virtual method and the GL/Software renderers don't need to
be involved.

Can we then DCHECK that there's no sync point to wait on in LockForRead instead
of waiting there too?

[vmiura, 2014-08-13 22:50:39]

Applied suggestions.  PTAL.

https://codereview.chromium.org/411643002/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:344: ++it) {
On 2014/08/13 13:52:12, danakj wrote:
> You can IterateResources here to get each of the resources on each quad. Then
> you don't need a switch on the quad material type.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/draw_quad...
> 
> Once you have all the resources, instead of doing a Lock/Unlock, how about we
> just add an API to wait on any sync points. Something like:
> 
> ResourceProvider::WaitSyncPointIfNeeded(ResourceId id) {
>   Resource* resource = GetResource(id);
>   DCHECK_EQ(resource->exported_count, 0);
>   DCHECK(resource->allocated);
>   if (!resource is gl type with gl_id) return;
>   if (!syncpoint) return;
>   DCHECK(resource->mailbox.IsValid);
>   WaitSyncPoint(syncpoint);
> }
> 
> Then we don't need a virtual method and the GL/Software renderers don't need
to
> be involved.
> 
> Can we then DCHECK that there's no sync point to wait on in LockForRead
instead
> of waiting there too?

Done.

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:483: ResourceProvider::ScopedReadLockGL
lock(resource_provider_,
No longer applicable.

[danakj, 2014-08-13 23:05:19]

This looks good. How can we test it? We should add tests for the
ResourceProvider API, and also for the renderer.

https://codereview.chromium.org/411643002/diff/20001/cc/output/direct_rendere...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/20001/cc/output/direct_rendere...
cc/output/direct_renderer.cc:237: // crbug.com/394547
git blame will point to the bug, i think you can drop this from the comment

https://codereview.chromium.org/411643002/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/411643002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2254: // Insert WaitSyncPointCHROMIUM on gl
context, and clear sync_point from
this comment says "what", not "why". i think it can be removed

[vmiura, 2014-08-15 21:20:39]

Resolved comments, and added unit tests.
I've moved the WaitOnResourceSyncPoints from direct_renderer to gl_renderer, as
I assume it only applies to gl_rendering.

PTAL, thanks.

https://codereview.chromium.org/411643002/diff/20001/cc/output/direct_rendere...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/411643002/diff/20001/cc/output/direct_rendere...
cc/output/direct_renderer.cc:237: // crbug.com/394547
On 2014/08/13 23:05:19, danakj wrote:
> git blame will point to the bug, i think you can drop this from the comment

Done.

https://codereview.chromium.org/411643002/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/411643002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2254: // Insert WaitSyncPointCHROMIUM on gl
context, and clear sync_point from
On 2014/08/13 23:05:19, danakj wrote:
> this comment says "what", not "why". i think it can be removed

Done.

[danakj, 2014-08-15 21:32:34]

LGTM

https://codereview.chromium.org/411643002/diff/80001/cc/output/gl_renderer_un...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/411643002/diff/80001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:805: EXPECT_CALL(*context,
waitSyncPoint(TestRenderPass::kTestSyncPoint))
Can you add a comment here like
// The sync points for all quads are waited on first. This sync point is for a
texture quad drawn later in the frame.

https://codereview.chromium.org/411643002/diff/80001/cc/test/render_pass_test...
File cc/test/render_pass_test_common.h (right):

https://codereview.chromium.org/411643002/diff/80001/cc/test/render_pass_test...
cc/test/render_pass_test_common.h:22: static const unsigned int kTestSyncPoint =
30;
can you add a bit more context to this name instead of just "Test"? something
like kSyncPointForMailboxTextureQuad?

[vmiura, 2014-08-15 21:55:00]

https://codereview.chromium.org/411643002/diff/80001/cc/output/gl_renderer_un...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/411643002/diff/80001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:805: EXPECT_CALL(*context,
waitSyncPoint(TestRenderPass::kTestSyncPoint))
On 2014/08/15 21:32:34, danakj wrote:
> Can you add a comment here like
> // The sync points for all quads are waited on first. This sync point is for a
> texture quad drawn later in the frame.

Done.

https://codereview.chromium.org/411643002/diff/80001/cc/test/render_pass_test...
File cc/test/render_pass_test_common.h (right):

https://codereview.chromium.org/411643002/diff/80001/cc/test/render_pass_test...
cc/test/render_pass_test_common.h:22: static const unsigned int kTestSyncPoint =
30;
On 2014/08/15 21:32:34, danakj wrote:
> can you add a bit more context to this name instead of just "Test"? something
> like kSyncPointForMailboxTextureQuad?

Done.

[danakj, 2014-08-15 22:25:03]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:26:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmiura@chromium.org/411643002/100001

[vmiura, 2014-08-15 22:46:56]

Sorry, missed some needed calls to WaitSyncPointIfNeeded in the unittests.

[vmiura, 2014-08-15 22:58:07]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:59:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmiura@chromium.org/411643002/120001

[commit-bot: I haz the power, 2014-08-16 06:19:52]

Committed patchset #7 (120001) as 290109