[Mesa-dev] [PATCH 06/24] mesa: implement SPIR-V loading in glShaderBinary

Ian Romanick idr at freedesktop.org
Mon Nov 27 22:34:54 UTC 2017


On 11/15/2017 05:22 AM, Eduardo Lima Mitev wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> v2: Add a gl_shader_spirv_data member to gl_shader, which already
>    encapsulates a gl_spirv_module where the binary will be saved.
>    (Eduardo Lima)
> ---
>  src/mesa/main/glspirv.c   | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/mesa/main/glspirv.h   |  5 +++++
>  src/mesa/main/mtypes.h    |  4 ++++
>  src/mesa/main/shaderapi.c | 41 ++++++++++++++++++++++++++++++++++---
>  src/mesa/main/shaderobj.c |  2 ++
>  5 files changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
> index d4724ad245c..390fa9e6984 100644
> --- a/src/mesa/main/glspirv.c
> +++ b/src/mesa/main/glspirv.c
> @@ -23,6 +23,8 @@
>  
>  #include "glspirv.h"
>  
> +#include "errors.h"
> +
>  #include "util/u_atomic.h"
>  
>  void
> @@ -61,6 +63,56 @@ _mesa_shader_spirv_data_reference(struct gl_shader_spirv_data **dest,
>        p_atomic_inc(&src->RefCount);
>  }
>  
> +void
> +_mesa_spirv_shader_binary(struct gl_context *ctx,
> +                          GLint n, struct gl_shader **shaders,
> +                          const void* binary, GLint length)

Since _mesa_ShaderBinary will already check length < 0, make length
size_t.  You can also make n be unsigned or plain int.

> +{
> +   struct gl_spirv_module *module;
> +   struct gl_shader_spirv_data *spirv_data;
> +
> +   if (!ctx->Extensions.ARB_gl_spirv) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glShaderBinary(SPIR-V)");
> +      return;
> +   }

I think this error should get moved to _mesa_ShaderBinary with the rest
of the API error checks.

> +
> +   if (n <= 0)
> +      return;

_mesa_ShaderBinary already checks n < 0.  Maybe just elide the call to
_mesa_spirv_shader_binary if n == 0?

