On 21/11/17 15:54, Timothy Arceri wrote:
On 21/11/17 15:34, Jordan Justen wrote:On 2017-11-20 19:00:28, Timothy Arceri wrote:I don't think this belongs in util. disk cache is generic and used by both Vulkan and Opengl drivers. These function are specific to one OpenGL extension and have a dependency on OpenGL types. I think you should just move this inside the src/mesa, I can't see this being reused anywhere else. Maybe src/mesa/program ?My goal is just to put it somewhere that all GL/GLES drivers in Mesa will use it. Since we have just the one binary_format value defined, we need to avoid different drivers using the same binary_format enum in different ways. I think I almost put it under src/mesa/main, but I guess src/mesa/program works too. Do you prefer either of those? Maybe it is so simple now that it could just be added into main/shaderobj.c?That file is already a bit of a dumping ground. I like it in the file you have now. Moving it to src/mesa/program makes sense to me.Are any of these ideas likely to push you over the edge for a Reviewed-by? :)Yeah I've skimmed everything else and it seems fine I just need to give it a better look before sending a r-b. I also thought Nicolai might want to take a look over these last few since he gave feedback on these in v1. I'll try take a better look later tonight or tomorrow morning. I'm keen to seem them land also so I can hook up Gallium support.Thanks, -JordanOn 21/11/17 09:27, Jordan Justen wrote:Signed-off-by: Jordan Justen <[email protected]> --- src/util/Makefile.sources | 2 + src/util/meson.build | 2 +src/util/program_binary.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++src/util/program_binary.h | 70 ++++++++++++++++++++++ 4 files changed, 223 insertions(+) create mode 100644 src/util/program_binary.c create mode 100644 src/util/program_binary.h diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources index 104ecae8ed3..e06f631128a 100644 --- a/src/util/Makefile.sources +++ b/src/util/Makefile.sources @@ -24,6 +24,8 @@ MESA_UTIL_FILES := \ mesa-sha1.h \ os_time.c \ os_time.h \ + program_binary.c \ + program_binary.h \ sha1/sha1.c \ sha1/sha1.h \ ralloc.c \ diff --git a/src/util/meson.build b/src/util/meson.build index ac86c9e111e..496da5c4328 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -48,6 +48,8 @@ files_mesa_util = files( 'mesa-sha1.h', 'os_time.c', 'os_time.h', + 'program_binary.c', + 'program_binary.h', 'sha1/sha1.c', 'sha1/sha1.h', 'ralloc.c', diff --git a/src/util/program_binary.c b/src/util/program_binary.c new file mode 100644 index 00000000000..e10b9f17862 --- /dev/null +++ b/src/util/program_binary.c @@ -0,0 +1,149 @@ +/* + * Mesa 3-D graphics library + * + * Copyright (c) 2017 Intel Corporation + *+ * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions:+ *+ * The above copyright notice and this permission notice shall be included+ * in all copies or substantial portions of the Software. + *+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR+ * OTHER DEALINGS IN THE SOFTWARE. + */ + +/** + * \file program_binary.c + * + * Helper functions for serializing a binary program. + */ + + +#include "main/mtypes.h" +#include "crc32.h" +#include "program_binary.h" + +/**+ * Mesa supports one binary format, but it must differentiate between formats+ * produced by different drivers and different Mesa versions. + *+ * Mesa uses a uint32_t value to specify an internal format. The only format + * defined has one uint32_t value of 0, followed by 20 bytes specifying a sha1+ * that uniquely identifies the Mesa driver type and version. + */ + +struct program_binary_header {+ /* If internal_format is 0, it must be followed by the 20 byte sha1 that + * identifies the Mesa driver and version supported. If we want to support + * something besides a sha1, then a new internal_format value can be added.+ */ + uint32_t internal_format; + uint8_t sha1[20];+ /* Fields following sha1 can be changed since the sha1 will guarantee that+ * the binary only works with the same Mesa version. + */ + uint32_t size; + uint32_t crc32; +}; + +unsigned +get_program_binary_header_size(void) +{ + return sizeof(struct program_binary_header); +} + +bool +write_program_binary(const void *payload, unsigned payload_size,+ const void *sha1, void *binary, unsigned binary_size,+ GLenum *binary_format) +{ + struct program_binary_header *hdr = binary; + + if (binary_size < sizeof(*hdr)) + return false; +
Can we add a comment here, looking at this code alone I had no idea why payload size could be bigger than the binary size, something like:
/* binary_size is the size of the buffer provided by the application. * Make sure our program (payload) will fit in the buffer. */
+ if (payload_size > binary_size - sizeof(*hdr)) + return false; + + hdr->internal_format = 0; + memcpy(hdr->sha1, sha1, sizeof(hdr->sha1)); + memcpy(hdr + 1, payload, payload_size); + hdr->size = payload_size; + + hdr->crc32 = util_hash_crc32(hdr + 1, payload_size); + *binary_format = GL_PROGRAM_BINARY_FORMAT_MESA; + + return true; +} + +static bool +simple_header_checks(const void *binary, unsigned length) +{ + const struct program_binary_header *hdr = binary; + if (binary == NULL || length < sizeof(*hdr))
For consistency can we make this: if (hdr == NULL || length < sizeof(*hdr))
+ return false; + + if (hdr->internal_format != 0) + return false; + + return true; +} + +static bool +check_crc32(const void *binary, unsigned length) +{ + const struct program_binary_header *hdr = binary; + uint32_t crc32; + unsigned crc32_len; + + crc32_len = hdr->size; + if (crc32_len > length - sizeof(*hdr)) + return false; + + crc32 = util_hash_crc32(hdr + 1, crc32_len); + if (hdr->crc32 != crc32) + return false; + + return true; +} + +static bool +is_program_binary_valid(GLenum binary_format, const void *sha1, + const void *binary, unsigned length) +{ + const struct program_binary_header *hdr = binary; + + if (binary_format != GL_PROGRAM_BINARY_FORMAT_MESA) + return false; + + if (!simple_header_checks(binary, length)) + return false; + + if (memcmp(hdr->sha1, sha1, sizeof(hdr->sha1)) != 0) + return false; + + if (!check_crc32(binary, length)) + return false; + + return true; +} + +const void* +get_program_binary_payload(GLenum binary_format, const void *sha1, + const void *binary, unsigned length) +{ + const struct program_binary_header *hdr = binary; + if (!is_program_binary_valid(binary_format, sha1, binary, length))
Can we stop passing binary and just use hdr here. All three internal functions above simply cast to program_binary_header anyway.
+ return NULL; + return (const uint8_t*)binary + sizeof(*hdr); +} diff --git a/src/util/program_binary.h b/src/util/program_binary.h new file mode 100644 index 00000000000..64f9ef767b7 --- /dev/null +++ b/src/util/program_binary.h @@ -0,0 +1,70 @@ +/* + * Mesa 3-D graphics library + * + * Copyright (C) 2004-2007 Brian Paul All Rights Reserved. + * Copyright (C) 2010 VMware, Inc. All Rights Reserved. + *+ * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions:+ *+ * The above copyright notice and this permission notice shall be included+ * in all copies or substantial portions of the Software. + *+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR+ * OTHER DEALINGS IN THE SOFTWARE. + */ + + +#ifndef PROGRAM_BINARY_H +#define PROGRAM_BINARY_H + +#include <stdbool.h> + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Returns the header size needed for a binary + */ +extern unsigned +get_program_binary_header_size(void); + +/** + * Returns the program binary for a given payload. + *+ * This binary can be used as a return for the glGetProgramBinary call.+ */ +extern bool +write_program_binary(const void *payload, unsigned payload_size,+ const void *sha1, void *binary, unsigned binary_size,+ GLenum *binary_format); + +/** + * Returns the payload within the binary. + * + * If NULL is returned, then the binary not supported. If non-NULL is+ * returned, it will be a pointer contained within the specified `binary`+ * buffer. + * + * This can be used to access the payload of `binary` during the + * glProgramBinary call. + */ +extern const void* +get_program_binary_payload(GLenum binary_format, const void *sha1, + const void *binary, unsigned length); + +#ifdef __cplusplus +} +#endif + +#endif /* PROGRAM_BINARY_H */_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
