[Mesa-dev] [PATCH v2 05/25] mesa: implement SPIR-V loading in glShaderBinary
Ian Romanick
idr at freedesktop.org
Thu Nov 30 23:57:51 UTC 2017
Two nits below...
On 11/30/2017 09:28 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)
>
> * Just use the 'spirv_data' member to know whether a gl_shader has
> the SPIR_V_BINARY_ARB state. (Timothy Arceri)
>
> * Remove redundant argument checks. Move extension presence check
> to API entry point where the rest of checks are. Retype 'n' and
> 'length'arguments to use the correct and more standard types.
> (Ian Romanick)
> ---
> src/mesa/main/glspirv.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> src/mesa/main/glspirv.h | 5 +++++
> src/mesa/main/mtypes.h | 4 ++++
> src/mesa/main/shaderapi.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> src/mesa/main/shaderobj.c | 2 ++
> 5 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
> index 8d1e652e088..d2e76bb1927 100644
> --- a/src/mesa/main/glspirv.c
> +++ b/src/mesa/main/glspirv.c
> @@ -25,6 +25,8 @@
>
> #include "errors.h"
>
> +#include "errors.h"
> +
> #include "util/u_atomic.h"
>
> void
> @@ -59,6 +61,47 @@ _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,
> + unsigned n, struct gl_shader **shaders,
> + const void* binary, size_t length)
> +{
> + struct gl_spirv_module *module;
> + struct gl_shader_spirv_data *spirv_data;
> +
> + assert(length >= 0);
> +
> + module = malloc(sizeof(*module) + (size_t)length);
Don't need the (size_t) because you made length be size_t. :)
> + 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->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..ba281f68bef 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,
> + unsigned n, struct gl_shader **shaders,
> + const void* binary, size_t length);
> +
> /**
> * \name API functions
> */
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 062eea609c7..50a47e0a65d 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -98,6 +98,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;
> @@ -2646,6 +2647,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 72824355838..24058e5ee2e 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"
> @@ -1051,6 +1052,16 @@ 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);
> +
> if (sh->CompileStatus == compile_skipped && !sh->FallbackSource) {
> /* If shader was previously compiled back-up the source in case of cache
> * fallback.
> @@ -2132,9 +2143,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:
> @@ -2148,6 +2157,36 @@ _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) {
> + if (!ctx->Extensions.ARB_gl_spirv)
> + _mesa_error(ctx, GL_INVALID_OPERATION, "glShaderBinary(SPIR-V)");
> + else if (n > 0)
> + _mesa_spirv_shader_binary(ctx, (unsigned) n, sh, binary,
> + (size_t) length);
This block should get enclosed in { } because it's more than one line.
With those fixed, this patch is
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> +
> + 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