[Piglit] [PATCH v2 4/4] getteximage-simple: Stress an i965 blitter path

[Lionel Landwerlin]

On 20/06/18 20:49, Nanley Chery wrote:
> On Wed, Jun 20, 2018 at 06:32:58PM +0100, Lionel Landwerlin wrote:
>> On 19/06/18 18:53, Nanley Chery wrote:
>>> On Tue, Jun 19, 2018 at 11:03:17AM +0100, Lionel Landwerlin wrote:
>>>> On 18/06/18 19:57, Nanley Chery wrote:
>>>>> Change the dimension of the texture to test how i965 handles having a
>>>>> row pitch too large for the BLT engine.
>>>>>
>>>>> v2: Query the maximum supported texture width (Ilia Mirkin)
>>>>> ---
>>>>>     tests/texturing/getteximage-simple.c | 25 +++++++++++++++++--------
>>>>>     1 file changed, 17 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/tests/texturing/getteximage-simple.c b/tests/texturing/getteximage-simple.c
>>>>> index 525e6a392..5b87b7ccd 100644
>>>>> --- a/tests/texturing/getteximage-simple.c
>>>>> +++ b/tests/texturing/getteximage-simple.c
>>>>> @@ -8,6 +8,10 @@
>>>>>      * texture images is executed before the readback.
>>>>>      *
>>>>>      * This used to crash for R300+bufmgr.
>>>>> + *
>>>>> + * This also used to stress test the blit methods in i965. The BLT engine only
>>>>> + * supports pitch sizes up to but not including 32768 dwords. BLORP supports
>>>>> + * even larger sizes.
>>>>>      */
>>>>>     #include "piglit-util-gl.h"
>>>>> @@ -21,10 +25,10 @@ PIGLIT_GL_TEST_CONFIG_BEGIN
>>>>>     PIGLIT_GL_TEST_CONFIG_END
>>>>> -#define MAX_TYPE_VAL UINT8_MAX
>>>>> -#define PIX_TYPE GLubyte
>>>>> -#define TEX_TYPE GL_UNSIGNED_BYTE
>>>>> -#define TEX_INT_FMT GL_RGBA8
>>>>> +#define MAX_TYPE_VAL 1.0
>>>>> +#define PIX_TYPE GLfloat
>>>>> +#define TEX_TYPE GL_FLOAT
>>>>> +#define TEX_INT_FMT GL_RGBA32F
>>>>>     #define TEX_FMT GL_RGBA
>>>>>     #define CHANNELS_PER_PIXEL 4
>>>> This is changing the format of the texture we're testing with.
>>> Yes, a higher resolution format is needed to hit the row pitch of
>>> interest on the older gens which have a max width of 8K.
>>>
>>>> In patch 3 I thought you wanted to make this configurable (through arguments
>>>> to the test maybe?), but this is just switching the format.
>>> In patch 3, I attempted to make the test configurable via a group of
>>> #defines.
>>>
>>>> Why not add a command line parameter?
>>>>
>>> I didn't want to add a command line parameter because there didn't seem
>>> to be any benefit to doing so. By changing the format and width, we're
>>> able to continue testing the issue in R300 as well as the teximage
>>> upload and download paths of interest in i965. Thoughts?
>> Not really, I was just wondering whether that format change would mean that
>> some driver wouldn't get tested anymore.
> Sorry for not mentioning it in the cover letter, but we do lose testing
> on i915 which doesn't support GL_ARB_texture_float. I didn't feel bad
> about this because it's not actively developed and the test was written
> to catch a regression in the R300 driver (which does support the
> extension AFACIT). Is that okay with you?

Yeah, I just wasn't sure whether you thought about it :)

>
> -Nanley
>
>> I guess it's a common enough format that everybody probably supports it.
>>
>> Thanks for the explanation :
>>
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>
>>> -Nanley
>>>
>>>> Thanks,
>>>>
>>>> -
>>>> Lionel
>>>>
>>>>> @@ -42,8 +46,8 @@ static bool test_getteximage(PIX_TYPE *data, size_t data_size, GLint w, GLint h)
>>>>>     			const unsigned pixel_channel = i % CHANNELS_PER_PIXEL;
>>>>>     			printf("GetTexImage() returns incorrect data in element %i\n", i);
>>>>>     			printf("    corresponding to (%i,%i) channel %i\n", pixel % w, pixel / w, pixel_channel);
>>>>> -			printf("    expected: %i\n", data[i]);
>>>>> -			printf("    got: %i\n", compare[i]);
>>>>> +			printf("    expected: %f\n", data[i]);
>>>>> +			printf("    got: %f\n", compare[i]);
>>>>>     			match = false;
>>>>>     			break;
>>>>>     		}
>>>>> @@ -53,11 +57,13 @@ static bool test_getteximage(PIX_TYPE *data, size_t data_size, GLint w, GLint h)
>>>>>     	return match;
>>>>>     }
>>>>> +
>>>>>     enum piglit_result
>>>>>     piglit_display(void)
>>>>>     {
>>>>> -	GLsizei height = 16;
>>>>> -	GLsizei width = 64;
>>>>> +	GLsizei height = 2;
>>>>> +	GLsizei width;
>>>>> +	glGetIntegerv(GL_MAX_TEXTURE_SIZE, &width);
>>>>>     	/* Upload random data to a texture with the given dimensions */
>>>>>     	const unsigned data_channels = width * height * CHANNELS_PER_PIXEL;
>>>>> @@ -94,6 +100,9 @@ piglit_display(void)
>>>>>     void piglit_init(int argc, char **argv)
>>>>>     {
>>>>> +	if (TEX_TYPE == GL_FLOAT)
>>>>> +		piglit_require_extension("GL_ARB_texture_float");
>>>>> +
>>>>>     	GLuint tex;
>>>>>     	glGenTextures(1, &tex);