From 16b8f65f6f3f6fd0dcb64b22539f7c2900e81e5d Mon Sep 17 00:00:00 2001 From: Michael Parkin-White Date: Tue, 28 Feb 2023 16:52:04 +0000 Subject: [PATCH] Fix: Ensure explicit UBO bind indices are used in Metal. Previously, UBO bind locations were linearly incremented and relied on the correct uniform location being queried. This fix is a future requirement for EEVEE next, however, pulling forward due to Issue #105280 highlighting a possible flaw with expected uniform locations. Authored by Apple: Michael Parkin-White Ref #96261 --- source/blender/gpu/metal/mtl_context.mm | 12 +++--- source/blender/gpu/metal/mtl_shader.mm | 38 ++++++++++++++++--- .../blender/gpu/metal/mtl_shader_generator.hh | 4 ++ .../blender/gpu/metal/mtl_shader_generator.mm | 17 ++++++--- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/source/blender/gpu/metal/mtl_context.mm b/source/blender/gpu/metal/mtl_context.mm index 3c1872d66f0..b5e10bccfb5 100644 --- a/source/blender/gpu/metal/mtl_context.mm +++ b/source/blender/gpu/metal/mtl_context.mm @@ -1075,13 +1075,13 @@ bool MTLContext::ensure_uniform_buffer_bindings( int ubo_size = 0; bool bind_dummy_buffer = false; - if (this->pipeline_state.ubo_bindings[ubo_index].bound) { + if (this->pipeline_state.ubo_bindings[ubo.buffer_index].bound) { /* Fetch UBO global-binding properties from slot. */ ubo_offset = 0; - ubo_buffer = this->pipeline_state.ubo_bindings[ubo_index].ubo->get_metal_buffer( + ubo_buffer = this->pipeline_state.ubo_bindings[ubo.buffer_index].ubo->get_metal_buffer( &ubo_offset); - ubo_size = this->pipeline_state.ubo_bindings[ubo_index].ubo->get_size(); + ubo_size = this->pipeline_state.ubo_bindings[ubo.buffer_index].ubo->get_size(); /* Use dummy zero buffer if no buffer assigned -- this is an optimization to avoid * allocating zero buffers. */ @@ -1232,13 +1232,13 @@ bool MTLContext::ensure_uniform_buffer_bindings( int ubo_size = 0; bool bind_dummy_buffer = false; - if (this->pipeline_state.ubo_bindings[ubo_index].bound) { + if (this->pipeline_state.ubo_bindings[ubo.buffer_index].bound) { /* Fetch UBO global-binding properties from slot. */ ubo_offset = 0; - ubo_buffer = this->pipeline_state.ubo_bindings[ubo_index].ubo->get_metal_buffer( + ubo_buffer = this->pipeline_state.ubo_bindings[ubo.buffer_index].ubo->get_metal_buffer( &ubo_offset); - ubo_size = this->pipeline_state.ubo_bindings[ubo_index].ubo->get_size(); + ubo_size = this->pipeline_state.ubo_bindings[ubo.buffer_index].ubo->get_size(); UNUSED_VARS_NDEBUG(ubo_size); /* Use dummy zero buffer if no buffer assigned -- this is an optimization to avoid diff --git a/source/blender/gpu/metal/mtl_shader.mm b/source/blender/gpu/metal/mtl_shader.mm index 3f0bcaea875..0ec234519a8 100644 --- a/source/blender/gpu/metal/mtl_shader.mm +++ b/source/blender/gpu/metal/mtl_shader.mm @@ -836,7 +836,7 @@ MTLRenderPipelineStateInstance *MTLShader::bake_pipeline_state( * However, it is more efficient to simply offset the uniform buffer base index to the * maximal number of VBO bind-points, as then UBO bind-points for similar draw calls * will align and avoid the requirement for additional binding. */ - int MTL_uniform_buffer_base_index = GPU_BATCH_VBO_MAX_LEN; + int MTL_uniform_buffer_base_index = pipeline_descriptor.vertex_descriptor.num_vert_buffers + 1; /* Null buffer index is used if an attribute is not found in the * bound VBOs #VertexFormat. */ @@ -989,11 +989,15 @@ MTLRenderPipelineStateInstance *MTLShader::bake_pipeline_state( /* Transform feedback constant. * Ensure buffer is placed after existing buffers, including default buffers. */ - int MTL_transform_feedback_buffer_index = (this->transform_feedback_type_ != - GPU_SHADER_TFB_NONE) ? - MTL_uniform_buffer_base_index + - mtl_interface->get_max_ubo_index() + 2 : - -1; + int MTL_transform_feedback_buffer_index = -1; + if (this->transform_feedback_type_ != GPU_SHADER_TFB_NONE) { + /* If using argument buffers, insert index after argument buffer index. Otherwise, insert + * after uniform buffer bindings. */ + MTL_transform_feedback_buffer_index = + (mtl_interface->uses_argument_buffer_for_samplers()) ? + (mtl_interface->get_argument_buffer_bind_index(ShaderStage::VERTEX) + 1) : + (MTL_uniform_buffer_base_index + mtl_interface->get_max_ubo_index() + 2); + } if (this->transform_feedback_type_ != GPU_SHADER_TFB_NONE) { [values setConstantValue:&MTL_transform_feedback_buffer_index @@ -1121,6 +1125,28 @@ MTLRenderPipelineStateInstance *MTLShader::bake_pipeline_state( desc.depthAttachmentPixelFormat = pipeline_descriptor.depth_attachment_format; desc.stencilAttachmentPixelFormat = pipeline_descriptor.stencil_attachment_format; + /* Bind-point range validation. + * We need to ensure that the PSO will have valid bind-point ranges, or is using the + * appropriate bindless fallback path if any bind limits are exceeded. */ +#ifdef NDEBUG + /* Ensure UBO and PushConstantBlock bindings are within range. */ + BLI_assert_msg((MTL_uniform_buffer_base_index + get_max_ubo_index() + 2) < + MTL_MAX_BUFFER_BINDINGS, + "UBO bindings exceed the fragment bind table limit."); + + /* Transform feedback buffer. */ + if (transform_feedback_type_ != GPU_SHADER_TFB_NONE) { + BLI_assert_msg(MTL_transform_feedback_buffer_index < MTL_MAX_BUFFER_BINDINGS, + "Transform feedback buffer binding exceeds the fragment bind table limit."); + } + + /* Argument buffer. */ + if (mtl_interface->uses_argument_buffer_for_samplers()) { + BLI_assert_msg(mtl_interface->get_argument_buffer_bind_index() < MTL_MAX_BUFFER_BINDINGS, + "Argument buffer binding exceeds the fragment bind table limit."); + } +#endif + /* Compile PSO */ MTLAutoreleasedRenderPipelineReflection reflection_data; id pso = [ctx->device diff --git a/source/blender/gpu/metal/mtl_shader_generator.hh b/source/blender/gpu/metal/mtl_shader_generator.hh index 84a921a852b..c52d39ba348 100644 --- a/source/blender/gpu/metal/mtl_shader_generator.hh +++ b/source/blender/gpu/metal/mtl_shader_generator.hh @@ -205,6 +205,7 @@ struct MSLUniformBlock { std::string name; ShaderStage stage; bool is_array; + uint slot; bool operator==(const MSLUniformBlock &right) const { @@ -418,6 +419,9 @@ class MSLGeneratorInterface { /* Parameters. */ shader::DepthWrite depth_write; + /* Bind index trackers. */ + int max_ubo_slot = -1; + /* Shader buffer bind indices for argument buffers per shader stage. * NOTE: Compute stage will re-use index 0. */ int sampler_argument_buffer_bind_index[3] = {-1, -1, -1}; diff --git a/source/blender/gpu/metal/mtl_shader_generator.mm b/source/blender/gpu/metal/mtl_shader_generator.mm index 0b866456ffc..5342a72f61b 100644 --- a/source/blender/gpu/metal/mtl_shader_generator.mm +++ b/source/blender/gpu/metal/mtl_shader_generator.mm @@ -1779,6 +1779,12 @@ void MSLGeneratorInterface::prepare_from_createinfo(const shader::ShaderCreateIn BLI_assert(res.uniformbuf.name.size() > 0); int64_t array_offset = res.uniformbuf.name.find_first_of("["); + /* UBO should either use an existing declared UBO bind slot, or automatically resolve + * index. */ + ubo.slot = (create_info_->auto_resource_location_) ? uniform_blocks.size() : res.slot; + BLI_assert(ubo.slot >= 0 && ubo.slot < MTL_MAX_UNIFORM_BUFFER_BINDINGS); + max_ubo_slot = max_ii(max_ubo_slot, ubo.slot); + ubo.type_name = res.uniformbuf.type_name; ubo.is_array = (array_offset > -1); if (ubo.is_array) { @@ -1872,8 +1878,9 @@ uint32_t MSLGeneratorInterface::get_sampler_argument_buffer_bind_index(ShaderSta if (sampler_argument_buffer_bind_index[get_shader_stage_index(stage)] >= 0) { return sampler_argument_buffer_bind_index[get_shader_stage_index(stage)]; } - sampler_argument_buffer_bind_index[get_shader_stage_index(stage)] = - (this->uniform_blocks.size() + 1); + + /* Sampler argument buffer to follow UBOs and PushConstantBlock. */ + sampler_argument_buffer_bind_index[get_shader_stage_index(stage)] = (max_ubo_slot + 2); return sampler_argument_buffer_bind_index[get_shader_stage_index(stage)]; } @@ -2205,7 +2212,6 @@ void MSLGeneratorInterface::generate_msl_textures_input_string(std::stringstream void MSLGeneratorInterface::generate_msl_uniforms_input_string(std::stringstream &out, ShaderStage stage) { - int ubo_index = 0; for (const MSLUniformBlock &ubo : this->uniform_blocks) { if (bool(ubo.stage & stage)) { /* For literal/existing global types, we do not need the class name-space accessor. */ @@ -2218,9 +2224,8 @@ void MSLGeneratorInterface::generate_msl_uniforms_input_string(std::stringstream * MTL_uniform_buffer_base_index is an offset depending on the number of unique VBOs * bound for the current PSO specialization. */ out << ubo.type_name << "* " << ubo.name << "[[buffer(MTL_uniform_buffer_base_index+" - << (ubo_index + 1) << ")]]"; + << (ubo.slot + 1) << ")]]"; } - ubo_index++; } } @@ -3256,7 +3261,7 @@ MTLShaderInterface *MSLGeneratorInterface::bake_shader_interface(const char *nam this->uniform_blocks[uniform_block].name.c_str(), name_buffer_size, name_buffer_offset), - uniform_block, + this->uniform_blocks[uniform_block].slot, 0, this->uniform_blocks[uniform_block].stage); } -- 2.30.2