[Mesa-dev] [PATCH v2 15/17] util: Add Mesa ARB_get_program_binary helper functions
Timothy Arceri
tarceri at itsqueeze.com
Tue Nov 21 11:17:44 UTC 2017
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,
>>
>> -Jordan
>>
>>>
>>> On 21/11/17 09:27, Jordan Justen wrote:
>>>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>>>> ---
>>>> 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
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list