Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(410)

Issue 12210129: gpu: Add the ability to wait on upload completion. (Closed)

Created:
7 years, 10 months ago by epenner
Modified:
7 years, 9 months ago
Reviewers:
greggman, nduca, reveman, Sami
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

gpu: Add the ability to wait on upload completion. Patch originally by Eric Penner <epenner@chromium.org>;. We currently have high latency if need to wait for uploads in the renderer client. If we can do a shallow client side wait it will be much better. This is effectively the same as doing synchronous uploads, but exposes less driver bug surface area then mixing uploads across threads. BUG=161828, 178634 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186190

Patch Set 1 #

Patch Set 2 : Patch. #

Patch Set 3 : Added entry point for waiting. Rebased and added test. #

Total comments: 1

Patch Set 4 : Allow waiting on a particular upload. #

Patch Set 5 : Formatting tweak. #

Total comments: 4

Patch Set 6 : Fix logging. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -36 lines) Patch
M content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M gpu/GLES2/gl2chromium_autogen.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M gpu/command_buffer/build_gles2_cmd_buffer.py View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_c_lib_autogen.h View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_cmd_helper_autogen.h View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_implementation.cc View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_implementation_autogen.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_interface_autogen.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_interface_stub_autogen.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_interface_stub_impl_autogen.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_trace_implementation_autogen.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_trace_implementation_impl_autogen.h View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M gpu/command_buffer/cmd_buffer_functions.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/common/gles2_cmd_format_autogen.h View 1 2 1 chunk +34 lines, -0 lines 0 comments Download
M gpu/command_buffer/common/gles2_cmd_format_test_autogen.h View 1 2 3 1 chunk +14 lines, -0 lines 0 comments Download
M gpu/command_buffer/common/gles2_cmd_ids_autogen.h View 1 2 1 chunk +5 lines, -4 lines 0 comments Download
M gpu/command_buffer/service/async_pixel_transfer_delegate_mock.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/service/gles2_cmd_decoder.cc View 1 2 3 4 1 chunk +20 lines, -0 lines 0 comments Download
M gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M gpu/command_buffer/service/gles2_cmd_decoder_unittest_3_autogen.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M ui/gl/async_pixel_transfer_delegate.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M ui/gl/async_pixel_transfer_delegate_android.cc View 1 2 3 9 chunks +40 lines, -32 lines 0 comments Download
M ui/gl/async_pixel_transfer_delegate_stub.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M ui/gl/async_pixel_transfer_delegate_stub.cc View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
epenner
Ptal. I whipped this up because it appears we hit a driver bug with doing ...
7 years, 10 months ago (2013-02-12 02:37:28 UTC) #1
nduca
How does this approach compare to advertising a GL extension that we dont support mixed ...
7 years, 10 months ago (2013-02-12 03:36:17 UTC) #2
epenner
On 2013/02/12 03:36:17, nduca wrote: > How does this approach compare to advertising a GL ...
7 years, 10 months ago (2013-02-12 04:18:12 UTC) #3
nduca
But this solution makes the gpu wait on the async upload. Thats probably a perf ...
7 years, 10 months ago (2013-02-12 04:22:02 UTC) #4
nduca
s/makes the gpu wait/makes the sync uploads wait on the async uploads/
7 years, 10 months ago (2013-02-12 04:22:24 UTC) #5
epenner
I should note that maybe we can just fix the Qualcomm issue with a bit ...
7 years, 10 months ago (2013-02-12 04:22:39 UTC) #6
epenner
On 2013/02/12 04:22:39, epenner wrote: > I should note that maybe we can just fix ...
7 years, 10 months ago (2013-02-12 04:28:50 UTC) #7
Sami
Thanks for this Eric. Sorry I wasn't able to try it out yet today. I ...
7 years, 10 months ago (2013-02-12 20:11:50 UTC) #8
Sami
Oh, and it occurs to me that we'll need to support mixing sync/async uploads for ...
7 years, 10 months ago (2013-02-12 20:14:05 UTC) #9
epenner
Yes Sami that was my thinking. Something like: - If we aren't blocked on uploads, ...
7 years, 10 months ago (2013-02-12 20:49:07 UTC) #10
Sami
As another data point, disabling server-side async uploads fixes the bug on Nexus 4, so ...
7 years, 10 months ago (2013-02-13 12:38:49 UTC) #11
epenner
> How about a new entrypoint just for inserting the server-side wait? Something based on ...
7 years, 10 months ago (2013-02-13 19:12:44 UTC) #12
Sami
On 2013/02/13 19:12:44, epenner wrote: > That would be great. But keep in mind we ...
7 years, 10 months ago (2013-02-13 19:17:11 UTC) #13
Sami
Eric, I hijacked your patch and added a glWaitAsyncTexImage2D() entry point that the resource provider ...
7 years, 9 months ago (2013-02-27 16:44:40 UTC) #14
reveman
How about we make WaitAsyncTexImage2DCHROMIUM(target) only wait for the texture currently bound to target to ...
7 years, 9 months ago (2013-02-28 02:12:00 UTC) #15
reveman
This looks good. Sami, can you add the texture specific addition and then I think ...
7 years, 9 months ago (2013-03-04 06:10:57 UTC) #16
Sami
This version makes it possible to wait on a particular upload. The Android delegate implements ...
7 years, 9 months ago (2013-03-04 18:57:47 UTC) #17
reveman
On 2013/03/04 18:57:47, Sami wrote: > This version makes it possible to wait on a ...
7 years, 9 months ago (2013-03-04 19:32:58 UTC) #18
Sami
On 2013/03/04 19:32:58, David Reveman wrote: > I'm afraid that using a WaitableEvent for completion ...
7 years, 9 months ago (2013-03-04 19:53:49 UTC) #19
reveman
On 2013/03/04 19:53:49, Sami wrote: > On 2013/03/04 19:32:58, David Reveman wrote: > > I'm ...
7 years, 9 months ago (2013-03-04 20:35:56 UTC) #20
reveman
Gregg, this is a minor addition to the CHROMIUM_async_pixel_transfers extension that is needed to improve ...
7 years, 9 months ago (2013-03-04 20:40:32 UTC) #21
greggman
lgtm https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client/gles2_implementation.cc File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client/gles2_implementation.cc#newcode3444 gpu/command_buffer/client/gles2_implementation.cc:3444: GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glWaitAsyncTexImage2DCHROMIUM()"); log the ...
7 years, 9 months ago (2013-03-04 21:25:52 UTC) #22
Sami
https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client/gles2_implementation.cc File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client/gles2_implementation.cc#newcode3444 gpu/command_buffer/client/gles2_implementation.cc:3444: GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glWaitAsyncTexImage2DCHROMIUM()"); On 2013/03/04 21:25:53, ...
7 years, 9 months ago (2013-03-05 12:25:26 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12210129/25001
7 years, 9 months ago (2013-03-05 12:33:42 UTC) #24
commit-bot: I haz the power
Change committed as 186190
7 years, 9 months ago (2013-03-05 17:02:18 UTC) #25
epenner
On 2013/03/05 17:02:18, I haz the power (commit-bot) wrote: > Change committed as 186190 LGTM ...
7 years, 9 months ago (2013-03-08 02:35:50 UTC) #26
reveman
On 2013/03/08 02:35:50, epenner wrote: > On 2013/03/05 17:02:18, I haz the power (commit-bot) wrote: ...
7 years, 9 months ago (2013-03-08 03:42:01 UTC) #27
Sami
On 2013/03/08 03:42:01, David Reveman wrote: > we might want to stop using a message ...
7 years, 9 months ago (2013-03-08 11:26:58 UTC) #28
epenner
7 years, 9 months ago (2013-03-08 19:45:27 UTC) #29
Message was sent while issue was closed.
Regarding locks, maybe I'm going overboard on that. My thinking was ideally all
locks would go away eventually (including the message loop ones), but it's
probably not worth worrying about.

Okay I understand the idea for the waits now. Thanks for the explanation! It
seems a bit tricky to get it right in the threaded version (non waited-on
textures could slip through between the waits on the main thread).

Could we perhaps provide a list of textures to block on instead?

Also, it seems like its getting good enough to avoid a big re-architecture (to
reduce latency of uploads by avoiding the GL pipeline). Wdyt?

Powered by Google App Engine
This is Rietveld 408576698