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

[Francisco Jerez]

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>

>> 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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20180720/02357806/attachment.sig>