> +
> +   assert(length >= 0);
> +
> +   module = malloc(sizeof(*module) + (size_t)length);
> +   if (!module) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary");
> +      return;
> +   }
> +
> +   p_atomic_set(&module->RefCount, 0);
> +   module->Length = length;
> +   memcpy(&module->Binary[0], binary, length);
> +
> +   for (int i = 0; i < n; ++i) {
> +      struct gl_shader *sh = shaders[i];
> +
> +      spirv_data = rzalloc(NULL, struct gl_shader_spirv_data);
> +      _mesa_shader_spirv_data_reference(&sh->spirv_data, spirv_data);
> +      _mesa_spirv_module_reference(&spirv_data->SpirVModule, module);
> +
> +      sh->SpirVBinary = GL_TRUE;

It didn't occur to me while reviewing the earlier patch, but if
SpirVBinary is never visible to the API, we should use bool, true, and
false instead of the GL types.

> +      sh->CompileStatus = compile_failure;
> +
> +      free((void *)sh->Source);
> +      sh->Source = NULL;
> +      free((void *)sh->FallbackSource);
> +      sh->FallbackSource = NULL;
> +
> +      ralloc_free(sh->ir);
> +      sh->ir = NULL;
> +      ralloc_free(sh->symbols);
> +      sh->symbols = NULL;
> +   }
> +}
> +
>  void GLAPIENTRY
>  _mesa_SpecializeShaderARB(GLuint shader,
>                            const GLchar *pEntryPoint,
> diff --git a/src/mesa/main/glspirv.h b/src/mesa/main/glspirv.h
> index b8a0125ea9f..5ffaa230629 100644
> --- a/src/mesa/main/glspirv.h
> +++ b/src/mesa/main/glspirv.h
> @@ -71,6 +71,11 @@ void
>  _mesa_shader_spirv_data_reference(struct gl_shader_spirv_data **dest,
>                                    struct gl_shader_spirv_data *src);
>  
> +void
> +_mesa_spirv_shader_binary(struct gl_context *ctx,
> +                          GLint n, struct gl_shader **shaders,
> +                          const void* binary, GLint length);
> +
>  /**
>   * \name API functions
>   */
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index cfc763f043e..f19477eb027 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -88,6 +88,7 @@ struct st_context;
>  struct gl_uniform_storage;
>  struct prog_instruction;
>  struct gl_program_parameter_list;
> +struct gl_shader_spirv_data;
>  struct set;
>  struct set_entry;
>  struct vbo_context;
> @@ -2637,6 +2638,9 @@ struct gl_shader
>     GLuint TransformFeedbackBufferStride[MAX_FEEDBACK_BUFFERS];
>  
>     struct gl_shader_info info;
> +
> +   /* ARB_gl_spirv related data */
> +   struct gl_shader_spirv_data *spirv_data;
>  };
>  
>  
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 9090bee9fb0..240d9fe6cbc 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -42,6 +42,7 @@
>  #include "main/context.h"
>  #include "main/dispatch.h"
>  #include "main/enums.h"
> +#include "main/glspirv.h"
>  #include "main/hash.h"
>  #include "main/mtypes.h"
>  #include "main/pipelineobj.h"
> @@ -1054,6 +1055,17 @@ set_shader_source(struct gl_shader *sh, const GLchar *source)
>  {
>     assert(sh);
>  
> +   /* The GL_ARB_gl_spirv spec adds the following to the end of the description
> +    * of ShaderSource:
> +    *
> +    *   "If <shader> was previously associated with a SPIR-V module (via the
> +    *    ShaderBinary command), that association is broken. Upon successful
> +    *    completion of this command the SPIR_V_BINARY_ARB state of <shader>
> +    *    is set to FALSE."
> +    */
> +   _mesa_shader_spirv_data_reference(&sh->spirv_data, NULL);
> +   sh->SpirVBinary = GL_FALSE;
> +
>     if (sh->CompileStatus == compile_skipped && !sh->FallbackSource) {
>        /* If shader was previously compiled back-up the source in case of cache
>         * fallback.
> @@ -2147,9 +2159,7 @@ _mesa_ShaderBinary(GLint n, const GLuint* shaders, GLenum binaryformat,
>                     const void* binary, GLint length)
>  {
>     GET_CURRENT_CONTEXT(ctx);
> -   (void) shaders;
> -   (void) binaryformat;
> -   (void) binary;
> +   struct gl_shader **sh;
>  
>     /* Page 68, section 7.2 'Shader Binaries" of the of the OpenGL ES 3.1, and
>      * page 88 of the OpenGL 4.5 specs state:
> @@ -2163,6 +2173,31 @@ _mesa_ShaderBinary(GLint n, const GLuint* shaders, GLenum binaryformat,
>        return;
>     }
>  
> +   /* Get all shader objects at once so we can make the operation
> +    * all-or-nothing.
> +    */
> +   if (n > SIZE_MAX / sizeof(*sh)) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary(count)");
> +      return;
> +   }
> +
> +   sh = alloca(sizeof(*sh) * (size_t)n);
> +   if (!sh) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary");
> +      return;
> +   }
> +
> +   for (int i = 0; i < n; ++i) {
> +      sh[i] = _mesa_lookup_shader_err(ctx, shaders[i], "glShaderBinary");
> +      if (!sh[i])
> +         return;
> +   }
> +
> +   if (binaryformat == GL_SHADER_BINARY_FORMAT_SPIR_V_ARB) {
> +      _mesa_spirv_shader_binary(ctx, n, sh, binary, length);
> +      return;
> +   }
> +
>     _mesa_error(ctx, GL_INVALID_ENUM, "glShaderBinary(format)");
>  }
>  
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index ce2e3df4fae..5c1cdd6b27a 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -33,6 +33,7 @@
>  #include "compiler/glsl/string_to_uint_map.h"
>  #include "main/glheader.h"
>  #include "main/context.h"
> +#include "main/glspirv.h"
>  #include "main/hash.h"
>  #include "main/mtypes.h"
>  #include "main/shaderapi.h"
> @@ -121,6 +122,7 @@ _mesa_new_shader(GLuint name, gl_shader_stage stage)
>  void
>  _mesa_delete_shader(struct gl_context *ctx, struct gl_shader *sh)
>  {
> +   _mesa_shader_spirv_data_reference(&sh->spirv_data, NULL);
>     free((void *)sh->Source);
>     free((void *)sh->FallbackSource);
>     free(sh->Label);
> 



More information about the mesa-dev mailing list