[Piglit] [PATCH v2] arb_shader_image_load_store: Test format incompatible texture buffer

[Danylo Piliaiev]

On 20.07.18 23:19, Francisco Jerez wrote:
> Danylo Piliaiev <danylo.piliaiev at gmail.com> writes:
>
>> On 20.07.18 17:04, Danylo Piliaiev wrote:
>>>
>>> On 20.07.18 02:26, Francisco Jerez wrote:
>>>> Danylo Piliaiev <danylo.piliaiev at gmail.com> writes:
>>>>
>>>>> Test for the regression which happened when GL_TEXTURE_BUFFER was
>>>>> allowed to have incompatible format.
>>>>>
>>>>> v2: Removed unnecessary code duplication - use upload_image instead
>>>>>        of init_level. (Francisco Jerez)
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106465
>>>>>
>>>>> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
>>>>> ---
>>>>>    .../arb_shader_image_load_store/invalid.c     | 23
>>>>> +++++++++++++++----
>>>>>    1 file changed, 19 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tests/spec/arb_shader_image_load_store/invalid.c
>>>>> b/tests/spec/arb_shader_image_load_store/invalid.c
>>>>> index ed4b6c064..99f6703a4 100644
>>>>> --- a/tests/spec/arb_shader_image_load_store/invalid.c
>>>>> +++ b/tests/spec/arb_shader_image_load_store/invalid.c
>>>>> @@ -266,13 +266,17 @@ static bool
>>>>>    invalidate_incompatible_format(const struct image_info img, GLuint
>>>>> prog)
>>>>>    {
>>>>>            GLenum base_format = image_base_internal_format(img.format);
>>>>> +        uint32_t pixels[4 * N];
>>>>> +        init_pixels(img, pixels, 1, 1, 1, 1);
>>>>> +
>>>>> +        /* upload_image instead of init_level to support
>>>>> GL_TEXTURE_BUFFER */
>>>>> +        bool ret = upload_image(img, 0, pixels);
>>>>> +
>>>> I don't think you actually need to call upload_image() here, it should
>>>> have been called already by init_image().
>>> I don't know where I'm looking... You are right. Only
>>> glBindImageTexture will be left
>>> and it's enough for the test.
>>>>>            /* Pick an incompatible texture format with a compatible base
>>>>>             * type. */
>>>>> -        bool ret = init_level(img, 0, (base_format == GL_RGBA32F ?
>>>>> -                                       GL_RGBA8 : GL_RG32UI), W, H);
>>>>> -
>>>>>            glBindImageTexture(0, get_texture(0), 0, GL_TRUE, 0,
>>>>> -                           GL_READ_WRITE, img.format->format);
>>>>> +                           GL_READ_WRITE, (base_format == GL_RGBA32F ?
>>>>> +                                           GL_RGBA8 : GL_RG32UI));
>>>>>              return ret && piglit_check_gl_error(GL_NO_ERROR);
>>>>>    }
>>>>> @@ -346,6 +350,8 @@ piglit_init(int argc, char **argv)
>>>>>            for (op = image_ops; op->name; ++op) {
>>>>>                    const struct image_info def_img = image_info(
>>>>>                            GL_TEXTURE_2D, op->formats[0].format, W, H);
>>>>> +                const struct image_info def_img_buffer = image_info(
>>>>> +                        GL_TEXTURE_BUFFER, op->formats[0].format,
>>>>> W, H);
>>>>>                      /*
>>>>>                     * According to the spec, an access is considered
>>>>> @@ -399,6 +405,15 @@ piglit_init(int argc, char **argv)
>>>>> invalidate_incompatible_format, false),
>>>>>                            "%s/incompatible format test", op->name);
>>>>>    +                /* Test for the regression which happened when
>>>>> +                 * GL_TEXTURE_BUFFER was allowed to have
>>>>> incompatible format.
>>>>> +                 */
>>>> FTR, did you confirm whether this test-case causes a crash after
>>>> re-applying the mesa patch that led to the regression?
>>>>
>>>> Thanks.
>>> This test-case doesn't crash with that patch. I picked the first
>>> format from the array
>>> (same as the other tests) which appears to be GL_RGBA32F, but
>>> unfortunately it's a
>>> format with which we cannot go out of bounds. Picking the format which
>>> will result in crash
>>> would be better (already tested and crash occurred). I'll do this in
>>> my hopefully final version of the patch.
>>>
>> Please, disregard this part, it doesn't crash regardless of format, test
>> just fails - nothing more.
> But I get that the test runs successfully after reverting the offending
> commit?  In that case patch is:
>
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
Yes, tests pass after reverting the offending commit and fail when 
applying it.
>>> Thank you for reviewing these embarrassing patches.
> No worries you're fine :)
>
>>>>> +                subtest(&status, true,
>>>>> +                        run_test(op, def_img_buffer, def_img_buffer,
>>>>> + invalidate_incompatible_format, false),
>>>>> +                        "%s/incompatible format test/image%s",
>>>>> +                        op->name, def_img_buffer.target->name);
>>>>> +
>>>>>                    /*
>>>>>                     * " * the texture bound to the image unit has
>>>>> layers,
>>>>>                     *     and the selected layer or cube map face
>>>>> doesn't
>>>>> -- 
>>>>> 2.17.1