GPU: Move UBO binding validation to GL backend

This also make the validation quicker by tracking the currently
bound slots.

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<const char *> 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 <stdio.h>
@@ -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<GLContext *>(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<GLContext *>(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<GLContext *>(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: