Reimplement vk::Query using sw sync primitives. Hide all the error-prone synchronization logic in the class, and expose a documented API. This should have no impact on behavior - it was authored to make it harder to break things in the future. That said, it appears to fix a whole bunch of flakes with the dEQP query tests. Bug: b/133127573 Change-Id: I5c30b79b9b1cd36dba1fa2d3c34af0f5bd62772a Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/31816 Kokoro-Presubmit: kokoro <noreply+kokoro@google.com> Reviewed-by: Chris Forbes <chrisforbes@google.com> Reviewed-by: Alexis Hétu <sugoi@google.com> Tested-by: Ben Clayton <bclayton@google.com>
diff --git a/src/Device/Renderer.cpp b/src/Device/Renderer.cpp index 85eb7f4..6ed66ff 100644 --- a/src/Device/Renderer.cpp +++ b/src/Device/Renderer.cpp
@@ -289,7 +289,7 @@ { for(auto query : queries) { - if(query->type == type) + if(query->getType() == type) { return true; } @@ -377,7 +377,7 @@ draw->queries = new std::list<vk::Query*>(); for(auto &query : queries) { - ++query->reference; // Atomic + query->start(); draw->queries->push_back(query); } } @@ -855,34 +855,23 @@ { for(auto &query : *(draw.queries)) { - std::unique_lock<std::mutex> mutexLock(query->mutex); - - switch(query->type) + switch(query->getType()) { case VK_QUERY_TYPE_OCCLUSION: for(int cluster = 0; cluster < clusterCount; cluster++) { - query->data += data.occlusion[cluster]; + query->add(data.occlusion[cluster]); } break; default: break; } - int queryRef = --query->reference; // Atomic - if(queryRef == 0) - { - query->state = vk::Query::FINISHED; - } - - // Manual unlocking is done before notifying, to avoid - // waking up the waiting thread only to block again - mutexLock.unlock(); - query->condition.notify_one(); + query->finish(); } delete draw.queries; - draw.queries = 0; + draw.queries = nullptr; } draw.vertexRoutine->unbind();
diff --git a/src/Device/Renderer.hpp b/src/Device/Renderer.hpp index 0d01f45..e8242e0 100644 --- a/src/Device/Renderer.hpp +++ b/src/Device/Renderer.hpp
@@ -32,7 +32,7 @@ namespace vk { class DescriptorSet; - struct Query; + class Query; } namespace sw
diff --git a/src/Vulkan/VkQueryPool.cpp b/src/Vulkan/VkQueryPool.cpp index f8b34f4..7211d0d 100644 --- a/src/Vulkan/VkQueryPool.cpp +++ b/src/Vulkan/VkQueryPool.cpp
@@ -13,7 +13,6 @@ // limitations under the License. #include "VkQueryPool.hpp" -#include "Common/Thread.hpp" #include <chrono> #include <cstring> @@ -21,6 +20,69 @@ namespace vk { + Query::Query() : finished(sw::Event::ClearMode::Manual), state(UNAVAILABLE), type(INVALID_TYPE), value(0) {} + + void Query::reset() + { + ASSERT(wg.count() == 0); + finished.clear(); + auto prevState = state.exchange(UNAVAILABLE); + ASSERT(prevState != ACTIVE); + type = INVALID_TYPE; + value = 0; + } + + void Query::prepare(VkQueryType ty) + { + auto prevState = state.exchange(ACTIVE); + ASSERT(prevState == UNAVAILABLE); + type = ty; + } + + void Query::start() + { + ASSERT(state == ACTIVE); + wg.add(); + } + + void Query::finish() + { + if (wg.done()) + { + auto prevState = state.exchange(FINISHED); + ASSERT(prevState == ACTIVE); + finished.signal(); + } + } + + Query::Data Query::getData() const + { + Data out; + out.state = state; + out.value = value; + return out; + } + + VkQueryType Query::getType() const + { + return type; + } + + void Query::wait() + { + finished.wait(); + } + + void Query::set(int64_t v) + { + value = v; + } + + void Query::add(int64_t v) + { + value += v; + } + QueryPool::QueryPool(const VkQueryPoolCreateInfo* pCreateInfo, void* mem) : pool(reinterpret_cast<Query*>(mem)), type(pCreateInfo->queryType), count(pCreateInfo->queryCount) @@ -61,7 +123,7 @@ // The sum of firstQuery and queryCount must be less than or equal to the number of queries ASSERT((firstQuery + queryCount) <= count); - + VkResult result = VK_SUCCESS; uint8_t* data = static_cast<uint8_t*>(pData); for(uint32_t i = firstQuery; i < (firstQuery + queryCount); i++, data += stride) @@ -72,14 +134,16 @@ // VK_NOT_READY. However, availability state is still written to pData for those // queries if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set. auto &query = pool[i]; - std::unique_lock<std::mutex> mutexLock(query.mutex); + if(flags & VK_QUERY_RESULT_WAIT_BIT) // Must wait for query to finish { - query.condition.wait(mutexLock, [&query] { return query.state == Query::FINISHED; }); + query.wait(); } + const auto current = query.getData(); + bool writeResult = true; - if(pool[i].state == Query::ACTIVE) + if(current.state == Query::ACTIVE) { result = VK_NOT_READY; writeResult = (flags & VK_QUERY_RESULT_PARTIAL_BIT); // Allow writing partial results @@ -90,11 +154,11 @@ uint64_t* result64 = reinterpret_cast<uint64_t*>(data); if(writeResult) { - result64[0] = pool[i].data; + result64[0] = current.value; } if(flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) // Output query availablity { - result64[1] = pool[i].state; + result64[1] = current.state; } } else @@ -102,11 +166,11 @@ uint32_t* result32 = reinterpret_cast<uint32_t*>(data); if(writeResult) { - result32[0] = static_cast<uint32_t>(pool[i].data); + result32[0] = static_cast<uint32_t>(current.value); } if(flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) // Output query availablity { - result32[1] = pool[i].state; + result32[1] = current.state; } } } @@ -123,31 +187,14 @@ UNIMPLEMENTED("flags"); } - std::unique_lock<std::mutex> lock(pool[query].mutex); - ASSERT(pool[query].state == Query::UNAVAILABLE); - pool[query].state = Query::ACTIVE; - pool[query].data = 0; - pool[query].reference = 1; - pool[query].type = type; + pool[query].prepare(type); + pool[query].start(); } void QueryPool::end(uint32_t query) { ASSERT(query < count); - - #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) - { - std::unique_lock<std::mutex> mutexLock(pool[query].mutex); - ASSERT(pool[query].state == Query::ACTIVE); - } - #endif - - int ref = --pool[query].reference; - if(ref == 0) - { - std::unique_lock<std::mutex> mutexLock(pool[query].mutex); - pool[query].state = Query::FINISHED; - } + pool[query].finish(); } void QueryPool::reset(uint32_t firstQuery, uint32_t queryCount) @@ -157,10 +204,7 @@ for(uint32_t i = firstQuery; i < (firstQuery + queryCount); i++) { - std::unique_lock<std::mutex> mutexLock(pool[i].mutex); - - pool[i].state = Query::UNAVAILABLE; - pool[i].data = 0; + pool[i].reset(); } } @@ -169,8 +213,7 @@ ASSERT(query < count); ASSERT(type == VK_QUERY_TYPE_TIMESTAMP); - std::unique_lock<std::mutex> mutexLock(pool[query].mutex); - pool[query].data = std::chrono::time_point_cast<std::chrono::nanoseconds>( - std::chrono::system_clock::now()).time_since_epoch().count(); + pool[query].set(std::chrono::time_point_cast<std::chrono::nanoseconds>( + std::chrono::system_clock::now()).time_since_epoch().count()); } } // namespace vk
diff --git a/src/Vulkan/VkQueryPool.hpp b/src/Vulkan/VkQueryPool.hpp index d85bf85..88ca21b 100644 --- a/src/Vulkan/VkQueryPool.hpp +++ b/src/Vulkan/VkQueryPool.hpp
@@ -15,6 +15,8 @@ #ifndef VK_QUERY_POOL_HPP_ #define VK_QUERY_POOL_HPP_ +#include "System/Synchronization.hpp" + #include "VkObject.hpp" #include <atomic> #include <condition_variable> @@ -23,8 +25,13 @@ namespace vk { -struct Query +class Query { +public: + static auto constexpr INVALID_TYPE = VK_QUERY_TYPE_MAX_ENUM; + + Query(); + enum State { UNAVAILABLE, @@ -32,12 +39,53 @@ FINISHED }; - std::mutex mutex; - std::condition_variable condition; - State state; // guarded by mutex - int64_t data; // guarded by mutex - std::atomic<int> reference; - VkQueryType type; + struct Data + { + State state; // The current query state. + int64_t value; // The current query value. + }; + + // reset() sets the state of the Query to UNAVAILABLE, sets the type to + // INVALID_TYPE and clears the query value. + // reset() must not be called while the query is in the ACTIVE state. + void reset(); + + // prepare() sets the Query type to ty, and sets the state to ACTIVE. + // prepare() must not be called when the query is already ACTIVE. + void prepare(VkQueryType ty); + + // start() begins a query task which is closed with a call to finish(). + // Query tasks can be nested. + // start() must only be called when in the ACTIVE state. + void start(); + + // finish() ends a query task begun with a call to start(). + // Once all query tasks are complete the query will transition to the + // FINISHED state. + // finish() must only be called when in the ACTIVE state. + void finish(); + + // wait() blocks until the query reaches the FINISHED state. + void wait(); + + // getData() returns the current query state and value. + Data getData() const; + + // getType() returns the type of query. + VkQueryType getType() const; + + // set() replaces the current query value with val. + void set(int64_t val); + + // add() adds val to the current query value. + void add(int64_t val); + +private: + sw::WaitGroup wg; + sw::Event finished; + std::atomic<State> state; + std::atomic<VkQueryType> type; + std::atomic<int64_t> value; }; class QueryPool : public Object<QueryPool, VkQueryPool>