On Wed, Jun 20, 2018 at 09:35:21PM +0100, Lionel Landwerlin wrote:
> 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 :)
>
Ah, okay :). Thanks for the reviews!
-Nanley
> >
> > -Nanley
> >
> > > I guess it's a common enough format that everybody probably supports it.
> > >
> > > Thanks for the explanation :
> > >
> > >
> > > Reviewed-by: Lionel Landwerlin <lionel.g.landwer...@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);
>
>
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
