GPU: Added image format validation for shader image bindings

The OpenGL specs require that the storage image qualifier in shaders
(e.g., "rgba32f") needs to be compatible with the format of a bound image
(see https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf#page=318).

We know that Blender currently does not handle this correctly in
multiple places. AMD and NVIDIA seem to silently ignore a mismatch and
just seem to use the format of the bound image. However, for the
Intel Windows drivers, this seems to lead to visual corruptions
(#141436, #141173). While a more graceful handling of a mismatch may
be nice, this is in line with the OpenGL specs.

This PR adds code for validating image formats for bindings.

Pull Request: https://projects.blender.org/blender/blender/pulls/143791

source/blender/gpu/intern/gpu_batch.cc

@@ -20,6 +20,7 @@
 #include "GPU_vertex_buffer.hh"
 #include "gpu_backend.hh"
 #include "gpu_context_private.hh"
+#include "gpu_debug_private.hh"
 #include "gpu_shader_private.hh"
 
 #include <cstring>
@@ -502,6 +503,10 @@ void GPU_batch_draw_advanced(
     return;
   }
 
+#ifndef NDEBUG
+  debug_validate_binding_image_format();
+#endif
+
   batch->draw(vertex_first, vertex_count, instance_first, instance_count);
 }
 
@@ -512,6 +517,10 @@ void GPU_batch_draw_indirect(Batch *batch, blender::gpu::StorageBuf *indirect_bu
   BLI_assert(Context::get()->shader != nullptr);
   Context::get()->assert_framebuffer_shader_compatibility(Context::get()->shader);
 
+#ifndef NDEBUG
+  debug_validate_binding_image_format();
+#endif
+
   batch->draw_indirect(indirect_buf, offset);
 }
 
@@ -526,6 +535,10 @@ void GPU_batch_multi_draw_indirect(Batch *batch,
   BLI_assert(Context::get()->shader != nullptr);
   Context::get()->assert_framebuffer_shader_compatibility(Context::get()->shader);
 
+#ifndef NDEBUG
+  debug_validate_binding_image_format();
+#endif
+
   batch->multi_draw_indirect(indirect_buf, count, offset, stride);
 }
 

source/blender/gpu/intern/gpu_compute.cc

@@ -9,6 +9,7 @@
 #include "GPU_compute.hh"
 
 #include "gpu_backend.hh"
+#include "gpu_debug_private.hh"
 
 void GPU_compute_dispatch(blender::gpu::Shader *shader,
                           uint groups_x_len,
@@ -18,6 +19,9 @@ void GPU_compute_dispatch(blender::gpu::Shader *shader,
 {
   blender::gpu::GPUBackend &gpu_backend = *blender::gpu::GPUBackend::get();
   GPU_shader_bind(shader, constants_state);
+#ifndef NDEBUG
+  blender::gpu::debug_validate_binding_image_format();
+#endif
   gpu_backend.compute_dispatch(groups_x_len, groups_y_len, groups_z_len);
 }
 
@@ -31,5 +35,8 @@ void GPU_compute_dispatch_indirect(
       indirect_buf_);
 
   GPU_shader_bind(shader, constants_state);
+#ifndef NDEBUG
+  blender::gpu::debug_validate_binding_image_format();
+#endif
   gpu_backend.compute_dispatch_indirect(indirect_buf);
 }

source/blender/gpu/intern/gpu_debug.cc

@@ -164,3 +164,39 @@ void GPU_debug_capture_scope_end(void *scope)
   /* Declare end of capture scope region. */
   ctx->debug_capture_scope_end(scope);
 }
+
+namespace blender::gpu {
+
+void debug_validate_binding_image_format()
+{
+  if (!(G.debug & G_DEBUG_GPU)) {
+    return;
+  }
+
+  const auto &texture_formats_state = Context::get()->state_manager->image_formats;
+  const auto &texture_formats_shader = Context::get()->shader->interface->image_formats_;
+  for (int image_unit = 0; image_unit < GPU_MAX_IMAGE; image_unit++) {
+    TextureWriteFormat format_state = texture_formats_state[image_unit];
+    TextureWriteFormat format_shader = texture_formats_shader[image_unit];
+    if (format_state != TextureWriteFormat::Invalid &&
+        format_shader == TextureWriteFormat::Invalid)
+    {
+      /* It is allowed for an image to be bound in the state manager but to be unused in the
+       * shader. */
+      continue;
+    }
+    if (UNLIKELY(texture_formats_shader[image_unit] != texture_formats_state[image_unit])) {
+      fprintf(
+          stderr,
+          "Error in GPU_debug_validate_binding_image_format: Image format mismatch detected for "
+          "shader '%s' at binding %d (shader format '%s' vs. bound texture format '%s').\n",
+          Context::get()->shader->name_get().c_str(),
+          image_unit,
+          GPU_texture_format_name(to_texture_format(texture_formats_shader[image_unit])),
+          GPU_texture_format_name(to_texture_format(texture_formats_state[image_unit])));
+      BLI_assert_unreachable();
+    }
+  }
+}
+
+}  // namespace blender::gpu

source/blender/gpu/intern/gpu_debug_private.hh

@@ -17,4 +17,6 @@ namespace blender::gpu {
 
 using DebugStack = Vector<StringRef>;
 
+void debug_validate_binding_image_format();
+
 }  // namespace blender::gpu

source/blender/gpu/intern/gpu_shader_interface.cc

@@ -13,12 +13,16 @@
 #include "BLI_span.hh"
 #include "BLI_vector.hh"
 
+#include "gpu_shader_create_info.hh"
 #include "gpu_shader_interface.hh"
 
 namespace blender::gpu {
 
 /* TODO(fclem): add unique ID for debugging. */
-ShaderInterface::ShaderInterface() = default;
+ShaderInterface::ShaderInterface()
+{
+  image_formats_.fill(TextureWriteFormat::Invalid);
+}
 
 ShaderInterface::~ShaderInterface()
 {
@@ -68,6 +72,25 @@ void ShaderInterface::sort_inputs()
   offset += constant_len_;
 }
 
+void ShaderInterface::set_image_formats_from_info(const shader::ShaderCreateInfo &info)
+{
+  for (const shader::ShaderCreateInfo::Resource &res : info.pass_resources_) {
+    if (res.bind_type == shader::ShaderCreateInfo::Resource::BindType::IMAGE) {
+      image_formats_[res.slot] = TextureWriteFormat(res.image.format);
+    }
+  }
+  for (const shader::ShaderCreateInfo::Resource &res : info.batch_resources_) {
+    if (res.bind_type == shader::ShaderCreateInfo::Resource::BindType::IMAGE) {
+      image_formats_[res.slot] = TextureWriteFormat(res.image.format);
+    }
+  }
+  for (const shader::ShaderCreateInfo::Resource &res : info.geometry_resources_) {
+    if (res.bind_type == shader::ShaderCreateInfo::Resource::BindType::IMAGE) {
+      image_formats_[res.slot] = TextureWriteFormat(res.image.format);
+    }
+  }
+}
+
 void ShaderInterface::debug_print() const
 {
   Span<ShaderInput> attrs = Span<ShaderInput>(inputs_, attr_len_);

source/blender/gpu/intern/gpu_shader_interface.hh

@@ -18,9 +18,11 @@
 #include "BLI_hash.h"
 #include "BLI_sys_types.h"
 
+#include "GPU_format.hh"
 #include "GPU_shader.hh"
 #include "GPU_vertex_format.hh" /* GPU_VERT_ATTR_MAX_LEN */
 #include "gpu_shader_create_info.hh"
+#include "gpu_texture_private.hh"
 
 namespace blender::gpu {
 
@@ -77,6 +79,9 @@ class ShaderInterface {
    */
   uint8_t attr_types_[GPU_VERT_ATTR_MAX_LEN];
 
+  /* Formats of all image units. */
+  std::array<TextureWriteFormat, GPU_MAX_IMAGE> image_formats_;
+
   ShaderInterface();
   virtual ~ShaderInterface();
 
@@ -161,6 +166,8 @@ class ShaderInterface {
    */
   void sort_inputs();
 
+  void set_image_formats_from_info(const shader::ShaderCreateInfo &info);
+
  private:
   inline const ShaderInput *input_lookup(const ShaderInput *const inputs,
                                          uint inputs_len,

source/blender/gpu/intern/gpu_state.cc

@@ -380,6 +380,8 @@ StateManager::StateManager()
   mutable_state.stencil_write_mask = 0x00;
   mutable_state.stencil_compare_mask = 0x00;
   mutable_state.stencil_reference = 0x00;
+
+  image_formats.fill(TextureWriteFormat::Invalid);
 }
 
 /** \} */

source/blender/gpu/intern/gpu_state_private.hh

@@ -141,6 +141,9 @@ class StateManager {
   GPUState state;
   GPUStateMutable mutable_state;
 
+  /* Formats of all image units. */
+  std::array<TextureWriteFormat, GPU_MAX_IMAGE> image_formats;
+
   StateManager();
   virtual ~StateManager() = default;
 

source/blender/gpu/intern/gpu_texture.cc

@@ -891,6 +891,7 @@ const char *GPU_texture_format_name(TextureFormat texture_format)
 
     case TextureFormat::Invalid:
       BLI_assert_unreachable();
+      return "Invalid";
   }
   BLI_assert_unreachable();
   return "";

source/blender/gpu/intern/gpu_texture_private.hh

@@ -73,6 +73,9 @@ ENUM_OPERATORS(eGPUSamplerFormat, GPU_SAMPLER_TYPE_UINT)
 #  define DEBUG_NAME_LEN 8
 #endif
 
+/* Maximum number of image units. */
+#define GPU_MAX_IMAGE 8
+
 /* Maximum number of FBOs a texture can be attached to. */
 #define GPU_TEX_MAX_FBO_ATTACHED 32
 

source/blender/gpu/metal/mtl_context.hh

@@ -850,7 +850,7 @@ class MTLContext : public Context {
   /* Context Global-State Texture Binding. */
   void texture_bind(gpu::MTLTexture *mtl_texture, uint texture_unit, bool is_image);
   void sampler_bind(MTLSamplerState, uint sampler_unit);
-  void texture_unbind(gpu::MTLTexture *mtl_texture, bool is_image);
+  void texture_unbind(gpu::MTLTexture *mtl_texture, bool is_image, StateManager *state_manager);
   void texture_unbind_all(bool is_image);
   void sampler_state_cache_init();
   id<MTLSamplerState> get_sampler_from_state(MTLSamplerState state);

source/blender/gpu/metal/mtl_context.mm

@@ -2408,7 +2408,9 @@ void MTLContext::sampler_bind(MTLSamplerState sampler_state, uint sampler_unit)
   this->pipeline_state.sampler_bindings[sampler_unit] = {true, sampler_state};
 }
 
-void MTLContext::texture_unbind(gpu::MTLTexture *mtl_texture, bool is_image)
+void MTLContext::texture_unbind(gpu::MTLTexture *mtl_texture,
+                                bool is_image,
+                                StateManager *state_manager)
 {
   BLI_assert(mtl_texture);
 
@@ -2422,6 +2424,9 @@ void MTLContext::texture_unbind(gpu::MTLTexture *mtl_texture, bool is_image)
     if (resource_bind_table[i].texture_resource == mtl_texture) {
       resource_bind_table[i].texture_resource = nullptr;
       resource_bind_table[i].used = false;
+      if (is_image) {
+        state_manager->image_formats[i] = TextureWriteFormat::Invalid;
+      }
     }
   }
 

source/blender/gpu/metal/mtl_shader_interface.mm

@@ -403,6 +403,9 @@ void MTLShaderInterface::prepare_common_shader_inputs(const shader::ShaderCreate
       current_input++;
     }
   }
+  if (info != nullptr) {
+    set_image_formats_from_info(*info);
+  }
 
   /* SSBO bindings. */
   BLI_assert(&inputs_[attr_len_ + ubo_len_ + uniform_len_] >= current_input);

source/blender/gpu/metal/mtl_state.mm

@@ -666,7 +666,7 @@ void MTLStateManager::texture_unbind(Texture *tex_)
   gpu::MTLTexture *mtl_tex = static_cast<gpu::MTLTexture *>(tex_);
   BLI_assert(mtl_tex);
   MTLContext *ctx = MTLContext::get();
-  ctx->texture_unbind(mtl_tex, false);
+  ctx->texture_unbind(mtl_tex, false, this);
 }
 
 void MTLStateManager::texture_unbind_all()
@@ -691,6 +691,7 @@ void MTLStateManager::image_bind(Texture *tex_, int unit)
   MTLContext *ctx = MTLContext::get();
   if (unit >= 0) {
     ctx->texture_bind(mtl_tex, unit, true);
+    image_formats[unit] = TextureWriteFormat(tex_->format_get());
   }
 }
 
@@ -700,7 +701,7 @@ void MTLStateManager::image_unbind(Texture *tex_)
   gpu::MTLTexture *mtl_tex = static_cast<gpu::MTLTexture *>(tex_);
   BLI_assert(mtl_tex);
   MTLContext *ctx = MTLContext::get();
-  ctx->texture_unbind(mtl_tex, true);
+  ctx->texture_unbind(mtl_tex, true, this);
 }
 
 void MTLStateManager::image_unbind_all()
@@ -708,6 +709,7 @@ void MTLStateManager::image_unbind_all()
   MTLContext *ctx = MTLContext::get();
   BLI_assert(ctx);
   ctx->texture_unbind_all(true);
+  image_formats.fill(TextureWriteFormat::Invalid);
 }
 
 /** \} */

source/blender/gpu/opengl/gl_shader_interface.cc

@@ -493,6 +493,7 @@ GLShaderInterface::GLShaderInterface(GLuint program, const shader::ShaderCreateI
     input->binding = -1;
     input++;
   }
+  set_image_formats_from_info(info);
 
   /* SSBOs */
   for (const ShaderCreateInfo::Resource &res : all_resources) {

source/blender/gpu/opengl/gl_state.cc

@@ -582,6 +582,7 @@ void GLStateManager::image_bind(Texture *tex_, int unit)
   }
   images_[unit] = tex->tex_id_;
   formats_[unit] = to_gl_internal_format(tex->format_);
+  image_formats[unit] = TextureWriteFormat(tex->format_get());
   tex->is_bound_image_ = true;
   dirty_image_binds_ |= 1ULL << unit;
 }
