From a4041364e16bfaf0551c135d6b821470d0b83ebc Mon Sep 17 00:00:00 2001 From: PixelyIon Date: Fri, 24 Dec 2021 12:39:18 +0530 Subject: [PATCH] Address CR comments Note: CR comments regarding `ShaderSet` and `PipelineStages` will be addressed at a later date with a common class for associative enum arrays. --- .../cpp/skyline/gpu/descriptor_allocator.cpp | 10 ++--- .../cpp/skyline/gpu/descriptor_allocator.h | 12 +++--- .../gpu/interconnect/graphics_context.h | 40 ++++++++++--------- .../main/cpp/skyline/gpu/shader_manager.cpp | 8 ++-- 4 files changed, 36 insertions(+), 34 deletions(-) diff --git a/app/src/main/cpp/skyline/gpu/descriptor_allocator.cpp b/app/src/main/cpp/skyline/gpu/descriptor_allocator.cpp index ca1405a5..8d1bd9cb 100644 --- a/app/src/main/cpp/skyline/gpu/descriptor_allocator.cpp +++ b/app/src/main/cpp/skyline/gpu/descriptor_allocator.cpp @@ -6,7 +6,7 @@ #include "descriptor_allocator.h" namespace skyline::gpu { - DescriptorAllocator::DescriptorPool::DescriptorPool(const vk::raii::Device &device, const vk::DescriptorPoolCreateInfo &createInfo) : vk::raii::DescriptorPool(device, createInfo), setCount(createInfo.maxSets) {} + DescriptorAllocator::DescriptorPool::DescriptorPool(const vk::raii::Device &device, const vk::DescriptorPoolCreateInfo &createInfo) : vk::raii::DescriptorPool(device, createInfo), freeSetCount(createInfo.maxSets) {} void DescriptorAllocator::AllocateDescriptorPool() { namespace maxwell3d = soc::gm20b::engine::maxwell3d::type; // We use Maxwell3D as reference for base descriptor counts @@ -31,13 +31,13 @@ namespace skyline::gpu { } DescriptorAllocator::ActiveDescriptorSet::ActiveDescriptorSet(std::shared_ptr pPool, vk::DescriptorSet set) : pool(std::move(pPool)), DescriptorSet(set) { - pool->setCount--; + pool->freeSetCount--; } DescriptorAllocator::ActiveDescriptorSet::~ActiveDescriptorSet() { std::scoped_lock lock(*pool); pool->getDevice().freeDescriptorSets(**pool, 1, this, *pool->getDispatcher()); - pool->setCount++; + pool->freeSetCount++; } DescriptorAllocator::DescriptorAllocator(GPU &gpu) : gpu(gpu) { @@ -61,9 +61,9 @@ namespace skyline::gpu { if (result == vk::Result::eSuccess) { return ActiveDescriptorSet(pool, set); } else if (result == vk::Result::eErrorOutOfPoolMemory) { - if (pool->setCount == 0) + if (pool->freeSetCount == 0) // The amount of maximum descriptor sets is insufficient - descriptorSetCount += BaseDescriptorSetCount; + descriptorSetCount += DescriptorSetCountIncrement; else // The amount of maximum descriptors is insufficient descriptorMultiplier++; diff --git a/app/src/main/cpp/skyline/gpu/descriptor_allocator.h b/app/src/main/cpp/skyline/gpu/descriptor_allocator.h index 6478402e..9e83e619 100644 --- a/app/src/main/cpp/skyline/gpu/descriptor_allocator.h +++ b/app/src/main/cpp/skyline/gpu/descriptor_allocator.h @@ -14,15 +14,15 @@ namespace skyline::gpu { GPU &gpu; std::mutex mutex; //!< Synchronizes the creation and replacement of the pool object - static constexpr u32 BaseDescriptorSetCount{64}; //!< An arbitrary amount of descriptor sets that we allocate in multiples of - u32 descriptorSetCount{BaseDescriptorSetCount}; //!< The maximum amount of descriptor sets in the pool + static constexpr u32 DescriptorSetCountIncrement{64}; //!< The amount of descriptor sets that we allocate in increments of + u32 descriptorSetCount{DescriptorSetCountIncrement}; //!< The maximum amount of descriptor sets in the pool u32 descriptorMultiplier{1}; //!< A multiplier for the maximum amount of descriptors in the pool /** * @brief A lockable VkDescriptorPool for maintaining external synchronization requirements */ struct DescriptorPool : public std::mutex, public vk::raii::DescriptorPool { - u64 setCount{}; //!< The amount of sets free to allocate from this pool + u64 freeSetCount{}; //!< The amount of sets free to allocate from this pool DescriptorPool(vk::raii::Device const &device, vk::DescriptorPoolCreateInfo const &createInfo); }; @@ -30,14 +30,14 @@ namespace skyline::gpu { std::shared_ptr pool; //!< The current pool used by any allocations in the class, replaced when an error is ran into /** - * @brief (Re-)Allocates the descriptor pool with the current multiplier applied to the base counts + * @brief (Re-)Allocates the descriptor pool with the current multiplier applied to the descriptor counts and the current descriptor set count * @note `DescriptorAllocator::mutex` **must** be locked prior to calling this */ void AllocateDescriptorPool(); public: /** - * @brief A RAII-bound descriptor set that automatically frees of resources into the pool on destruction while respecting external synchronization requirements + * @brief A RAII-bound descriptor set that automatically frees resources into the pool on destruction while respecting external synchronization requirements */ struct ActiveDescriptorSet : public vk::DescriptorSet { private: @@ -56,7 +56,7 @@ namespace skyline::gpu { DescriptorAllocator(GPU &gpu); /** - * @note It is UB to allocate a set with a descriptor type that isn't in the pool + * @note It is UB to allocate a set with a descriptor type that isn't in the pool as defined in AllocateDescriptorPool */ ActiveDescriptorSet AllocateSet(vk::DescriptorSetLayout layout); }; diff --git a/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h b/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h index 95df9f35..4f2bba66 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h +++ b/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h @@ -592,8 +592,8 @@ namespace skyline::gpu::interconnect { } }; - struct Shaders : public std::array { - Shaders() : array({ + struct ShaderSet : public std::array { + ShaderSet() : array({ Shader{ShaderCompiler::Stage::VertexA}, Shader{ShaderCompiler::Stage::VertexB}, Shader{ShaderCompiler::Stage::TessellationControl}, @@ -646,7 +646,7 @@ namespace skyline::gpu::interconnect { }; IOVA shaderBaseIova{}; //!< The base IOVA that shaders are located at an offset from - Shaders shaders; + ShaderSet shaders; PipelineStages pipelineStages; std::array shaderStagesInfo{}; //!< Storage backing for the pipeline shader stage information for all shaders aside from 'VertexA' which uses the same stage as 'VertexB' @@ -688,7 +688,7 @@ namespace skyline::gpu::interconnect { auto setStageRecompile{[this](maxwell3d::PipelineStage stage) { pipelineStages[stage].needsRecompile = true; }}; - ((void) setStageRecompile(stages), ...); + (setStageRecompile(stages), ...); } } @@ -704,24 +704,26 @@ namespace skyline::gpu::interconnect { if (shader.invalidated) { // If a shader is invalidated, we need to reparse the program (given that it has changed) - bool shouldParseShader{true}; - if (!shader.data.empty() && shader.shouldCheckSame) { - // A fast path to check if the shader is the same as before to avoid reparsing the shader - auto newIovaRanges{channelCtx.asCtx->gmmu.TranslateRange(shaderBaseIova + shader.offset, shader.data.size())}; - auto originalShader{shader.data.data()}; + bool shouldParseShader{[&]() { + if (!shader.data.empty() && shader.shouldCheckSame) { + // A fast path to check if the shader is the same as before to avoid reparsing the shader + auto newIovaRanges{channelCtx.asCtx->gmmu.TranslateRange(shaderBaseIova + shader.offset, shader.data.size())}; + auto originalShader{shader.data.data()}; - shouldParseShader = false; - for (auto &range : newIovaRanges) { - if (range.data() && std::memcmp(range.data(), originalShader, range.size()) == 0) { - originalShader += range.size(); - } else { - shouldParseShader = true; - break; + for (auto &range : newIovaRanges) { + if (range.data() && std::memcmp(range.data(), originalShader, range.size()) == 0) { + originalShader += range.size(); + } else { + return true; + } } - } - shader.shouldCheckSame = true; - } + return false; + } else { + shader.shouldCheckSame = true; // We want to reset the value and check for it being same the next time + return true; + } + }()}; if (shouldParseShader) { // A pass to check if the shader has a BRA infloop opcode ending (On most commercial games) diff --git a/app/src/main/cpp/skyline/gpu/shader_manager.cpp b/app/src/main/cpp/skyline/gpu/shader_manager.cpp index ac3651d7..37ceba61 100644 --- a/app/src/main/cpp/skyline/gpu/shader_manager.cpp +++ b/app/src/main/cpp/skyline/gpu/shader_manager.cpp @@ -117,11 +117,11 @@ namespace skyline::gpu { } [[nodiscard]] u32 SharedMemorySize() const final { - return 0; // Shared memory size is only relevant for compute shaders + return 0; // Only relevant for compute shaders } [[nodiscard]] std::array WorkgroupSize() const final { - return {0, 0, 0}; // Workgroup size is only relevant for compute shaders + return {0, 0, 0}; // Only relevant for compute shaders } }; @@ -156,11 +156,11 @@ namespace skyline::gpu { } [[nodiscard]] u32 SharedMemorySize() const final { - return 0; + return 0; // Only relevant for compute shaders } [[nodiscard]] std::array WorkgroupSize() const final { - return {0, 0, 0}; + return {0, 0, 0}; // Only relevant for compute shaders } };