<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 17, 2015 at 1:34 PM, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On 02/13/2015 11:36 AM, Laura Ekstrand wrote:<br>
> ---<br>
>  .../getcompressedtextureimage.c                    | 198 +++++++++++++++++++++<br>
>  1 file changed, 198 insertions(+)<br>
>  create mode 100644 tests/spec/arb_direct_state_access/getcompressedtextureimage.c<br>
><br>
> diff --git a/tests/spec/arb_direct_state_access/getcompressedtextureimage.c b/tests/spec/arb_direct_state_access/getcompressedtextureimage.c<br>
> new file mode 100644<br>
> index 0000000..e4f15d7<br>
> --- /dev/null<br>
> +++ b/tests/spec/arb_direct_state_access/getcompressedtextureimage.c<br>
> @@ -0,0 +1,198 @@<br>
> +/*<br>
> + * Copyright 2015 Intel Corporation<br>
> + *<br>
> + * Permission is hereby granted, free of charge, to any person obtaining a<br>
> + * copy of this software and associated documentation files (the "Software"),<br>
> + * to deal in the Software without restriction, including without limitation<br>
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
> + * and/or sell copies of the Software, and to permit persons to whom the<br>
> + * Software is furnished to do so, subject to the following conditions:<br>
> + *<br>
> + * The above copyright notice and this permission notice (including the next<br>
> + * paragraph) shall be included in all copies or substantial portions of the<br>
> + * Software.<br>
> + *<br>
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL<br>
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER<br>
> + * DEALINGS IN THE SOFTWARE.<br>
> + */<br>
> +<br>
> +/** @file getcompressedtextureimage.c<br>
> + *<br>
> + * Tests to make sure that glGetCompressedTextureImage works correctly.<br>
> + * The basic idea is to create some fake compressed data, upload it to the<br>
> + * driver with the traditional commands, download it with DSA, and compare.<br>
> + * Of course, this assumes that the traditional commands are correct, which<br>
> + * may not actually be the case.<br>
> + */<br>
> +<br>
> +#include "piglit-util-gl.h"<br>
> +<br>
> +PIGLIT_GL_TEST_CONFIG_BEGIN<br>
> +<br>
> +     config.supports_gl_compat_version = 10;<br>
<br>
</div></div>Related to Ilia's comments about checking for additional extensions...<br>
do we actually want to test compatibility profile?  Jason pointed this<br>
out to me on IRC late January:<br>
<br>
"Dependencies<br>
<br>
    OpenGL 2.0 is required."<br>
<br>
This means Mesa will need a flag for the extension because there are<br>
pre-2.0 drivers.  Every DSA function will need to check the flag and<br>
possibly generate GL_INVALID_OPERATION.<br>
<br>
If we only support core profile (and always enable it in core profile),<br>
the functions won't even be put in the dispatch table for non-core.  No<br>
flag, and no checks in the functions.<br>
<br>
So, the $64,000.00 question is: Do we care to support DSA on the few<br>
cards that can do 2.0 but not 3.1?<br>
<br>
Either way, it should be safe to change this to<br>
<br>
        config.supports_gl_compat_version = 20;<br>
