Re: [Piglit] [PATCH] astc: avoid deleting a random texture
On Mon, Nov 23, 2015 at 1:28 PM, Ilia Mirkin wrote: > On Mon, Nov 23, 2015 at 4:16 PM, Ian Romanick wrote: > > On 11/23/2015 12:43 PM, Ilia Mirkin wrote: > >> On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick > wrote: > >>> On 11/21/2015 04:08 PM, Ilia Mirkin wrote: > In the check_error case, decompressed_tex is completely uninitialized > and might point to any texture. This can wreak various havoc. > >>> > Thanks for noticing this issue. > >>> I might suggest a different approach. In the check_error case, ensure > >>> that decompress_texture is zero. Remove the check_error parameter, and > >>> s/!check_error/decompress_texture == > >>> 0/g;s/check_error/decompress_texture != 0/g. > >> > >> Is it legal to delete texture 0? Or can texture 0 be a real thing? > >> Otherwise I can just initialize it to that below. > > > > It's similar to free(NULL) in that respect. From the glDeleteTextures > > man page: > > > >glDeleteTextures silently ignores 0's and names that do not > correspond > >to existing textures. > > Ah that's perfect. I should have checked the man page when I was > futzing with the test. > > > > >> Separately, how would you feel about making the error color decoding > >> check failing return a warn instead of a fail? I can't get Adreno A420 > >> to spit it out... haven't tried on blob, perhaps I'm missing some bit > >> of programming. I just get black with the HDR data instead of the > >> error color. > > > > Hmm... I don't know how this particular test is structured. When we've > > had cases like this in the past, we've split the test into subcases. > > One (or several) subcase checks the things that the driver in question > > can pass, and one, single subcase checks the thing that the driver > > fails. > > > > If this test is conducive that kind of a split (perhaps by running it > > twice with different data), that would be my preference. > > Right now there are 3 subtests (ldr, hdr, srgb), each of which tests > loading 3 sets of data (ldr, hdr, and ldr srgb). If there's no > astc_hdr support, the hdr subtest isn't run, and when loading hdr > data, it checks for the error color. Instead of 3 subtests, I could > split this up into 9 subtests for the full cross -- does that sound > reasonable, and then we'd pass the ldr-data ones and fail the hdr data > ones. > > TBH I'm unsure what the diff between the ldr and hdr subtests is... > they seem identical except that HDR one skips if you don't hdr. But > otherwise it's identical to the LDR one from what I can tell. Also > from the looks of it the srgb test doesn't need the full cross either, > it only runs against the ldrs data. Urgh, I guess this test needs a > bit of a redo :( Perhaps we can sucker Nanley into fixing it up? > I actually have a patch sitting on the list which redoes some of the logic: http://lists.freedesktop.org/archives/piglit/2015-October/017743.html The test previously wasn't splitting up into subtests correctly. - Nanley ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] astc: avoid deleting a random texture
On Mon, Nov 23, 2015 at 4:16 PM, Ian Romanick wrote: > On 11/23/2015 12:43 PM, Ilia Mirkin wrote: >> On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick wrote: >>> On 11/21/2015 04:08 PM, Ilia Mirkin wrote: In the check_error case, decompressed_tex is completely uninitialized and might point to any texture. This can wreak various havoc. >>> >>> I might suggest a different approach. In the check_error case, ensure >>> that decompress_texture is zero. Remove the check_error parameter, and >>> s/!check_error/decompress_texture == >>> 0/g;s/check_error/decompress_texture != 0/g. >> >> Is it legal to delete texture 0? Or can texture 0 be a real thing? >> Otherwise I can just initialize it to that below. > > It's similar to free(NULL) in that respect. From the glDeleteTextures > man page: > >glDeleteTextures silently ignores 0's and names that do not correspond >to existing textures. Ah that's perfect. I should have checked the man page when I was futzing with the test. > >> Separately, how would you feel about making the error color decoding >> check failing return a warn instead of a fail? I can't get Adreno A420 >> to spit it out... haven't tried on blob, perhaps I'm missing some bit >> of programming. I just get black with the HDR data instead of the >> error color. > > Hmm... I don't know how this particular test is structured. When we've > had cases like this in the past, we've split the test into subcases. > One (or several) subcase checks the things that the driver in question > can pass, and one, single subcase checks the thing that the driver > fails. > > If this test is conducive that kind of a split (perhaps by running it > twice with different data), that would be my preference. Right now there are 3 subtests (ldr, hdr, srgb), each of which tests loading 3 sets of data (ldr, hdr, and ldr srgb). If there's no astc_hdr support, the hdr subtest isn't run, and when loading hdr data, it checks for the error color. Instead of 3 subtests, I could split this up into 9 subtests for the full cross -- does that sound reasonable, and then we'd pass the ldr-data ones and fail the hdr data ones. TBH I'm unsure what the diff between the ldr and hdr subtests is... they seem identical except that HDR one skips if you don't hdr. But otherwise it's identical to the LDR one from what I can tell. Also from the looks of it the srgb test doesn't need the full cross either, it only runs against the ldrs data. Urgh, I guess this test needs a bit of a redo :( Perhaps we can sucker Nanley into fixing it up? -ilia ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] astc: avoid deleting a random texture
On 11/23/2015 12:43 PM, Ilia Mirkin wrote: > On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick wrote: >> On 11/21/2015 04:08 PM, Ilia Mirkin wrote: >>> In the check_error case, decompressed_tex is completely uninitialized >>> and might point to any texture. This can wreak various havoc. >> >> I might suggest a different approach. In the check_error case, ensure >> that decompress_texture is zero. Remove the check_error parameter, and >> s/!check_error/decompress_texture == >> 0/g;s/check_error/decompress_texture != 0/g. > > Is it legal to delete texture 0? Or can texture 0 be a real thing? > Otherwise I can just initialize it to that below. It's similar to free(NULL) in that respect. From the glDeleteTextures man page: glDeleteTextures silently ignores 0's and names that do not correspond to existing textures. > Separately, how would you feel about making the error color decoding > check failing return a warn instead of a fail? I can't get Adreno A420 > to spit it out... haven't tried on blob, perhaps I'm missing some bit > of programming. I just get black with the HDR data instead of the > error color. Hmm... I don't know how this particular test is structured. When we've had cases like this in the past, we've split the test into subcases. One (or several) subcase checks the things that the driver in question can pass, and one, single subcase checks the thing that the driver fails. If this test is conducive that kind of a split (perhaps by running it twice with different data), that would be my preference. >> It would also be cool if this test used piglit_draw_rect_tex instead of >> open-coding it. > > It would be even cooler if this test passed for me with -auto as well > as without it... for now it only passes without -auto for me. I'm at a > total loss as to what the difference is. I'm blaming DRI3 and GLAMOR. > Anyways, I'm not in the business of making every piglit test pretty :) > "It was like that when I got there." Fair enough. You can't blame me for trying. > -ilia > >> >>> Signed-off-by: Ilia Mirkin >>> --- >>> tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c | 3 >>> ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git >>> a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >>> b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >>> index 20f2415..d9c1c30 100644 >>> --- a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >>> +++ b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >>> @@ -213,7 +213,8 @@ bool draw_compare_levels(bool check_error, bool >>> check_srgb, >>> >>> /* Delete bound textures */ >>> glDeleteTextures(1, &compressed_tex); >>> - glDeleteTextures(1, &decompressed_tex); >>> + if (!check_error) >>> + glDeleteTextures(1, &decompressed_tex); >>> >>> piglit_present_results(); >>> return pass; >>> ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] astc: avoid deleting a random texture
On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick wrote: > On 11/21/2015 04:08 PM, Ilia Mirkin wrote: >> In the check_error case, decompressed_tex is completely uninitialized >> and might point to any texture. This can wreak various havoc. > > I might suggest a different approach. In the check_error case, ensure > that decompress_texture is zero. Remove the check_error parameter, and > s/!check_error/decompress_texture == > 0/g;s/check_error/decompress_texture != 0/g. Is it legal to delete texture 0? Or can texture 0 be a real thing? Otherwise I can just initialize it to that below. Separately, how would you feel about making the error color decoding check failing return a warn instead of a fail? I can't get Adreno A420 to spit it out... haven't tried on blob, perhaps I'm missing some bit of programming. I just get black with the HDR data instead of the error color. > > It would also be cool if this test used piglit_draw_rect_tex instead of > open-coding it. It would be even cooler if this test passed for me with -auto as well as without it... for now it only passes without -auto for me. I'm at a total loss as to what the difference is. I'm blaming DRI3 and GLAMOR. Anyways, I'm not in the business of making every piglit test pretty :) "It was like that when I got there." -ilia > >> Signed-off-by: Ilia Mirkin >> --- >> tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c | 3 >> ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git >> a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >> b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >> index 20f2415..d9c1c30 100644 >> --- a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >> +++ b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c >> @@ -213,7 +213,8 @@ bool draw_compare_levels(bool check_error, bool >> check_srgb, >> >> /* Delete bound textures */ >> glDeleteTextures(1, &compressed_tex); >> - glDeleteTextures(1, &decompressed_tex); >> + if (!check_error) >> + glDeleteTextures(1, &decompressed_tex); >> >> piglit_present_results(); >> return pass; >> > ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] astc: avoid deleting a random texture
On 11/21/2015 04:08 PM, Ilia Mirkin wrote: > In the check_error case, decompressed_tex is completely uninitialized > and might point to any texture. This can wreak various havoc. I might suggest a different approach. In the check_error case, ensure that decompress_texture is zero. Remove the check_error parameter, and s/!check_error/decompress_texture == 0/g;s/check_error/decompress_texture != 0/g. It would also be cool if this test used piglit_draw_rect_tex instead of open-coding it. > Signed-off-by: Ilia Mirkin > --- > tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git > a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c > b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c > index 20f2415..d9c1c30 100644 > --- a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c > +++ b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c > @@ -213,7 +213,8 @@ bool draw_compare_levels(bool check_error, bool > check_srgb, > > /* Delete bound textures */ > glDeleteTextures(1, &compressed_tex); > - glDeleteTextures(1, &decompressed_tex); > + if (!check_error) > + glDeleteTextures(1, &decompressed_tex); > > piglit_present_results(); > return pass; > ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit