[Piglit] [PATCH 3/4] arb_internalformat_query2: fix TBO testing
Marek Olšák
maraeo at gmail.com
Tue Oct 24 01:15:52 UTC 2017
I just sent patch version 2 separately.
Marek
On Tue, Oct 24, 2017 at 3:15 AM, Marek Olšák <maraeo at gmail.com> wrote:
> On Sun, Oct 22, 2017 at 12:33 PM, Alejandro Piñeiro
> <apinheiro at igalia.com> wrote:
>> On 21/10/17 14:55, Marek Olšák wrote:
>>> From: Marek Olšák <marek.olsak at amd.com>
>>>
>>> This was never tested and it uses functions that are illegal with TBOs,
>>> like glGetTexParameteriv and glTexImage2DMultisample.
>>
>> Hi, I didn't check all the patch in detail, but as Illia is saying, it
>> is correct to pass TEXTURE_BUFFER to GetInternalFormat*. I also disagree
>> that this case it is not tested, or at least for the case I checked. See
>> below, I have two extra comments.
>>
>>> ---
>>> tests/spec/arb_internalformat_query2/common.h | 4 +++-
>>> tests/spec/arb_internalformat_query2/format-components.c | 2 +-
>>> tests/spec/arb_internalformat_query2/generic-pname-checks.c | 2 +-
>>> .../spec/arb_internalformat_query2/image-format-compatibility-type.c | 2 +-
>>> 4 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/spec/arb_internalformat_query2/common.h b/tests/spec/arb_internalformat_query2/common.h
>>> index df68581..0e5e11f 100644
>>> --- a/tests/spec/arb_internalformat_query2/common.h
>>> +++ b/tests/spec/arb_internalformat_query2/common.h
>>> @@ -25,26 +25,28 @@
>>>
>>> static const GLenum valid_targets[] = {
>>> GL_TEXTURE_1D,
>>> GL_TEXTURE_1D_ARRAY,
>>> GL_TEXTURE_2D,
>>> GL_TEXTURE_2D_ARRAY,
>>> GL_TEXTURE_3D,
>>> GL_TEXTURE_CUBE_MAP,
>>> GL_TEXTURE_CUBE_MAP_ARRAY,
>>> GL_TEXTURE_RECTANGLE,
>>> - GL_TEXTURE_BUFFER,
>>> GL_RENDERBUFFER,
>>> GL_TEXTURE_2D_MULTISAMPLE,
>>> GL_TEXTURE_2D_MULTISAMPLE_ARRAY,
>>> + GL_TEXTURE_BUFFER,
>>> };
>>>
>>> +#define NUM_TARGETS_EXCEPT_TBO (ARRAY_SIZE(valid_targets) - 1)
>>> +
>>
>> Not sure if this is really needed, as we have texture_targets, defined
>> on common.h too, that is valid_targets except TEXTURE_BUFFER and
>> RENDERBUFFER. The latter also need some special handling.
>>
>>> static const GLenum invalid_targets[] = {
>>> GL_FRAMEBUFFER,
>>> GL_COLOR_ATTACHMENT0,
>>> GL_COLOR_ATTACHMENT1,
>>> GL_COLOR_ATTACHMENT2,
>>> GL_COLOR_ATTACHMENT3,
>>> GL_COLOR_ATTACHMENT4,
>>> GL_COLOR_ATTACHMENT5,
>>> GL_COLOR_ATTACHMENT6,
>>> GL_COLOR_ATTACHMENT7,
>>> diff --git a/tests/spec/arb_internalformat_query2/format-components.c b/tests/spec/arb_internalformat_query2/format-components.c
>>> index a484e01..983091f 100644
>>> --- a/tests/spec/arb_internalformat_query2/format-components.c
>>> +++ b/tests/spec/arb_internalformat_query2/format-components.c
>>> @@ -244,21 +244,21 @@ check_format_components(void)
>>> test_data *data = test_data_new(0, 1);
>>> unsigned i;
>>> int testing64;
>>>
>>> for (i = 0; i < ARRAY_SIZE(pnames); i++) {
>>> bool pass = true;
>>>
>>> for (testing64 = 0; testing64 <= 1; testing64++) {
>>> test_data_set_testing64(data, testing64);
>>>
>>> - pass = try(valid_targets, ARRAY_SIZE(valid_targets),
>>> + pass = try(valid_targets, NUM_TARGETS_EXCEPT_TBO,
>>> valid_internalformats, ARRAY_SIZE(valid_internalformats),
>>> pnames[i], data)
>>> && pass;
>>> }
>>>
>>> piglit_report_subtest_result(pass ? PIGLIT_PASS : PIGLIT_FAIL,
>>> "%s", piglit_get_gl_enum_name(pnames[i]));
>>>
>>> check_pass = check_pass && pass;
>>> }
>>> diff --git a/tests/spec/arb_internalformat_query2/generic-pname-checks.c b/tests/spec/arb_internalformat_query2/generic-pname-checks.c
>>> index e521fac..feb953c 100644
>>> --- a/tests/spec/arb_internalformat_query2/generic-pname-checks.c
>>> +++ b/tests/spec/arb_internalformat_query2/generic-pname-checks.c
>>> @@ -255,21 +255,21 @@ check_basic(const GLenum *pnames, unsigned num_pnames,
>>> test_data *data = test_data_new(0, 1);
>>> unsigned i;
>>> int testing64;
>>>
>>> for (i = 0; i < num_pnames; i++) {
>>> bool pass = true;
>>>
>>> for (testing64 = 0; testing64 <= 1; testing64++) {
>>> test_data_set_testing64(data, testing64);
>>>
>>> - pass = try_basic(valid_targets, ARRAY_SIZE(valid_targets),
>>> + pass = try_basic(valid_targets, NUM_TARGETS_EXCEPT_TBO,
>>> valid_internalformats, ARRAY_SIZE(valid_internalformats),
>>> pnames[i],
>>> possible_values, num_possible_values,
>>> data)
>>> && pass;
>>> }
>>>
>>> piglit_report_subtest_result(pass ? PIGLIT_PASS : PIGLIT_FAIL,
>>> "%s", piglit_get_gl_enum_name(pnames[i]));
>>>
>>> diff --git a/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c b/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
>>> index 28bf292..af46c60 100644
>>> --- a/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
>>> +++ b/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
>>> @@ -138,21 +138,21 @@ try_local(const GLenum *targets, unsigned num_targets,
>>> static bool
>>> check_format_compatibility_type(void)
>>> {
>>> bool pass = true;
>>> test_data *data = test_data_new(0, 1);
>>> int testing64;
>>>
>>> for (testing64 = 0; testing64 <= 1; testing64++) {
>>> test_data_set_testing64(data, testing64);
>>>
>>> - pass = try_local(valid_targets, ARRAY_SIZE(valid_targets),
>>> + pass = try_local(valid_targets, NUM_TARGETS_EXCEPT_TBO,
>>
>> Your point is that TEXTURE_BUFFER shouldn't be tested here. But it is
>> tested in the sense that for this specific pname, querying should return
>> GL_NONE.
>>
>> C&P the comment at try_local:
>> /* From the spec:
>> *
>> * "- IMAGE_FORMAT_COMPATIBILITY_TYPE: The matching criteria use for
>> * the resource when used as an image textures is returned in
>> * <params>. This is equivalent to calling GetTexParameter with
>> * <value> set to IMAGE_FORMAT_COMPATIBILITY_TYPE. Possible values
>> * are IMAGE_FORMAT_COMPATIBILITY_BY_SIZE or
>> * IMAGE_FORMAT_COMPATIBILITY_BY_CLASS. If the resource is not
>> * supported for image textures, or if image textures are not
>> * supported, NONE is returned."
>> *
>> * So try_local is equivalent to try_basic, except that instead of
>> * checking against a list of possible value, we test against the
>> * value returned by GetTexParameter, or against GL_NONE if not
>> * supported of if it is not a texture.
>> */
>>
>> So in this case, TBO is tested (or should be, unless a bug on the test),
>> but the test is just to confirm that we get a GL_NONE. On the code is
>> checked against texture_targets. As it doesn't belong there, it is not
>> used glGetTexParameteriv and just checks if GL_NONE is returned.
>
> This test does use glGetTexParameter(GL_TEXTURE_BUFFER).
>
> Marek
More information about the Piglit
mailing list