<br>
at the very least.<br>
<span><br>
> +<br>
> +     config.window_visual = PIGLIT_GL_VISUAL_RGBA |<br>
> +             PIGLIT_GL_VISUAL_DOUBLE;<br>
> +<br>
> +PIGLIT_GL_TEST_CONFIG_END<br>
> +<br>
> +/* Copied from Mesa src/mesa/main/formats.csv:<br>
> + * MESA_FORMAT_RGBA_DXT5, s3tc, 4, 4, x128<br>
> + */<br>
> +#define FORMAT GL_COMPRESSED_RGBA_S3TC_DXT5_EXT<br>
> +#define WIDTH 32<br>
> +#define HEIGHT 32<br>
> +#define WIDTH_IN_BLOCKS 8<br>
> +#define HEIGHT_IN_BLOCKS 8<br>
> +#define BLOCK_BYTES 16<br>
> +#define IMAGE_SIZE (WIDTH_IN_BLOCKS * HEIGHT_IN_BLOCKS * BLOCK_BYTES)<br>
> +#define CHAR_LIMIT 100<br>
> +<br>
> +static GLubyte *expected;<br>
> +static GLboolean pass = GL_TRUE;<br>
<br>
</span>static bool pass = true;<br>
<div><div><br>
> +<br>
> +static void<br>
> +subtest(GLboolean local_result, const char *test_name)<br>
> +{<br>
> +     pass = local_result && pass;<br>
> +     piglit_report_subtest_result(local_result ? PIGLIT_PASS : PIGLIT_FAIL,<br>
> +                                  test_name);<br>
> +}<br>
> +<br>
> +static void<br>
> +init_random_data(void)<br>
> +{<br>
> +     int i;<br>
> +<br>
> +     expected = malloc(18 * IMAGE_SIZE);<br>
> +     for (i = 0; i < 18 * IMAGE_SIZE; ++i) {<br>
> +             expected[i] = (GLubyte) rand();<br>
> +     }<br>
> +}<br>
> +<br>
> +void<br>
> +piglit_init(int argc, char **argv)<br>
> +{<br>
> +     piglit_require_extension("GL_ARB_direct_state_access");<br>
> +     srand(0);<br>
> +     init_random_data();<br>
> +}<br>
> +<br>
> +static bool<br>
> +compare_to_expected(const GLubyte *data, int num_layers)<br>
> +{<br>
> +     return memcmp(expected, data, num_layers * IMAGE_SIZE) == 0;<br>
> +}<br>
<br>
</div></div>Why not fold this function into subtest (above)?  It looks like either<br>
function is only ever called as<br>
<br>
    subtest(compare_to_expected(...), ...);</blockquote><div>Martin has a macro SUBTEST_CONDITION that I'm hoping to use here instead.  I've asked him to send out a commit that adds the macro to piglit-util-gl.h.  Right now, I'm using this function as a stand-in.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
> +<br>
> +static void<br>
> +upload_subtest(GLenum target, bool use_pbo)<br>
> +{<br>
> +     GLuint tex, pbo;<br>
> +     int num_layers;<br>
> +     char test_name [CHAR_LIMIT];<br>
> +     int i;<br>
> +<br>
> +     /* Prepare to read data from expected */<br>
> +     GLubyte *data = expected;<br>
> +     if (use_pbo) {<br>
> +             glGenBuffers(1, &pbo);<br>
> +             glBindBuffer(GL_PIXEL_UNPACK_BUFFER, pbo);<br>
> +             glBufferData(GL_PIXEL_UNPACK_BUFFER, 18 * IMAGE_SIZE,<br>
> +                          expected, GL_STATIC_DRAW);<br>
> +             data = NULL;<br>
> +     }<br>
> +<br>
> +     /* Upload it using traditional path */<br>
> +     glGenTextures(1, &tex);<br>
> +     glBindTexture(target, tex);<br>
> +<br>
> +     switch (target) {<br>
> +<br>
> +     case GL_TEXTURE_2D:<br>
> +             num_layers = 1;<br>
> +             glTexStorage2D(target, 1, FORMAT, WIDTH, HEIGHT);<br>
> +             glCompressedTexSubImage2D(target, 0, 0, 0, WIDTH, HEIGHT,<br>
> +                                       FORMAT, num_layers * IMAGE_SIZE,<br>
> +                                       data);<br>
> +             break;<br>
> +     case GL_TEXTURE_2D_ARRAY:<br>
> +             num_layers = 3;<br>
> +             glTexStorage3D(target, 1, FORMAT, WIDTH, HEIGHT, num_layers);<br>
> +             glCompressedTexSubImage3D(target, 0, 0, 0, 0, WIDTH, HEIGHT,<br>
> +                                       num_layers, FORMAT,<br>
> +                                       num_layers * IMAGE_SIZE, data);<br>
> +             break;<br>
> +     case GL_TEXTURE_CUBE_MAP:<br>
> +             num_layers = 6;<br>
> +             glTexStorage2D(target, 1, FORMAT, WIDTH, HEIGHT);<br>
> +             for (i = 0; i < num_layers; ++i) {<br>
> +                     glCompressedTexSubImage2D(<br>
> +                             GL_TEXTURE_CUBE_MAP_POSITIVE_X + i, 0, 0, 0,<br>
> +                             WIDTH, HEIGHT, FORMAT, IMAGE_SIZE,<br>
> +                             data + i * IMAGE_SIZE);<br>
> +             }<br>
> +             break;<br>
> +     case GL_TEXTURE_CUBE_MAP_ARRAY:<br>
> +             num_layers = 18;<br>
> +             glTexStorage3D(target, 1, FORMAT, WIDTH, HEIGHT, num_layers);<br>
> +             glCompressedTexSubImage3D(target, 0, 0, 0, 0, WIDTH, HEIGHT,<br>
> +                                       num_layers, FORMAT,<br>
> +                                       num_layers * IMAGE_SIZE, data);<br>
> +             break;<br>
> +     default:<br>
> +             printf("Bad texture target.\n");<br>
> +             piglit_report_result(PIGLIT_FAIL);<br>
> +<br>
> +     }<br>
> +<br>
> +     /* Prepare to download into data. */<br>
> +     if (use_pbo) {<br>
> +             glBindBuffer(GL_PIXEL_PACK_BUFFER, pbo);<br>
> +             glBufferData(GL_PIXEL_PACK_BUFFER, 18 * IMAGE_SIZE,<br>
> +                          NULL, GL_STATIC_READ);<br>
> +             /* data = NULL from above */<br>
> +     }<br>
> +     else<br>
> +             data = malloc(18 * IMAGE_SIZE);<br>
> +<br>
> +     /* Download it using DSA */<br>
> +     glBindTexture(target, 0);<br>
> +     glGetCompressedTextureImage(tex, 0, num_layers * IMAGE_SIZE, data);<br>
> +<br>
> +     if (use_pbo)<br>
> +             data = glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);<br>
<br>
</div></div>Using 'data' for multiple things in this way seems really fragile.  I<br>
think it would be both more clear and less fragile as:<br></blockquote><div>I guess I misunderstood your comment on the old test here.  I will fix this in the v2.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
        /* Download it using DSA */<br>
