diff options
Diffstat (limited to 'chromium/gpu')
4 files changed, 175 insertions, 61 deletions
diff --git a/chromium/gpu/command_buffer/service/sync_point_manager.cc b/chromium/gpu/command_buffer/service/sync_point_manager.cc index a3c4bf152e0..7fdf99e7e13 100644 --- a/chromium/gpu/command_buffer/service/sync_point_manager.cc +++ b/chromium/gpu/command_buffer/service/sync_point_manager.cc @@ -72,8 +72,9 @@ void SyncPointOrderData::Destroy() { uint32_t SyncPointOrderData::GenerateUnprocessedOrderNumber() { base::AutoLock auto_lock(lock_); DCHECK(!destroyed_); - unprocessed_order_num_ = sync_point_manager_->GenerateOrderNumber(); - return unprocessed_order_num_; + last_unprocessed_order_num_ = sync_point_manager_->GenerateOrderNumber(); + unprocessed_order_nums_.push(last_unprocessed_order_num_); + return last_unprocessed_order_num_; } void SyncPointOrderData::BeginProcessingOrderNumber(uint32_t order_num) { @@ -85,29 +86,6 @@ void SyncPointOrderData::BeginProcessingOrderNumber(uint32_t order_num) { DCHECK_LE(order_num, unprocessed_order_num()); current_order_num_ = order_num; paused_ = false; - - // Catch invalid waits which were waiting on fence syncs that do not exist. - // When we begin processing an order number, we should release any fence - // syncs which were enqueued but the order number never existed. - // Release without the lock to avoid possible deadlocks. - std::vector<OrderFence> ensure_releases; - { - base::AutoLock auto_lock(lock_); - while (!order_fence_queue_.empty()) { - const OrderFence& order_fence = order_fence_queue_.top(); - if (order_fence_queue_.top().order_num < order_num) { - ensure_releases.push_back(order_fence); - order_fence_queue_.pop(); - continue; - } - break; - } - } - - for (OrderFence& order_fence : ensure_releases) { - order_fence.client_state->EnsureWaitReleased(order_fence.fence_release, - order_fence.release_callback); - } } void SyncPointOrderData::PauseProcessingOrderNumber(uint32_t order_num) { @@ -132,9 +110,20 @@ void SyncPointOrderData::FinishProcessingOrderNumber(uint32_t order_num) { DCHECK_GT(order_num, processed_order_num_); processed_order_num_ = order_num; + DCHECK(!unprocessed_order_nums_.empty()); + DCHECK_EQ(order_num, unprocessed_order_nums_.front()); + unprocessed_order_nums_.pop(); + + uint32_t next_order_num = 0; + if (!unprocessed_order_nums_.empty()) + next_order_num = unprocessed_order_nums_.front(); + while (!order_fence_queue_.empty()) { const OrderFence& order_fence = order_fence_queue_.top(); - if (order_fence_queue_.top().order_num <= order_num) { + // It's possible for the fence's order number to equal next order number. + // This happens when the wait was enqueued with an order number greater + // than the last unprocessed order number. So don't release the fence yet. + if (!next_order_num || order_fence.order_num < next_order_num) { ensure_releases.push_back(order_fence); order_fence_queue_.pop(); continue; @@ -144,6 +133,7 @@ void SyncPointOrderData::FinishProcessingOrderNumber(uint32_t order_num) { } for (OrderFence& order_fence : ensure_releases) { + DLOG(ERROR) << "Client did not release sync token as expected"; order_fence.client_state->EnsureWaitReleased(order_fence.fence_release, order_fence.release_callback); } @@ -158,19 +148,22 @@ bool SyncPointOrderData::ValidateReleaseOrderNumber( if (destroyed_) return false; - // Release should have a possible unprocessed order number lower than the wait - // order number. - if ((processed_order_num_ + 1) >= wait_order_num) + // We should have unprocessed order numbers which could potentially release + // this fence. + if (unprocessed_order_nums_.empty()) return false; - // Release should have more unprocessed numbers if we are waiting. - if (unprocessed_order_num_ <= processed_order_num_) + // We should have an unprocessed order number lower than the wait order + // number for the wait to be valid. It's not possible for wait order number to + // equal next unprocessed order number, but we handle that defensively. + DCHECK_NE(wait_order_num, unprocessed_order_nums_.front()); + if (wait_order_num <= unprocessed_order_nums_.front()) return false; // So far it could be valid, but add an order fence guard to be sure it // gets released eventually. uint32_t expected_order_num = - std::min(unprocessed_order_num_, wait_order_num); + std::min(unprocessed_order_nums_.back(), wait_order_num); order_fence_queue_.push(OrderFence(expected_order_num, fence_release, release_callback, std::move(client_state))); @@ -237,17 +230,20 @@ bool SyncPointClientState::WaitForRelease(uint64_t release, const base::Closure& callback) { // Lock must be held the whole time while we validate otherwise it could be // released while we are checking. - { - base::AutoLock auto_lock(fence_sync_lock_); - if (release > fence_sync_release_ && - order_data_->ValidateReleaseOrderNumber(this, wait_order_num, release, - callback)) { - // Add the callback which will be called upon release. - release_callback_queue_.push(ReleaseCallback(release, callback)); - return true; - } - } + base::AutoLock auto_lock(fence_sync_lock_); + // Already released, do not run the callback. + if (release <= fence_sync_release_) + return false; + + if (order_data_->ValidateReleaseOrderNumber(this, wait_order_num, release, + callback)) { + // Add the callback which will be called upon release. + release_callback_queue_.push(ReleaseCallback(release, callback)); + return true; + } + + DLOG(ERROR) << "Client waiting on non-existent sync token"; return false; } diff --git a/chromium/gpu/command_buffer/service/sync_point_manager.h b/chromium/gpu/command_buffer/service/sync_point_manager.h index be8e510d30f..ca5c4c95583 100644 --- a/chromium/gpu/command_buffer/service/sync_point_manager.h +++ b/chromium/gpu/command_buffer/service/sync_point_manager.h @@ -51,7 +51,7 @@ class GPU_EXPORT SyncPointOrderData uint32_t unprocessed_order_num() const { base::AutoLock auto_lock(lock_); - return unprocessed_order_num_; + return last_unprocessed_order_num_; } uint32_t current_order_num() const { @@ -122,9 +122,7 @@ class GPU_EXPORT SyncPointOrderData bool paused_ = false; // This lock protects destroyed_, processed_order_num_, - // unprocessed_order_num_, and order_fence_queue_. All order numbers (n) in - // order_fence_queue_ must follow the invariant: - // processed_order_num_ < n <= unprocessed_order_num_. + // unprocessed_order_nums_, and order_fence_queue_. mutable base::Lock lock_; bool destroyed_ = false; @@ -132,8 +130,13 @@ class GPU_EXPORT SyncPointOrderData // Last finished IPC order number. uint32_t processed_order_num_ = 0; - // Unprocessed order number expected to be processed under normal execution. - uint32_t unprocessed_order_num_ = 0; + // Last unprocessed order number. Updated in GenerateUnprocessedOrderNumber. + uint32_t last_unprocessed_order_num_ = 0; + + // Queue of unprocessed order numbers. Order numbers are enqueued in + // GenerateUnprocessedOrderNumber, and dequeued in + // FinishProcessingOrderNumber. + std::queue<uint32_t> unprocessed_order_nums_; // In situations where we are waiting on fence syncs that do not exist, we // validate by making sure the order number does not pass the order number @@ -141,7 +144,9 @@ class GPU_EXPORT SyncPointOrderData // wait command's, we should automatically release up to the expected // release count. Note that this also releases other lower release counts, // so a single misbehaved fence sync is enough to invalidate/signal all - // previous fence syncs. + // previous fence syncs. All order numbers (n) in order_fence_queue_ must + // follow the invariant: + // unprocessed_order_nums_.front() < n <= unprocessed_order_nums_.back(). OrderFenceQueue order_fence_queue_; DISALLOW_COPY_AND_ASSIGN(SyncPointOrderData); diff --git a/chromium/gpu/command_buffer/service/sync_point_manager_unittest.cc b/chromium/gpu/command_buffer/service/sync_point_manager_unittest.cc index a72c48a504b..f926a747baf 100644 --- a/chromium/gpu/command_buffer/service/sync_point_manager_unittest.cc +++ b/chromium/gpu/command_buffer/service/sync_point_manager_unittest.cc @@ -431,26 +431,20 @@ TEST_F(SyncPointManagerTest, NonExistentOrderNumRelease) { EXPECT_TRUE(valid_wait); EXPECT_EQ(10, test_num); - // Release stream should know it should release fence sync by order [3], - // so going through order [1] should not release it yet. + // Release stream should know it should release fence sync by order [3], but + // it has no unprocessed order numbers less than 3, so it runs the callback. release_stream.BeginProcessing(); EXPECT_EQ(1u, release_stream.order_data->current_order_num()); release_stream.EndProcessing(); EXPECT_FALSE(sync_point_manager_->IsSyncTokenReleased(sync_token)); - EXPECT_EQ(10, test_num); - - // Beginning order [4] should immediately trigger the wait although the fence - // sync is still not released yet. - release_stream.BeginProcessing(); - EXPECT_EQ(4u, release_stream.order_data->current_order_num()); EXPECT_EQ(123, test_num); - EXPECT_FALSE(sync_point_manager_->IsSyncTokenReleased(sync_token)); // Ensure that the wait callback does not get triggered again when it is // actually released. - test_num = 1; + release_stream.BeginProcessing(); + test_num = 10; release_stream.client_state->ReleaseFenceSync(1); - EXPECT_EQ(1, test_num); + EXPECT_EQ(10, test_num); EXPECT_TRUE(sync_point_manager_->IsSyncTokenReleased(sync_token)); } @@ -480,4 +474,123 @@ TEST_F(SyncPointManagerTest, WaitOnSameSequenceFails) { EXPECT_FALSE(sync_point_manager_->IsSyncTokenReleased(sync_token)); } +TEST_F(SyncPointManagerTest, HandleInvalidWaitOrderNumber) { + CommandBufferNamespace kNamespaceId = gpu::CommandBufferNamespace::GPU_IO; + CommandBufferId kCmdBufferId1 = CommandBufferId::FromUnsafeValue(0x123); + CommandBufferId kCmdBufferId2 = CommandBufferId::FromUnsafeValue(0x234); + + SyncPointStream stream1(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId1); + SyncPointStream stream2(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId2); + + stream1.AllocateOrderNum(); // stream 1, order num 1 + stream2.AllocateOrderNum(); // stream 2, order num 2 + stream2.AllocateOrderNum(); // stream 2, order num 3 + stream1.AllocateOrderNum(); // stream 1, order num 4 + + // Run stream 1, order num 1. + stream1.BeginProcessing(); + stream1.EndProcessing(); + + // Stream 2 waits on stream 1 with order num 3. This wait is invalid because + // there's no unprocessed order num less than 3 on stream 1. + int test_num = 10; + bool valid_wait = sync_point_manager_->Wait( + SyncToken(kNamespaceId, kCmdBufferId1, 1), + stream2.order_data->sequence_id(), 3, + base::Bind(&SyncPointManagerTest::SetIntegerFunction, &test_num, 123)); + EXPECT_FALSE(valid_wait); + EXPECT_EQ(10, test_num); +} + +TEST_F(SyncPointManagerTest, RetireInvalidWaitAfterOrderNumberPasses) { + CommandBufferNamespace kNamespaceId = gpu::CommandBufferNamespace::GPU_IO; + CommandBufferId kCmdBufferId1 = CommandBufferId::FromUnsafeValue(0x123); + CommandBufferId kCmdBufferId2 = CommandBufferId::FromUnsafeValue(0x234); + + SyncPointStream stream1(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId1); + SyncPointStream stream2(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId2); + + stream1.AllocateOrderNum(); // stream 1, order num 1 + stream1.AllocateOrderNum(); // stream 1, order num 2 + stream2.AllocateOrderNum(); // stream 2, order num 3 + + // Stream 2 waits on stream 1 with order num 3. + int test_num = 10; + bool valid_wait = sync_point_manager_->Wait( + SyncToken(kNamespaceId, kCmdBufferId1, 1), + stream2.order_data->sequence_id(), 3, + base::Bind(&SyncPointManagerTest::SetIntegerFunction, &test_num, 123)); + EXPECT_TRUE(valid_wait); + EXPECT_EQ(10, test_num); + + stream1.AllocateOrderNum(); // stream 1, order num 4 + + // Run stream 1, order num 1. The wait isn't retired. + stream1.BeginProcessing(); + stream1.EndProcessing(); + EXPECT_EQ(10, test_num); + + // Run stream 1, order num 2. Wait is retired because we reach the last order + // number that was unprocessed at the time the wait was enqueued. + stream1.BeginProcessing(); + stream1.EndProcessing(); + EXPECT_EQ(123, test_num); +} + +TEST_F(SyncPointManagerTest, HandleInvalidCyclicWaits) { + CommandBufferNamespace kNamespaceId = gpu::CommandBufferNamespace::GPU_IO; + CommandBufferId kCmdBufferId1 = CommandBufferId::FromUnsafeValue(0x123); + CommandBufferId kCmdBufferId2 = CommandBufferId::FromUnsafeValue(0x234); + + SyncPointStream stream1(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId1); + SyncPointStream stream2(sync_point_manager_.get(), kNamespaceId, + kCmdBufferId2); + + stream1.AllocateOrderNum(); // stream 1, order num 1 + stream2.AllocateOrderNum(); // stream 2, order num 2 + stream1.AllocateOrderNum(); // stream 1, order num 3 + stream2.AllocateOrderNum(); // stream 2, order num 4 + + // Stream 2 waits on stream 1 with order num 2. + int test_num1 = 10; + bool valid_wait = sync_point_manager_->Wait( + SyncToken(kNamespaceId, kCmdBufferId1, 1), + stream2.order_data->sequence_id(), 2, + base::Bind(&SyncPointManagerTest::SetIntegerFunction, &test_num1, 123)); + EXPECT_TRUE(valid_wait); + EXPECT_EQ(10, test_num1); + + // Stream 1 waits on stream 2 with order num 3. + int test_num2 = 10; + valid_wait = sync_point_manager_->Wait( + SyncToken(kNamespaceId, kCmdBufferId2, 1), + stream1.order_data->sequence_id(), 3, + base::Bind(&SyncPointManagerTest::SetIntegerFunction, &test_num2, 123)); + EXPECT_TRUE(valid_wait); + EXPECT_EQ(10, test_num2); + + // Run stream 1, order num 1. + stream1.BeginProcessing(); + stream1.EndProcessing(); + + // Since there's no unprocessed order num less than 2 on stream 1, the wait is + // released. + EXPECT_EQ(123, test_num1); + EXPECT_EQ(10, test_num2); + + // Run stream 2, order num 2. + stream2.BeginProcessing(); + stream2.EndProcessing(); + + // Since there's no unprocessed order num less than 3 on stream 2, the wait is + // released. + EXPECT_EQ(123, test_num1); + EXPECT_EQ(123, test_num2); +} + } // namespace gpu diff --git a/chromium/gpu/config/gpu_lists_version.h b/chromium/gpu/config/gpu_lists_version.h index 3d89bf18a19..2cc6499bd40 100644 --- a/chromium/gpu/config/gpu_lists_version.h +++ b/chromium/gpu/config/gpu_lists_version.h @@ -3,6 +3,6 @@ #ifndef GPU_CONFIG_GPU_LISTS_VERSION_H_ #define GPU_CONFIG_GPU_LISTS_VERSION_H_ -#define GPU_LISTS_VERSION "2b81d816997306dfdd0c2168d67028502f1cdee2" +#define GPU_LISTS_VERSION "926d27280b919a1aeb0091392d56f63a38d2bc0b" #endif // GPU_CONFIG_GPU_LISTS_VERSION_H_ |