[Mesa-dev] [PATCH 1/2] vulkan/util: Introduce format utilities
Jason Ekstrand
jason at jlekstrand.net
Wed Jun 28 00:45:56 UTC 2017
On Tue, Jun 27, 2017 at 4:29 PM, Chad Versace <chadversary at chromium.org>
wrote:
> On Fri 23 Jun 2017, alexandros.frantzis at collabora.com wrote:
> > From: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> >
> > Introduce utilities to describe, find and compare Vulkan formats based
> > on their color component masks, taking into account system endianness.
> > ---
> > src/vulkan/Makefile.sources | 2 +
> > src/vulkan/util/vk_format_util.c | 173 ++++++++++++++++++++++++++++++
> +++++++++
> > src/vulkan/util/vk_format_util.h | 100 ++++++++++++++++++++++
> > 3 files changed, 275 insertions(+)
> > create mode 100644 src/vulkan/util/vk_format_util.c
> > create mode 100644 src/vulkan/util/vk_format_util.h
> >
> > diff --git a/src/vulkan/Makefile.sources b/src/vulkan/Makefile.sources
> > index 2cf7218e92..f4f1f14d5a 100644
> > --- a/src/vulkan/Makefile.sources
> > +++ b/src/vulkan/Makefile.sources
> > @@ -17,6 +17,8 @@ VULKAN_WSI_X11_FILES := \
> >
> > VULKAN_UTIL_FILES := \
> > util/vk_alloc.h \
> > + util/vk_format_util.c \
> > + util/vk_format_util.h \
> > util/vk_util.c \
> > util/vk_util.h
> >
> > diff --git a/src/vulkan/util/vk_format_util.c
> b/src/vulkan/util/vk_format_util.c
> > new file mode 100644
> > index 0000000000..aa6d403e84
> > --- /dev/null
> > +++ b/src/vulkan/util/vk_format_util.c
> > @@ -0,0 +1,173 @@
> > +/*
> > + * Copyright © 2017 Collabora Ltd.
> > + *
> > + * 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 (including the
> next
> > + * paragraph) 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.
> > + */
> > +
> > +#include "vk_format_util.h"
> > +
> > +#include "util/u_vector.h"
> > +#include "util/u_math.h"
> > +
> > +#include <assert.h>
> > +
> > +static inline uint32_t
> > +bswap24(uint32_t n)
> > +{
> > + return (n & 0xff000000) |
> > + ((n >> 16) & 0x000000ff) |
> > + (n & 0x0000ff00) |
> > + ((n << 16) & 0x00ff0000);
> > +}
> > +
> > +static inline uint32_t
> > +bswap(uint32_t n, int bits)
> > +{
> > + switch (bits)
> > + {
>
> Mesa code-style puts the '{' on the same line as 'switch'.
>
> > + case 8:
> > + return n;
> > + case 16:
> > + return util_bswap16(n);
> > + case 24:
> > + return bswap24(n);
> > + case 32:
> > + return util_bswap32(n);
> > + default:
> > + assert(!"Invalid bswap bits");
> > + return n;
>
> If someone passes an invalid param here, there's little point in
> continuing, even in release builds, because continuing will produce only
> undefined behavior. So let's optimize the release build by removing the
> default case, like this:
>
> bswap(...)
> {
> switch (bits) {
> case 8:
> ...
> case 16:
> ...
> case 32:
> ...
> }
>
> unreachable("invalid bswap bits");
> }
>
> > + }
> > +}
> > +
> > +struct format_spec {
> > + VkFormat format;
> > + struct vk_format_util_spec spec;
> > +};
> > +
> > +#define VF(FMT, BPP, R, G, B, A, E, S) \
> > + {VK_FORMAT_##FMT, {BPP, R, G, B, A, VK_FORMAT_UTIL_ENDIANNESS_##E,
> VK_FORMAT_UTIL_##S}}
> > +
> > +// SRGB formats are presented to the user before the UNORM ones.
>
> The comment should explain why the sRGB formats precede the others.
>
> Also, Mesa code-style uses /* comments, not // comments.
>
> > +static const struct format_spec format_specs[] = {
> > + VF(R8_SRGB, 8, 0x000000ff, 0x00000000, 0x00000000, 0x00000000, BIG,
> SWAPPABLE),
> > + VF(R8G8_SRGB, 16, 0x0000ff00, 0x000000ff, 0x00000000, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(R8G8B8_SRGB, 24, 0x00ff0000, 0x0000ff00, 0x000000ff, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(B8G8R8_SRGB, 24, 0x000000ff, 0x0000ff00, 0x00ff0000, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(R8G8B8A8_SRGB, 32, 0xff000000, 0x00ff0000, 0x0000ff00,
> 0x000000ff, BIG, SWAPPABLE),
> > + VF(B8G8R8A8_SRGB, 32, 0x0000ff00, 0x00ff0000, 0xff000000,
> 0x000000ff, BIG, SWAPPABLE),
> > + VF(A8B8G8R8_SRGB_PACK32, 32, 0x000000ff, 0x0000ff00, 0x00ff0000,
> 0xff000000, NATIVE, SWAPPABLE),
> > +
> > + VF(R8_UNORM, 8, 0x000000ff, 0x00000000, 0x00000000, 0x00000000, BIG,
> SWAPPABLE),
> > + VF(R8G8_UNORM, 16, 0x0000ff00, 0x000000ff, 0x00000000, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(R8G8B8_UNORM, 24, 0x00ff0000, 0x0000ff00, 0x000000ff, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(B8G8R8_UNORM, 24, 0x000000ff, 0x0000ff00, 0x00ff0000, 0x00000000,
> BIG, SWAPPABLE),
> > + VF(R8G8B8A8_UNORM, 32, 0xff000000, 0x00ff0000, 0x0000ff00,
> 0x000000ff, BIG, SWAPPABLE),
> > + VF(B8G8R8A8_UNORM, 32, 0x0000ff00, 0x00ff0000, 0xff000000,
> 0x000000ff, BIG, SWAPPABLE),
> > + VF(R4G4_UNORM_PACK8, 8, 0x000000f0, 0x0000000f, 0x00000000,
> 0x00000000, NATIVE, NON_SWAPPABLE),
> > + VF(R4G4B4A4_UNORM_PACK16, 16, 0x0000f000, 0x00000f00, 0x000000f0,
> 0x0000000f, NATIVE, NON_SWAPPABLE),
> > + VF(B4G4R4A4_UNORM_PACK16, 16, 0x000000f0, 0x00000f00, 0x0000f000,
> 0x0000000f, NATIVE, NON_SWAPPABLE),
> > + VF(R5G6B5_UNORM_PACK16, 16, 0x0000f800, 0x000007e0, 0x0000001f,
> 0x00000000, NATIVE, NON_SWAPPABLE),
> > + VF(B5G6R5_UNORM_PACK16, 16, 0x0000001f, 0x000007e0, 0x0000f800,
> 0x00000000, NATIVE, NON_SWAPPABLE),
> > + VF(R5G5B5A1_UNORM_PACK16, 16, 0x0000f800, 0x000007c0, 0x0000003e,
> 0x00000001, NATIVE, NON_SWAPPABLE),
> > + VF(B5G5R5A1_UNORM_PACK16, 16, 0x0000003e, 0x000007c0, 0x0000f800,
> 0x00000001, NATIVE, NON_SWAPPABLE),
> > + VF(A1R5G5B5_UNORM_PACK16, 16, 0x00007c00, 0x000003e0, 0x0000001f,
> 0x00008000, NATIVE, NON_SWAPPABLE),
> > + VF(A8B8G8R8_UNORM_PACK32, 32, 0x000000ff, 0x0000ff00, 0x00ff0000,
> 0xff000000, NATIVE, SWAPPABLE),
> > + VF(A2R10G10B10_UNORM_PACK32, 32, 0x3ff00000, 0x000ffc00, 0x000003ff,
> 0xc0000000, NATIVE, NON_SWAPPABLE),
> > + VF(A2B10G10R10_UNORM_PACK32, 32, 0x000003ff, 0x000ffc00, 0x3ff00000,
> 0xc0000000, NATIVE, NON_SWAPPABLE),
> > +};
>
> The concepts of "swappable" and "non-swappable", as applied to formats
> here, is not a concept present in the Vulkan spec.
>
> I'm not sure what swappable/non-swappable encodes in this table, and why
> vk_compare_format_specs() is trying to do with swappability. I tried
> reverse-engineering the patch's intent, and formed the following
> observations and questions:
>
> 1. The table partitions the formats into 3 classes:
>
> a. Big-endian and swappable. All non-packed formats belong to
> this class.
>
> b. Native-endian and non-swappable. All packed formats, except
> VK_FORMAT_A8B8G8R8_SRGB_PACK32 and
> VK_FORMAT_A8B8G8R8_UNORM_PACK32, belong to this class.
>
> c. Native-endian and swappable. Exactly
> VK_FORMAT_A8B8G8R8_SRGB_PACK32 and
> VK_FORMAT_A8B8G8R8_UNORM_PACK32 belong to this class.
>
> 2. Why are no formats marked as little-endian?
>
> 3. Why are the A8B8G8R8 formats special? Because each channel is
> byte-aligned?
>
> I suspect that the table is actually trying to capture the
> distinction between "packed" and "non-packed" formats, as defined
> by the Vulkan spec, in section 32.2.1 Format Definition. (Some people
> say "array" formats instead of "non-packed" formats). (The Vulkan spec
> also defines the concept of "block" formats, but that's irrelevant to
> this patch).
>
> I don't believe the endianness scheme in this patch is not able to capture
> the
> bit-layout of VK_FORMAT_R16G16B16A16_UNORM, but the Vulkan spec's notion
> of "packed/non-packed" is. According to table "Byte mappings for
> non-packed/compressed color formats" in the Vulkan 1.0.51 spec,
> VK_FORMAT_R16G16B16A16_UNORM is a non-packed format where the red channel
> is bytes 0-1, green is bytes 2-3, blue is bytes 4-5, etc, but the bytes
> within each channel are ordered by host endianness.
>
Drive-by: Yes, I agree that this doesn't match Vulkan particularly well.
It's a very X11 or drm_fourcc way of described formats and is nothing like
anything else we have in mesa. That doesn't mean it's strictly wrong, just
that it's awkward to people who are used to some of the other mechanisms
for describing formats.
> I don't understand how swappable/non-swappable apply to
> VK_FORMAT_R16G16B16A16_UNORM.
>
> > +
> > +#undef VF
> > +
> > +static inline vk_format_util_endianness
> > +native_endianness()
>
> In C, the empty parameter list is not what you want. The compiler
> interprets the empty parameter parameter list as an old-style variadic
> parameter list. You want native_endianness(void) instead.
>
> Anyway, this function should be replaced by the macros in
> src/util/u_endian.h.
>
> > +{
> > + const unsigned ui = 1;
> > + return *((const char *) &ui) == 1 ? VK_FORMAT_UTIL_ENDIANNESS_LITTLE
> :
> > + VK_FORMAT_UTIL_ENDIANNESS_BIG;
> > +}
> > +
> > +void
> > +vk_get_format_spec(VkFormat format,
> > + struct vk_format_util_spec *spec)
> > +{
> > + static const struct vk_format_util_spec empty_spec =
> > + {0, 0x0, 0x0, 0x0, 0x0, VK_FORMAT_UTIL_ENDIANNESS_NATIVE,
> VK_FORMAT_UTIL_NON_SWAPPABLE};
>
> The verbose struct initializer should be replaced with `empty_spec = {0};`
> The C99 spec guarantees that `= {0}` produces a zero-initialized struct
> here.
>
> > +
> > + for (size_t i = 0; i < ARRAY_SIZE(format_specs); i++) {
> > + if (format_specs[i].format == format) {
> > + *spec = format_specs[i].spec;
> > + return;
> > + }
> > + }
> > +
> > + *spec = empty_spec;
> > +}
> > +
> > +bool
> > +vk_compare_format_specs(const struct vk_format_util_spec* spec1,
> > + const struct vk_format_util_spec* spec2)
> > +{
> > + const vk_format_util_endianness endianness1 =
> > + spec1->endianness == VK_FORMAT_UTIL_ENDIANNESS_NATIVE ?
> > + native_endianness() : spec1->endianness;
> > +
> > + const vk_format_util_endianness endianness2 =
> > + spec2->endianness == VK_FORMAT_UTIL_ENDIANNESS_NATIVE ?
> > + native_endianness() : spec2->endianness;
> > +
> > + if (endianness1 == endianness2) {
> > + return spec1->bits_per_pixel == spec2->bits_per_pixel &&
> > + spec1->red_mask == spec2->red_mask &&
> > + spec1->green_mask == spec2->green_mask &&
> > + spec1->blue_mask == spec2->blue_mask &&
> > + spec1->alpha_mask == spec2->alpha_mask;
> > + }
> > + else {
>
> I don't understand why the 'else' branch exists. But that's likely due
> to my confusion regarding swappable/non-swappable, explained above.
>
> Also, Mesa code-style is `} else {`.
>
> > + if (spec1->swappable == VK_FORMAT_UTIL_SWAPPABLE) {
> > + return spec1->bits_per_pixel == spec2->bits_per_pixel &&
> > + bswap(spec1->red_mask, spec1->bits_per_pixel) ==
> spec2->red_mask &&
> > + bswap(spec1->green_mask, spec1->bits_per_pixel) ==
> spec2->green_mask &&
> > + bswap(spec1->blue_mask, spec1->bits_per_pixel) ==
> spec2->blue_mask &&
> > + bswap(spec1->alpha_mask, spec1->bits_per_pixel) ==
> spec2->alpha_mask;
> > + }
> > + else if (spec2->swappable == VK_FORMAT_UTIL_SWAPPABLE) {
> > + return spec1->bits_per_pixel == spec2->bits_per_pixel &&
> > + spec1->red_mask == bswap(spec2->red_mask,
> spec2->bits_per_pixel) &&
> > + spec1->green_mask == bswap(spec2->green_mask,
> spec2->bits_per_pixel) &&
> > + spec1->blue_mask == bswap(spec2->blue_mask,
> spec2->bits_per_pixel) &&
> > + spec1->alpha_mask == bswap(spec2->alpha_mask,
> spec2->bits_per_pixel);
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > +void
> > +vk_find_matching_formats(const struct vk_format_util_spec *spec,
> > + struct u_vector *formats)
> > +{
> > + for (size_t i = 0; i < ARRAY_SIZE(format_specs); i++) {
> > + if (vk_compare_format_specs(spec, &format_specs[i].spec)) {
> > + VkFormat *f = u_vector_add(formats);
> > + if (f)
> > + *f = format_specs[i].format;
> > + }
> > + }
> > +}
> > diff --git a/src/vulkan/util/vk_format_util.h
> b/src/vulkan/util/vk_format_util.h
> > new file mode 100644
> > index 0000000000..a1f9d87abe
> > --- /dev/null
> > +++ b/src/vulkan/util/vk_format_util.h
> > @@ -0,0 +1,100 @@
> > +/*
> > + * Copyright © 2017 Collabora Ltd.
> > + *
> > + * 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 (including the
> next
> > + * paragraph) 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 VK_FORMAT_UTIL_H
> > +#define VK_FORMAT_UTIL_H
> > +
> > +#include <vulkan/vulkan.h>
> > +#include <stdbool.h>
> > +
> > +struct u_vector;
> > +
> > +typedef enum vk_format_util_endianness {
> > + VK_FORMAT_UTIL_ENDIANNESS_NATIVE,
> > + VK_FORMAT_UTIL_ENDIANNESS_LITTLE,
> > + VK_FORMAT_UTIL_ENDIANNESS_BIG,
> > +} vk_format_util_endianness;
> > +
> > +typedef enum vk_format_util_swappable {
> > + VK_FORMAT_UTIL_NON_SWAPPABLE,
> > + VK_FORMAT_UTIL_SWAPPABLE,
> > +} vk_format_util_swappable;
> > +
> > +/*
> > + * Structure describing a pixel format.
> > + *
> > + * The masks express the bit positions of the components in an native
> integer
> > + * storing a pixel of this format.
> > + *
> > + * Examples:
> > + *
> > + * * A pixel format which is always stored as a 0xR8G8B8A8 32-bit
> integer:
> > + *
> > + * {32, 0xff000000, 0x00ff0000, 0x0000ff00, 0x000000ff, NATIVE,
> SWAPPABLE}
> > + *
> > + * * A pixel format which is always stored as R8,G8,B8,A8 in memory
> order,
> > + * with R at the lowest address:
> > + *
> > + * {32, 0xff000000, 0x00ff0000, 0x0000ff00, 0x000000ff, BIG,
> SWAPPABLE}
> > + * or
> > + * {32, 0x000000ff, 0x0000ff00, 0x00ff0000, 0xff000000, LITTLE,
> SWAPPABLE}
> > + *
> > + * * A pixel format which is always stored as a 0xR5G6B5 16-bit
> integer,
> > + * and cannot change endianness by byte-swapping:
> > + *
> > + * {16, 0x0000f800, 0x000007e0, 0x0000001f, 0x00000000, NATIVE,
> NON_SWAPPABLE},
> > + *
> > + */
> > +struct vk_format_util_spec {
> > + int bits_per_pixel;
> > + uint32_t red_mask;
> > + uint32_t green_mask;
> > + uint32_t blue_mask;
> > + uint32_t alpha_mask;
> > + /* The endianness for which the masks are correct */
> > + vk_format_util_endianness endianness;
> > + /* Whether the masks can be byte-swapped to switch endianness */
> > + vk_format_util_swappable swappable;
> > +};
> > +
> > +
> > +/* Fill a vk_format_util_spec structure to describe a Vulkan format. */
> > +void
> > +vk_get_format_spec(VkFormat format,
> > + struct vk_format_util_spec *spec);
> > +
> > +/* Compare two pixel formats described by vk_format_util_spec
> structures. */
> > +bool
> > +vk_compare_format_specs(const struct vk_format_util_spec *spec1,
> > + const struct vk_format_util_spec *spec2);
> > +
> > +/*
> > + * Find Vulkan pixel formats that match a vk_format_util_spec.
> > + *
> > + * Note that, at the moment, only UNORM and SRGB formats are considered.
> > + */
> > +void
> > +vk_find_matching_formats(const struct vk_format_util_spec *spec,
> > + struct u_vector *formats);
> > +
> > +#endif
> > --
> > 2.11.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170627/fe499ee6/attachment-0001.html>
More information about the mesa-dev
mailing list