I just sent patch version 2 separately. Marek
On Tue, Oct 24, 2017 at 3:15 AM, Marek Olšák <mar...@gmail.com> wrote: > On Sun, Oct 22, 2017 at 12:33 PM, Alejandro Piñeiro > <apinhe...@igalia.com> wrote: >> On 21/10/17 14:55, Marek Olšák wrote: >>> From: Marek Olšák <marek.ol...@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 _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit