[Mesa-dev] [PATCH 06/24] mesa: implement SPIR-V loading in glShaderBinary
Eduardo Lima
elima at igalia.com
Tue Nov 28 09:41:45 UTC 2017
On 11/27/2017 11:34 PM, Ian Romanick wrote:
> 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.
True, I will fix that.
>> +{
>> + 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.
Ok.
>> +
>> + if (n <= 0)
>> + return;
> _mesa_ShaderBinary already checks n < 0. Maybe just elide the call to
> _mesa_spirv_shader_binary if n == 0?
Ok.
>> +
>> + 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.
I agree with this change, but per Timothy's comments, I have locally
removed the SpirVBinary flag altogether to use nullness of spriv_data
instead.
So it doesn't apply anymore.
>> + 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