From 2dc0ff79ece34286c7078922668fd9f0a19b47b7 Mon Sep 17 00:00:00 2001 From: Wollnashorn Date: Fri, 16 Jun 2023 13:26:44 +0200 Subject: [PATCH] video_core: Use sampler IDs instead pointers in the pipeline config The previous approach of storing pointers returned by `GetGraphicsSampler`/`GetComputeSampler` caused UB, as these functions can cause reallocation of the sampler slot vector and therefore invalidate the pointers --- .../renderer_opengl/gl_compute_pipeline.cpp | 23 ++++++++++---- .../renderer_opengl/gl_graphics_pipeline.cpp | 8 ++--- .../renderer_vulkan/pipeline_helper.h | 5 ++-- .../renderer_vulkan/vk_compute_pipeline.cpp | 6 ++-- .../renderer_vulkan/vk_graphics_pipeline.cpp | 6 ++-- .../texture_cache/image_view_base.cpp | 1 - src/video_core/texture_cache/texture_cache.h | 30 +++++++++++++++---- .../texture_cache/texture_cache_base.h | 12 ++++++++ 8 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_compute_pipeline.cpp b/src/video_core/renderer_opengl/gl_compute_pipeline.cpp index 1a0cea9b7..3151c0db8 100644 --- a/src/video_core/renderer_opengl/gl_compute_pipeline.cpp +++ b/src/video_core/renderer_opengl/gl_compute_pipeline.cpp @@ -87,7 +87,8 @@ void ComputePipeline::Configure() { texture_cache.SynchronizeComputeDescriptors(); boost::container::static_vector views; - std::array samplers; + boost::container::static_vector samplers; + std::array gl_samplers; std::array textures; std::array images; GLsizei sampler_binding{}; @@ -131,7 +132,6 @@ void ComputePipeline::Configure() { for (u32 index = 0; index < desc.count; ++index) { const auto handle{read_handle(desc, index)}; views.push_back({handle.first}); - samplers[sampler_binding++] = 0; } } for (const auto& desc : info.image_buffer_descriptors) { @@ -142,8 +142,8 @@ void ComputePipeline::Configure() { const auto handle{read_handle(desc, index)}; views.push_back({handle.first}); - Sampler* const sampler = texture_cache.GetComputeSampler(handle.second); - samplers[sampler_binding++] = sampler->Handle(); + VideoCommon::SamplerId sampler = texture_cache.GetComputeSamplerId(handle.second); + samplers.push_back(sampler); } } for (const auto& desc : info.image_descriptors) { @@ -186,10 +186,17 @@ void ComputePipeline::Configure() { const VideoCommon::ImageViewInOut* views_it{views.data() + num_texture_buffers + num_image_buffers}; + const VideoCommon::SamplerId* samplers_it{samplers.data()}; texture_binding += num_texture_buffers; image_binding += num_image_buffers; u32 texture_scaling_mask{}; + + for (const auto& desc : info.texture_buffer_descriptors) { + for (u32 index = 0; index < desc.count; ++index) { + gl_samplers[sampler_binding++] = 0; + } + } for (const auto& desc : info.texture_descriptors) { for (u32 index = 0; index < desc.count; ++index) { ImageView& image_view{texture_cache.GetImageView((views_it++)->id)}; @@ -198,6 +205,12 @@ void ComputePipeline::Configure() { texture_scaling_mask |= 1u << texture_binding; } ++texture_binding; + + const Sampler& sampler{texture_cache.GetSampler(*(samplers_it++))}; + const bool use_fallback_sampler{sampler.HasAddedAnisotropy() && + !image_view.SupportsAnisotropy()}; + gl_samplers[sampler_binding++] = + use_fallback_sampler ? sampler.HandleWithDefaultAnisotropy() : sampler.Handle(); } } u32 image_scaling_mask{}; @@ -228,7 +241,7 @@ void ComputePipeline::Configure() { if (texture_binding != 0) { ASSERT(texture_binding == sampler_binding); glBindTextures(0, texture_binding, textures.data()); - glBindSamplers(0, sampler_binding, samplers.data()); + glBindSamplers(0, sampler_binding, gl_samplers.data()); } if (image_binding != 0) { glBindImageTextures(0, image_binding, images.data()); diff --git a/src/video_core/renderer_opengl/gl_graphics_pipeline.cpp b/src/video_core/renderer_opengl/gl_graphics_pipeline.cpp index 58e4e1919..c58f760b8 100644 --- a/src/video_core/renderer_opengl/gl_graphics_pipeline.cpp +++ b/src/video_core/renderer_opengl/gl_graphics_pipeline.cpp @@ -275,7 +275,7 @@ GraphicsPipeline::GraphicsPipeline(const Device& device, TextureCache& texture_c template void GraphicsPipeline::ConfigureImpl(bool is_indexed) { std::array views; - std::array samplers; + std::array samplers; size_t views_index{}; size_t samplers_index{}; @@ -350,7 +350,7 @@ void GraphicsPipeline::ConfigureImpl(bool is_indexed) { const auto handle{read_handle(desc, index)}; views[views_index++] = {handle.first}; - Sampler* const sampler{texture_cache.GetGraphicsSampler(handle.second)}; + VideoCommon::SamplerId sampler{texture_cache.GetGraphicsSamplerId(handle.second)}; samplers[samplers_index++] = sampler; } } @@ -444,7 +444,7 @@ void GraphicsPipeline::ConfigureImpl(bool is_indexed) { program_manager.BindSourcePrograms(source_programs); } const VideoCommon::ImageViewInOut* views_it{views.data()}; - const Sampler** samplers_it{samplers.data()}; + const VideoCommon::SamplerId* samplers_it{samplers.data()}; GLsizei texture_binding = 0; GLsizei image_binding = 0; GLsizei sampler_binding{}; @@ -484,7 +484,7 @@ void GraphicsPipeline::ConfigureImpl(bool is_indexed) { ++texture_binding; ++stage_texture_binding; - const Sampler& sampler{**(samplers_it++)}; + const Sampler& sampler{texture_cache.GetSampler(*(samplers_it++))}; const bool use_fallback_sampler{sampler.HasAddedAnisotropy() && !image_view.SupportsAnisotropy()}; gl_samplers[sampler_binding++] = diff --git a/src/video_core/renderer_vulkan/pipeline_helper.h b/src/video_core/renderer_vulkan/pipeline_helper.h index 0a9dce937..71c783709 100644 --- a/src/video_core/renderer_vulkan/pipeline_helper.h +++ b/src/video_core/renderer_vulkan/pipeline_helper.h @@ -178,7 +178,7 @@ public: inline void PushImageDescriptors(TextureCache& texture_cache, GuestDescriptorQueue& guest_descriptor_queue, const Shader::Info& info, RescalingPushConstant& rescaling, - const Sampler**& samplers, + const VideoCommon::SamplerId*& samplers, const VideoCommon::ImageViewInOut*& views) { const u32 num_texture_buffers = Shader::NumDescriptors(info.texture_buffer_descriptors); const u32 num_image_buffers = Shader::NumDescriptors(info.image_buffer_descriptors); @@ -187,9 +187,10 @@ inline void PushImageDescriptors(TextureCache& texture_cache, for (const auto& desc : info.texture_descriptors) { for (u32 index = 0; index < desc.count; ++index) { const VideoCommon::ImageViewId image_view_id{(views++)->id}; + const VideoCommon::SamplerId sampler_id{*(samplers++)}; ImageView& image_view{texture_cache.GetImageView(image_view_id)}; const VkImageView vk_image_view{image_view.Handle(desc.type)}; - const Sampler& sampler{**(samplers++)}; + const Sampler& sampler{texture_cache.GetSampler(sampler_id)}; const bool use_fallback_sampler{sampler.HasAddedAnisotropy() && !image_view.SupportsAnisotropy()}; const VkSampler vk_sampler{use_fallback_sampler ? sampler.HandleWithDefaultAnisotropy() diff --git a/src/video_core/renderer_vulkan/vk_compute_pipeline.cpp b/src/video_core/renderer_vulkan/vk_compute_pipeline.cpp index eb292dc34..73e585c2b 100644 --- a/src/video_core/renderer_vulkan/vk_compute_pipeline.cpp +++ b/src/video_core/renderer_vulkan/vk_compute_pipeline.cpp @@ -115,7 +115,7 @@ void ComputePipeline::Configure(Tegra::Engines::KeplerCompute& kepler_compute, static constexpr size_t max_elements = 64; boost::container::static_vector views; - boost::container::static_vector samplers; + boost::container::static_vector samplers; const auto& qmd{kepler_compute.launch_description}; const auto& cbufs{qmd.const_buffer_config}; @@ -160,7 +160,7 @@ void ComputePipeline::Configure(Tegra::Engines::KeplerCompute& kepler_compute, const auto handle{read_handle(desc, index)}; views.push_back({handle.first}); - Sampler* const sampler = texture_cache.GetComputeSampler(handle.second); + VideoCommon::SamplerId sampler = texture_cache.GetComputeSamplerId(handle.second); samplers.push_back(sampler); } } @@ -192,7 +192,7 @@ void ComputePipeline::Configure(Tegra::Engines::KeplerCompute& kepler_compute, buffer_cache.BindHostComputeBuffers(); RescalingPushConstant rescaling; - const Sampler** samplers_it{samplers.data()}; + const VideoCommon::SamplerId* samplers_it{samplers.data()}; const VideoCommon::ImageViewInOut* views_it{views.data()}; PushImageDescriptors(texture_cache, guest_descriptor_queue, info, rescaling, samplers_it, views_it); diff --git a/src/video_core/renderer_vulkan/vk_graphics_pipeline.cpp b/src/video_core/renderer_vulkan/vk_graphics_pipeline.cpp index e6e004cb6..c1595642e 100644 --- a/src/video_core/renderer_vulkan/vk_graphics_pipeline.cpp +++ b/src/video_core/renderer_vulkan/vk_graphics_pipeline.cpp @@ -298,7 +298,7 @@ void GraphicsPipeline::AddTransition(GraphicsPipeline* transition) { template void GraphicsPipeline::ConfigureImpl(bool is_indexed) { std::array views; - std::array samplers; + std::array samplers; size_t sampler_index{}; size_t view_index{}; @@ -367,7 +367,7 @@ void GraphicsPipeline::ConfigureImpl(bool is_indexed) { const auto handle{read_handle(desc, index)}; views[view_index++] = {handle.first}; - Sampler* const sampler{texture_cache.GetGraphicsSampler(handle.second)}; + VideoCommon::SamplerId sampler{texture_cache.GetGraphicsSamplerId(handle.second)}; samplers[sampler_index++] = sampler; } } @@ -453,7 +453,7 @@ void GraphicsPipeline::ConfigureImpl(bool is_indexed) { RescalingPushConstant rescaling; RenderAreaPushConstant render_area; - const Sampler** samplers_it{samplers.data()}; + const VideoCommon::SamplerId* samplers_it{samplers.data()}; const VideoCommon::ImageViewInOut* views_it{views.data()}; const auto prepare_stage{[&](size_t stage) LAMBDA_FORCEINLINE { buffer_cache.BindHostStageBuffers(stage); diff --git a/src/video_core/texture_cache/image_view_base.cpp b/src/video_core/texture_cache/image_view_base.cpp index 5ebe9b9bb..000eec869 100644 --- a/src/video_core/texture_cache/image_view_base.cpp +++ b/src/video_core/texture_cache/image_view_base.cpp @@ -46,7 +46,6 @@ ImageViewBase::ImageViewBase(const ImageInfo& info, const ImageViewInfo& view_in ImageViewBase::ImageViewBase(const NullImageViewParams&) : image_id{NULL_IMAGE_ID} {} bool ImageViewBase::SupportsAnisotropy() const noexcept { - using namespace VideoCommon; switch (format) { case PixelFormat::R8_UNORM: case PixelFormat::R8_SNORM: diff --git a/src/video_core/texture_cache/texture_cache.h b/src/video_core/texture_cache/texture_cache.h index c7f7448e9..4027d860b 100644 --- a/src/video_core/texture_cache/texture_cache.h +++ b/src/video_core/texture_cache/texture_cache.h @@ -222,30 +222,50 @@ void TextureCache

::CheckFeedbackLoop(std::span views) { template typename P::Sampler* TextureCache

::GetGraphicsSampler(u32 index) { + return &slot_samplers[GetGraphicsSamplerId(index)]; +} + +template +typename P::Sampler* TextureCache

::GetComputeSampler(u32 index) { + return &slot_samplers[GetComputeSamplerId(index)]; +} + +template +SamplerId TextureCache

::GetGraphicsSamplerId(u32 index) { if (index > channel_state->graphics_sampler_table.Limit()) { LOG_DEBUG(HW_GPU, "Invalid sampler index={}", index); - return &slot_samplers[NULL_SAMPLER_ID]; + return NULL_SAMPLER_ID; } const auto [descriptor, is_new] = channel_state->graphics_sampler_table.Read(index); SamplerId& id = channel_state->graphics_sampler_ids[index]; if (is_new) { id = FindSampler(descriptor); } - return &slot_samplers[id]; + return id; } template -typename P::Sampler* TextureCache

::GetComputeSampler(u32 index) { +SamplerId TextureCache

::GetComputeSamplerId(u32 index) { if (index > channel_state->compute_sampler_table.Limit()) { LOG_DEBUG(HW_GPU, "Invalid sampler index={}", index); - return &slot_samplers[NULL_SAMPLER_ID]; + return NULL_SAMPLER_ID; } const auto [descriptor, is_new] = channel_state->compute_sampler_table.Read(index); SamplerId& id = channel_state->compute_sampler_ids[index]; if (is_new) { id = FindSampler(descriptor); } - return &slot_samplers[id]; + return id; +} + +template +const typename P::Sampler& TextureCache

::GetSampler(SamplerId id) const noexcept { + return slot_samplers[id]; +} + +template +typename P::Sampler& TextureCache

::GetSampler(SamplerId id) noexcept { + return slot_samplers[id]; } template diff --git a/src/video_core/texture_cache/texture_cache_base.h b/src/video_core/texture_cache/texture_cache_base.h index 3bfa92154..d96ddea9d 100644 --- a/src/video_core/texture_cache/texture_cache_base.h +++ b/src/video_core/texture_cache/texture_cache_base.h @@ -159,6 +159,18 @@ public: /// Get the sampler from the compute descriptor table in the specified index Sampler* GetComputeSampler(u32 index); + /// Get the sampler id from the graphics descriptor table in the specified index + SamplerId GetGraphicsSamplerId(u32 index); + + /// Get the sampler id from the compute descriptor table in the specified index + SamplerId GetComputeSamplerId(u32 index); + + /// Return a constant reference to the given sampler id + [[nodiscard]] const Sampler& GetSampler(SamplerId id) const noexcept; + + /// Return a reference to the given sampler id + [[nodiscard]] Sampler& GetSampler(SamplerId id) noexcept; + /// Refresh the state for graphics image view and sampler descriptors void SynchronizeGraphicsDescriptors();