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

Issue 2752393002: gpu: Add SequenceId for identifying sync point sequences. (Closed)

Created:
3 years, 9 months ago by sunnyps
Modified:
3 years, 9 months ago
Reviewers:
DaleCurtis, piman
CC:
chromium-reviews, feature-media-reviews_chromium.org, fuzzing_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

gpu: Add SequenceId for identifying sync point sequences. This adds SequenceId which is a global identifier for sync point sequences assigned by SyncPointManager. The idea is that every new task queue created by the gpu scheduler will have a new SyncPointOrderData instance and hence a SequenceId. This SequenceId will be passed around to sync point clients and whatever needs to post tasks to the scheduler e.g. message filter will have a route id to sequence id map. It is possible to look up the sequence id, and hence task queue, for a sync token. This will be used by gpu scheduler for priority inheritance. BUG=514813 R=piman CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2752393002 Cr-Commit-Position: refs/heads/master@{#458541} Committed: https://chromium.googlesource.com/chromium/src/+/c8cc93e27f2d38c39e8af5f79b8e69685348263e

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : fix android compile #

Patch Set 4 : fix failing tests #

Total comments: 8

Patch Set 5 : piman's review #

Total comments: 2

Patch Set 6 : piman's review 2 + fix tests #

Patch Set 7 : fix lock order inversion #

Total comments: 2

Patch Set 8 : piman's review 3 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+581 lines, -644 lines) Patch
M gpu/command_buffer/service/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
A gpu/command_buffer/service/sequence_id.h View 1 chunk +17 lines, -0 lines 0 comments Download
M gpu/command_buffer/service/sync_point_manager.h View 1 2 3 4 9 chunks +108 lines, -74 lines 0 comments Download
M gpu/command_buffer/service/sync_point_manager.cc View 1 2 3 4 5 6 7 12 chunks +169 lines, -104 lines 0 comments Download
M gpu/command_buffer/service/sync_point_manager_unittest.cc View 1 2 3 4 5 19 chunks +57 lines, -52 lines 0 comments Download
M gpu/command_buffer/tests/fuzzer_main.cc View 6 chunks +13 lines, -10 lines 0 comments Download
M gpu/command_buffer/tests/gl_manager.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M gpu/command_buffer/tests/gl_manager.cc View 1 7 chunks +16 lines, -14 lines 0 comments Download
M gpu/ipc/in_process_command_buffer.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M gpu/ipc/in_process_command_buffer.cc View 1 9 chunks +18 lines, -15 lines 0 comments Download
M gpu/ipc/service/gpu_channel.h View 1 2 14 chunks +23 lines, -70 lines 0 comments Download
M gpu/ipc/service/gpu_channel.cc View 1 2 3 4 26 chunks +78 lines, -202 lines 0 comments Download
M gpu/ipc/service/gpu_channel_manager.h View 1 chunk +0 lines, -8 lines 0 comments Download
M gpu/ipc/service/gpu_channel_manager.cc View 1 chunk +0 lines, -18 lines 0 comments Download
M gpu/ipc/service/gpu_command_buffer_stub.h View 1 2 3 4 7 chunks +17 lines, -8 lines 0 comments Download
M gpu/ipc/service/gpu_command_buffer_stub.cc View 1 2 3 4 23 chunks +57 lines, -62 lines 0 comments Download
M gpu/ipc/service/stream_texture_android.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/gpu/ipc/service/gpu_video_decode_accelerator.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/gpu/ipc/service/gpu_video_encode_accelerator.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 54 (37 generated)
sunnyps
PTAL This adds SequenceId which is a global identifier for sync point sequences assigned by ...
3 years, 9 months ago (2017-03-17 02:46:06 UTC) #13
piman
LGTM overall, see a few things. https://codereview.chromium.org/2752393002/diff/60001/gpu/command_buffer/service/sync_point_manager.cc File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/2752393002/diff/60001/gpu/command_buffer/service/sync_point_manager.cc#newcode205 gpu/command_buffer/service/sync_point_manager.cc:205: command_buffer_id_); nit: let's ...
3 years, 9 months ago (2017-03-17 19:12:38 UTC) #16
sunnyps
Thanks! https://codereview.chromium.org/2752393002/diff/60001/gpu/command_buffer/service/sync_point_manager.cc File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/2752393002/diff/60001/gpu/command_buffer/service/sync_point_manager.cc#newcode205 gpu/command_buffer/service/sync_point_manager.cc:205: command_buffer_id_); On 2017/03/17 19:12:37, piman wrote: > nit: ...
3 years, 9 months ago (2017-03-17 21:48:23 UTC) #19
piman
LGTM+nit https://codereview.chromium.org/2752393002/diff/80001/gpu/command_buffer/service/sync_point_manager.cc File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/2752393002/diff/80001/gpu/command_buffer/service/sync_point_manager.cc#newcode207 gpu/command_buffer/service/sync_point_manager.cc:207: DCHECK(sync_point_manager_); // not destroyed nit: move to above ...
3 years, 9 months ago (2017-03-17 21:57:31 UTC) #20
sunnyps
I decided not to add new wrapper classes for SyncPointClientState and SyncPointOrderData. I'll land this ...
3 years, 9 months ago (2017-03-18 02:13:31 UTC) #25
sunnyps
dalecurtis@ please rs media/gpu/ipc/service changes. Thanks!
3 years, 9 months ago (2017-03-18 02:15:34 UTC) #27
DaleCurtis
lgtm
3 years, 9 months ago (2017-03-20 17:59:21 UTC) #30
sunnyps
piman@ ptal fixed a lock order inversion that tsan found
3 years, 9 months ago (2017-03-20 20:18:44 UTC) #33
piman
https://codereview.chromium.org/2752393002/diff/120001/gpu/command_buffer/service/sync_point_manager.cc File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/2752393002/diff/120001/gpu/command_buffer/service/sync_point_manager.cc#newcode399 gpu/command_buffer/service/sync_point_manager.cc:399: // Copy order data map to prevent deadlock. That ...
3 years, 9 months ago (2017-03-20 21:30:23 UTC) #34
sunnyps
ptal https://codereview.chromium.org/2752393002/diff/120001/gpu/command_buffer/service/sync_point_manager.cc File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/2752393002/diff/120001/gpu/command_buffer/service/sync_point_manager.cc#newcode399 gpu/command_buffer/service/sync_point_manager.cc:399: // Copy order data map to prevent deadlock. ...
3 years, 9 months ago (2017-03-20 21:40:34 UTC) #36
piman
lgtm
3 years, 9 months ago (2017-03-20 23:00:49 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752393002/140001
3 years, 9 months ago (2017-03-20 23:04:09 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/412525)
3 years, 9 months ago (2017-03-21 00:44:46 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752393002/140001
3 years, 9 months ago (2017-03-21 00:51:09 UTC) #47
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/254045)
3 years, 9 months ago (2017-03-21 02:48:56 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752393002/140001
3 years, 9 months ago (2017-03-21 19:59:25 UTC) #51
commit-bot: I haz the power
3 years, 9 months ago (2017-03-21 20:53:44 UTC) #54
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c8cc93e27f2d38c39e8af5f79b8e...

Powered by Google App Engine
This is Rietveld 408576698