[Piglit] [PATCH 3/4] Add a fast clear test for non-MSRT surfaces

[Pohjolainen, Topi]

On Tue, Dec 01, 2015 at 02:47:50PM +0000, Neil Roberts wrote:
> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> 
> > On Tue, Dec 01, 2015 at 02:18:59PM +0000, Neil Roberts wrote:
> >> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> >> 
> >> >> +	if (single_sample) {
> >> >> +		glTexParameteri(tex_target,
> >> >> +				GL_TEXTURE_MAG_FILTER,
> >> >> +				GL_NEAREST);
> >> >> +		glTexParameteri(tex_target,
> >> >> +				GL_TEXTURE_MIN_FILTER,
> >> >> +				GL_NEAREST);
> >> >> +		glTexParameteri(tex_target,
> >> >> +				GL_TEXTURE_MAX_LEVEL,
> >> >> +				0);
> >> >> +		glTexImage2D(tex_target,
> >> >> +			     0, /* level */
> >> >> +			     format->internalformat,
> >> >> +			     128, 128, /* width/height */
> >> >
> >> > Do we need 128x128 instead of simply 1x1 for the driver to take a
> >> > different path?
> >> 
> >> Yes that's right, tiling is disabled for textures with a height of 1 or
> >> with a pitch smaller than a tile and fast clears are disabled for
> >> non-tiled surfaces.
> >> 
> >> >> +			     0, /* border */
> >> >> +			     GL_RGBA, GL_UNSIGNED_BYTE,
> >> >> +			     NULL);
> >> >> +	} else {
> >> >> +		piglit_reset_gl_error();
> >> >> +
> >> >> +		glTexImage2DMultisample(tex_target,
> >> >> +					2, /* samples */
> >> >> +					format->internalformat,
> >> >> +					128, 128, /* width/height */
> >> >
> >> > Before this was 1x1, it is not clear to me why we need to change this? Or do
> >> > we?
> >> 
> >> You're right, there's no reason to change it. The size is completely
> >> arbitrary here but I thought it looked a bit weird to have two different
> >> sizes. Maybe I should explain this in the commit message? It would be
> >> nice if you could put commit comments directly inline with the diff
> >> somehow :)
> >
> > Should we explain the "glTexImage2D()" case in the test - the fact that i965
> > needs large enough buffer for tiling to be used. And then perhaps another
> > comment here explaining that in multisample case there is no such restriction.
> >
> > If we had something in the commit only one would need to be smart enough to
> > go and see the commit in order to understand where the sizes come from. What
> > do you think?
> 
> That sounds sensible, yes. I will make a v2.

And you can add to all four patches:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>