<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Nov 23, 2015 at 1:28 PM, Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu" target="_blank">imirkin@alum.mit.edu</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Nov 23, 2015 at 4:16 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: > On 11/23/2015 12:43 PM, Ilia Mirkin wrote: >> On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> 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. >>> </blockquote><div> </div><div>Thanks for noticing this issue. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> >>> 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? </blockquote><div> I actually have a patch sitting on the list which redoes some of the logic: <a href="http://lists.freedesktop.org/archives/piglit/2015-October/017743.html" target="_blank">http://lists.freedesktop.org/archives/piglit/2015-October/017743.html</a> The test previously wasn't splitting up into subtests correctly. - Nanley </div></div></div></div>