@@ -597,6 +598,7 @@ void GLStateManager::image_unbind(Texture *tex_)
   for (int i = 0; i < ARRAY_SIZE(images_); i++) {
     if (images_[i] == tex_id) {
       images_[i] = 0;
+      image_formats[i] = TextureWriteFormat::Invalid;
       dirty_image_binds_ |= 1ULL << i;
     }
   }
@@ -611,6 +613,7 @@ void GLStateManager::image_unbind_all()
       dirty_image_binds_ |= 1ULL << i;
     }
   }
+  image_formats.fill(TextureWriteFormat::Invalid);
   this->image_bind_apply();
 }
 

source/blender/gpu/vulkan/vk_shader_interface.cc

@@ -132,6 +132,7 @@ void VKShaderInterface::init(const shader::ShaderCreateInfo &info)
       input++;
     }
   }
+  set_image_formats_from_info(info);
 
   /* Push constants. */
   int32_t push_constant_location = 1024;

source/blender/gpu/vulkan/vk_state_manager.cc

@@ -74,20 +74,21 @@ void VKStateManager::texture_unbind_all()
 void VKStateManager::image_bind(Texture *tex, int binding)
 {
   VKTexture *texture = unwrap(tex);
-  images_.bind(texture, binding);
+  images_.bind(texture, binding, TextureWriteFormat(tex->format_get()), this);
   is_dirty = true;
 }
 
 void VKStateManager::image_unbind(Texture *tex)
 {
   VKTexture *texture = unwrap(tex);
-  images_.unbind(texture);
+  images_.unbind(texture, this);
   is_dirty = true;
 }
 
 void VKStateManager::image_unbind_all()
 {
   images_.unbind_all();
+  image_formats.fill(TextureWriteFormat::Invalid);
   is_dirty = true;
 }
 
