From 53a806f6dffb2a778e383a82c3d0cdb6e9d9d552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Foucault?= Date: Tue, 1 Sep 2020 02:41:29 +0200 Subject: GPU: Move UBO binding validation to GL backend This also make the validation quicker by tracking the currently bound slots. --- source/blender/draw/intern/draw_manager_exec.c | 54 ----------------------- source/blender/gpu/intern/gpu_shader_interface.hh | 26 +++++++++++ source/blender/gpu/intern/gpu_shader_private.hh | 5 +++ source/blender/gpu/opengl/gl_batch.cc | 1 + source/blender/gpu/opengl/gl_context.cc | 5 +++ source/blender/gpu/opengl/gl_context.hh | 4 ++ source/blender/gpu/opengl/gl_debug.cc | 31 ++++++++++++- source/blender/gpu/opengl/gl_debug.hh | 7 +++ source/blender/gpu/opengl/gl_immediate.cc | 1 + source/blender/gpu/opengl/gl_shader_interface.cc | 2 + source/blender/gpu/opengl/gl_shader_interface.hh | 2 - source/blender/gpu/opengl/gl_uniform_buffer.cc | 7 +++ source/blender/gpu/opengl/gl_uniform_buffer.hh | 2 + 13 files changed, 90 insertions(+), 57 deletions(-) diff --git a/source/blender/draw/intern/draw_manager_exec.c b/source/blender/draw/intern/draw_manager_exec.c index 5584405c965..1d15548e1ed 100644 --- a/source/blender/draw/intern/draw_manager_exec.c +++ b/source/blender/draw/intern/draw_manager_exec.c @@ -567,58 +567,6 @@ BLI_INLINE void draw_indirect_call(DRWShadingGroup *shgroup, DRWCommandsState *s } } -#ifndef NDEBUG -/** - * Opengl specification is strict on buffer binding. - * - * " If any active uniform block is not backed by a - * sufficiently large buffer object, the results of shader - * execution are undefined, and may result in GL interruption or - * termination. " - Opengl 3.3 Core Specification - * - * For now we only check if the binding is correct. Not the size of - * the bound ubo. - * - * See T55475. - * */ -static bool ubo_bindings_validate(DRWShadingGroup *shgroup) -{ - bool valid = true; -# ifdef DEBUG_UBO_BINDING - /* Check that all active uniform blocks have a non-zero buffer bound. */ - GLint program = 0; - GLint active_blocks = 0; - - glGetIntegerv(GL_CURRENT_PROGRAM, &program); - glGetProgramiv(program, GL_ACTIVE_UNIFORM_BLOCKS, &active_blocks); - - for (uint i = 0; i < active_blocks; i++) { - int binding = 0; - int buffer = 0; - - glGetActiveUniformBlockiv(program, i, GL_UNIFORM_BLOCK_BINDING, &binding); - glGetIntegeri_v(GL_UNIFORM_BUFFER_BINDING, binding, &buffer); - - if (buffer == 0) { - char blockname[64]; - glGetActiveUniformBlockName(program, i, sizeof(blockname), NULL, blockname); - - if (valid) { - printf("Trying to draw with missing UBO binding.\n"); - valid = false; - } - - DRWPass *parent_pass = DRW_memblock_elem_from_handle(DST.vmempool->passes, - &shgroup->pass_handle); - - printf("Pass : %s, Block : %s, Binding %d\n", parent_pass->name, blockname, binding); - } - } -# endif - return valid; -} -#endif - static void draw_update_uniforms(DRWShadingGroup *shgroup, DRWCommandsState *state, bool *use_tfeedback) @@ -688,8 +636,6 @@ static void draw_update_uniforms(DRWShadingGroup *shgroup, } } } - - BLI_assert(ubo_bindings_validate(shgroup)); } BLI_INLINE void draw_select_buffer(DRWShadingGroup *shgroup, diff --git a/source/blender/gpu/intern/gpu_shader_interface.hh b/source/blender/gpu/intern/gpu_shader_interface.hh index 6e1cb342c68..265fe90fc76 100644 --- a/source/blender/gpu/intern/gpu_shader_interface.hh +++ b/source/blender/gpu/intern/gpu_shader_interface.hh @@ -83,12 +83,21 @@ class ShaderInterface { { return input_lookup(inputs_ + attr_len_, ubo_len_, name); } + inline const ShaderInput *ubo_get(const int binding) const + { + return input_lookup(inputs_ + attr_len_, ubo_len_, binding); + } inline const ShaderInput *uniform_get(const char *name) const { return input_lookup(inputs_ + attr_len_ + ubo_len_, uniform_len_, name); } + inline const char *input_name_get(const ShaderInput *input) const + { + return name_buffer_ + input->name_offset; + } + /* Returns uniform location. */ inline int32_t uniform_builtin(const GPUUniformBuiltin builtin) const { @@ -116,6 +125,10 @@ class ShaderInterface { inline const ShaderInput *input_lookup(const ShaderInput *const inputs, const uint inputs_len, const char *name) const; + + inline const ShaderInput *input_lookup(const ShaderInput *const inputs, + const uint inputs_len, + const int binding) const; }; inline const char *ShaderInterface::builtin_uniform_name(GPUUniformBuiltin u) @@ -226,4 +239,17 @@ inline const ShaderInput *ShaderInterface::input_lookup(const ShaderInput *const return NULL; /* not found */ } +inline const ShaderInput *ShaderInterface::input_lookup(const ShaderInput *const inputs, + const uint inputs_len, + const int binding) const +{ + /* Simple linear search for now. */ + for (int i = inputs_len - 1; i >= 0; i--) { + if (inputs[i].binding == binding) { + return inputs + i; + } + } + return NULL; /* not found */ +} + } // namespace blender::gpu diff --git a/source/blender/gpu/intern/gpu_shader_private.hh b/source/blender/gpu/intern/gpu_shader_private.hh index dc6941a067e..9c9aa835b97 100644 --- a/source/blender/gpu/intern/gpu_shader_private.hh +++ b/source/blender/gpu/intern/gpu_shader_private.hh @@ -64,6 +64,11 @@ class Shader { virtual void vertformat_from_shader(GPUVertFormat *) const = 0; + inline const char *const name_get(void) const + { + return name; + }; + protected: void print_errors(Span sources, char *log, const char *stage); }; diff --git a/source/blender/gpu/opengl/gl_batch.cc b/source/blender/gpu/opengl/gl_batch.cc index 63547bf190f..bb7d5654efd 100644 --- a/source/blender/gpu/opengl/gl_batch.cc +++ b/source/blender/gpu/opengl/gl_batch.cc @@ -330,6 +330,7 @@ void GLBatch::bind(int i_first) void GLBatch::draw(int v_first, int v_count, int i_first, int i_count) { + GL_CHECK_RESOURCES("Batch"); GL_CHECK_ERROR("Batch Pre drawing"); this->bind(i_first); diff --git a/source/blender/gpu/opengl/gl_context.cc b/source/blender/gpu/opengl/gl_context.cc index 666f2b4756a..a3b060abf0e 100644 --- a/source/blender/gpu/opengl/gl_context.cc +++ b/source/blender/gpu/opengl/gl_context.cc @@ -36,6 +36,7 @@ #include "gl_debug.hh" #include "gl_immediate.hh" #include "gl_state.hh" +#include "gl_uniform_buffer.hh" #include "gl_backend.hh" /* TODO remove */ #include "gl_context.hh" @@ -146,6 +147,10 @@ void GLContext::activate(void) back_right->size_set(w, h); } } + + /* Not really following the state but we should consider + * no ubo bound when activating a context. */ + bound_ubo_slots = 0; } void GLContext::deactivate(void) diff --git a/source/blender/gpu/opengl/gl_context.hh b/source/blender/gpu/opengl/gl_context.hh index 99076fa3d1e..fa1c6b9918e 100644 --- a/source/blender/gpu/opengl/gl_context.hh +++ b/source/blender/gpu/opengl/gl_context.hh @@ -52,6 +52,10 @@ class GLSharedOrphanLists { }; class GLContext : public GPUContext { + public: + /** Used for debugging purpose. Bitflags of all bound slots. */ + uint16_t bound_ubo_slots; + /* TODO(fclem) these needs to become private. */ public: /** VBO for missing vertex attrib binding. Avoid undefined behavior on some implementation. */ diff --git a/source/blender/gpu/opengl/gl_debug.cc b/source/blender/gpu/opengl/gl_debug.cc index 2dc0835adfb..fb3c4f52fd0 100644 --- a/source/blender/gpu/opengl/gl_debug.cc +++ b/source/blender/gpu/opengl/gl_debug.cc @@ -30,6 +30,9 @@ #include "glew-mx.h" +#include "gl_context.hh" +#include "gl_uniform_buffer.hh" + #include "gl_debug.hh" #include @@ -140,7 +143,8 @@ void init_gl_callbacks(void) /* -------------------------------------------------------------------- */ /** \name Error Checking * - * This is only useful for implementation that does not support the KHR_debug extension. + * This is only useful for implementation that does not support the KHR_debug extension OR when the + * implementations do not report any errors even when clearly doing shady things. * \{ */ void check_gl_error(const char *info) @@ -173,6 +177,31 @@ void check_gl_error(const char *info) } } +void check_gl_resources(const char *info) +{ + GLContext *ctx = static_cast(GPU_context_active_get()); + ShaderInterface *interface = ctx->shader->interface; + /* NOTE: This only check binding. To be valid, the bound ubo needs to + * be big enough to feed the data range the shader awaits. */ + uint16_t ubo_needed = interface->enabled_ubo_mask_; + ubo_needed &= ~ctx->bound_ubo_slots; + + if (ubo_needed == 0) { + return; + } + + for (int i = 0; ubo_needed != 0; i++, ubo_needed >>= 1) { + if ((ubo_needed & 1) != 0) { + const ShaderInput *ubo_input = interface->ubo_get(i); + const char *ubo_name = interface->input_name_get(ubo_input); + const char *sh_name = ctx->shader->name_get(); + char msg[256]; + SNPRINTF(msg, "Missing UBO bind at slot %d : %s > %s : %s", i, sh_name, ubo_name, info); + debug_callback(0, GL_DEBUG_TYPE_ERROR, 0, GL_DEBUG_SEVERITY_HIGH, 0, msg, NULL); + } + } +} + /** \} */ } // namespace blender::gpu::debug \ No newline at end of file diff --git a/source/blender/gpu/opengl/gl_debug.hh b/source/blender/gpu/opengl/gl_debug.hh index 47c9558f13a..dd98505ebc1 100644 --- a/source/blender/gpu/opengl/gl_debug.hh +++ b/source/blender/gpu/opengl/gl_debug.hh @@ -31,7 +31,14 @@ namespace debug { # define GL_CHECK_ERROR(info) #endif +#ifdef DEBUG +# define GL_CHECK_RESOURCES(info) debug::check_gl_resources(info) +#else +# define GL_CHECK_RESOURCES(info) +#endif + void check_gl_error(const char *info); +void check_gl_resources(const char *info); void init_gl_callbacks(void); } // namespace debug diff --git a/source/blender/gpu/opengl/gl_immediate.cc b/source/blender/gpu/opengl/gl_immediate.cc index 614b6893347..7f12f41a598 100644 --- a/source/blender/gpu/opengl/gl_immediate.cc +++ b/source/blender/gpu/opengl/gl_immediate.cc @@ -90,6 +90,7 @@ uchar *GLImmediate::begin() /* Does the current buffer have enough room? */ const size_t available_bytes = buffer_size() - buffer_offset(); + GL_CHECK_RESOURCES("Immediate"); GL_CHECK_ERROR("Immediate Pre-Begin"); glBindBuffer(GL_ARRAY_BUFFER, vbo_id()); diff --git a/source/blender/gpu/opengl/gl_shader_interface.cc b/source/blender/gpu/opengl/gl_shader_interface.cc index 423db5c8c97..d611efcd975 100644 --- a/source/blender/gpu/opengl/gl_shader_interface.cc +++ b/source/blender/gpu/opengl/gl_shader_interface.cc @@ -124,6 +124,8 @@ GLShaderInterface::GLShaderInterface(GLuint program) glGetProgramiv(program, GL_ACTIVE_UNIFORMS, &active_uniform_len); uniform_len = active_uniform_len; + BLI_assert(ubo_len <= 16 && "enabled_ubo_mask_ is uint16_t"); + /* Work around driver bug with Intel HD 4600 on Windows 7/8, where * GL_ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH does not work. */ if (attr_len > 0 && max_attr_name_len == 0) { diff --git a/source/blender/gpu/opengl/gl_shader_interface.hh b/source/blender/gpu/opengl/gl_shader_interface.hh index 0b9585aa389..8cac84a5990 100644 --- a/source/blender/gpu/opengl/gl_shader_interface.hh +++ b/source/blender/gpu/opengl/gl_shader_interface.hh @@ -55,8 +55,6 @@ class GLShaderInterface : public ShaderInterface { void ref_add(GLVaoCache *ref); void ref_remove(GLVaoCache *ref); - // bool resource_binding_validate(); - MEM_CXX_CLASS_ALLOC_FUNCS("GLShaderInterface"); }; diff --git a/source/blender/gpu/opengl/gl_uniform_buffer.cc b/source/blender/gpu/opengl/gl_uniform_buffer.cc index 5115034639c..0e0c64e5c60 100644 --- a/source/blender/gpu/opengl/gl_uniform_buffer.cc +++ b/source/blender/gpu/opengl/gl_uniform_buffer.cc @@ -110,6 +110,11 @@ void GLUniformBuf::bind(int slot) slot_ = slot; glBindBufferBase(GL_UNIFORM_BUFFER, slot_, ubo_id_); + +#ifdef DEBUG + BLI_assert(slot < 16); + static_cast(GPU_context_active_get())->bound_ubo_slots |= 1 << slot; +#endif } void GLUniformBuf::unbind(void) @@ -117,6 +122,8 @@ void GLUniformBuf::unbind(void) #ifdef DEBUG /* NOTE: This only unbinds the last bound slot. */ glBindBufferBase(GL_UNIFORM_BUFFER, slot_, 0); + /* Hope that the context did not change. */ + static_cast(GPU_context_active_get())->bound_ubo_slots &= ~(1 << slot_); #endif slot_ = 0; } diff --git a/source/blender/gpu/opengl/gl_uniform_buffer.hh b/source/blender/gpu/opengl/gl_uniform_buffer.hh index b71356c4121..8cd2ab91be9 100644 --- a/source/blender/gpu/opengl/gl_uniform_buffer.hh +++ b/source/blender/gpu/opengl/gl_uniform_buffer.hh @@ -37,7 +37,9 @@ namespace gpu { **/ class GLUniformBuf : public UniformBuf { private: + /** Slot to which this UBO is currently bound. -1 if not bound. */ int slot_ = -1; + /** OpenGL Object handle. */ GLuint ubo_id_ = 0; public: -- cgit v1.2.3