</span>        glBindTexture(target, 0);<br>
        if (use_pbo) {<br>
                glBindBuffer(GL_PIXEL_PACK_BUFFER, pbo);<br>
                glBufferData(GL_PIXEL_PACK_BUFFER, 18 * IMAGE_SIZE,<br>
                             NULL, GL_STATIC_READ);<br>
<br>
                glGetCompressedTextureImage(tex, 0, num_layers * IMAGE_SIZE, (void *) 0);<br>
<br>
                data = glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);<br>
        } else {<br>
<span>                data = malloc(18 * IMAGE_SIZE);<br>
</span><span>                glGetCompressedTextureImage(tex, 0, num_layers * IMAGE_SIZE, data);<br>
        }<br>
<br>
</span>I might also be tempted to use a separate buffer object for the pack<br>
buffer.  glBufferData doesn't guarantee the contents of buffer, and it<br>
doesn't guarantee that it will be a fresh allocation.  If<br>
glGetCompressedTextureImage does nothing (perhaps because an error was<br>
generated), you could still have the same data in the buffer resulting<br>
in a false positive.<br></blockquote><div>Ok, I will correct this in the v2.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
> +<br>
> +     /* Perform comparison and report the results. */<br>
> +     snprintf(test_name, CHAR_LIMIT, "upload %s%s",<br>
> +              piglit_get_gl_enum_name(target), use_pbo ? " PBO" : "");<br>
> +     subtest(compare_to_expected(data, num_layers), test_name);<br>
> +<br>
> +     /* Clean up */<br>
> +     if (use_pbo)<br>
> +             glDeleteBuffers(1, &pbo);<br>
> +     else<br>
> +             free(data);<br>
> +}<br>
> +<br>
> +enum piglit_result<br>
> +piglit_display(void)<br>
> +{<br>
> +     /* Non-PBO tests */<br>
> +     upload_subtest(GL_TEXTURE_2D, false);<br>
> +     upload_subtest(GL_TEXTURE_2D_ARRAY, false);<br>
> +     upload_subtest(GL_TEXTURE_CUBE_MAP, false);<br>
> +     upload_subtest(GL_TEXTURE_CUBE_MAP_ARRAY, false);<br>
> +<br>
> +     /* PBO tests */<br>
> +     upload_subtest(GL_TEXTURE_2D, true);<br>
> +     upload_subtest(GL_TEXTURE_2D_ARRAY, true);<br>
> +     upload_subtest(GL_TEXTURE_CUBE_MAP, true);<br>
> +     upload_subtest(GL_TEXTURE_CUBE_MAP_ARRAY, true);<br>
> +<br>
> +     return pass ? PIGLIT_PASS : PIGLIT_FAIL;<br>
> +}<br>
><br>
<br>
</div></div></blockquote></div><br></div></div>