@@ -113,7 +114,7 @@ void VKStateManager::unbind_from_all_namespaces(void *resource)
 {
   uniform_buffers_.unbind(resource);
   storage_buffers_.unbind(resource);
-  images_.unbind(resource);
+  images_.unbind(resource, this);
   textures_.unbind(resource);
   is_dirty = true;
 }

source/blender/gpu/vulkan/vk_state_manager.hh

@@ -72,7 +72,10 @@ template<int Offset> class BindSpaceImages {
  public:
   Vector<VKTexture *> bound_resources;
 
-  void bind(VKTexture *resource, int binding)
+  void bind(VKTexture *resource,
+            int binding,
+            TextureWriteFormat format,
+            StateManager *state_manager)
   {
     if (binding >= Offset) {
       binding -= Offset;
@@ -81,6 +84,7 @@ template<int Offset> class BindSpaceImages {
       bound_resources.resize(binding + 1);
     }
     bound_resources[binding] = resource;
+    state_manager->image_formats[binding] = format;
   }
 
   VKTexture *get(int binding) const
@@ -91,11 +95,12 @@ template<int Offset> class BindSpaceImages {
     return bound_resources[binding];
   }
 
-  void unbind(void *resource)
+  void unbind(void *resource, StateManager *state_manager)
   {
     for (int index : IndexRange(bound_resources.size())) {
       if (bound_resources[index] == resource) {
         bound_resources[index] = nullptr;
+        state_manager->image_formats[index] = TextureWriteFormat::Invalid;
       }
     }
   }