Re: [Piglit] [PATCH] astc: avoid deleting a random texture

[Nanley Chery]

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

[Ian Romanick]

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, _tex);
> - glDeleteTextures(1, _tex);
> + if (!check_error)
> + glDeleteTextures(1, _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

[Ilia Mirkin]

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, _tex);
>> - glDeleteTextures(1, _tex);
>> + if (!check_error)
>> + glDeleteTextures(1, _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

[Ian Romanick]

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, _tex);
>>> - glDeleteTextures(1, _tex);
>>> + if (!check_error)
>>> + glDeleteTextures(1, _tex);
>>>
>>>   piglit_present_results();
>>>   return pass;
>>>

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH] astc: avoid deleting a random texture

[Ilia Mirkin]

In the check_error case, decompressed_tex is completely uninitialized
and might point to any texture. This can wreak various havoc.

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, _tex);
-   glDeleteTextures(1, _tex);
+   if (!check_error)
+   glDeleteTextures(1, _tex);
 
piglit_present_results();
return pass;
-- 
2.4.10

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit