Re: [Mesa-dev] [PATCH 06/24] mesa: implement SPIR-V loading in glShaderBinary
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
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 was previo
Re: [Mesa-dev] [PATCH 06/24] mesa: implement SPIR-V loading in glShaderBinary
On 11/15/2017 05:22 AM, Eduardo Lima Mitev wrote:
> From: Nicolai Hähnle
>
> 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 was previously associated with a SPIR-V mo
[Mesa-dev] [PATCH 06/24] mesa: implement SPIR-V loading in glShaderBinary
From: Nicolai Hähnle
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)
+{
+ 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;
+ }
+
+ if (n <= 0)
+ return;
+
+ 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;
+ 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 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
+*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 OpenG
