[Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy

[Chad Versace]

On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote:
> MESA_FORMAT_XRGB8888 is equivalent to MESA_FORMAT_ARGB8888 in terms
> of storage on the device, so okay to use this optimized copy routine.
>
> This series builds on work from Frank Henigman to optimize the
> process of uploading a texture to the GPU. This series adds support for
> MESA_XRGB_8888 and full miptrees where were found to be common activities
> in the Smokin' Guns game. The issue was found while profiling the app
> but that part is not benchmarked. Smokin-Guns uses mipmap textures with
> an internal format of GL_RGB (MESA_XRGB_8888 in the driver).
>
> These changes need a performance tool to run against to show how they
> improve execution performance for specific texture formats. Using this
> benchmark I've measured the following improvement on my Ivybridge
> Intel(R) Xeon(R) CPU E3-1225 V2 @ 3.20GHz.
>
> Using 1024x1024, RGBA 8888 source, mipmap
>                                  <<THIS PATCH>>

I don't understand. What do you mean by ``<<THIS PATCH>>``? That all these
numbers were obtained with this patch? But that doesn't make sense, because
these are before-and-after numbers. And it can't be just this patch, because
these numbers are identical to the numbers quoted in patch 2.

> internal-format	Before (MB/sec)     XRGB (MB/sec)	mipmap (MB/sec)
> GL_RGBA		628.15		    627.15		615.90
> GL_RGB		265.95	 	    456.35		611.53
> 512x512
> GL_RGBA		600.23	  	    597.00		619.95
> GL_RGB		255.50		    440.62		611.28
> 256x256
> GL_RGBA		489.08		    487.80		587.42
> GL_RGB		229.03	  	    376.63		585.00
>
> Test shows similar pattern for 512x512 and 256x256.

The above table confuses me. There is a column named "Before", but no column
named "After". There 'internal-format' exists in the same location as '512x512'
and '256x256', but 'internal-format' is not a size.

> Benchmark has been sent to mesa-dev list: teximage_enh


---- -8<- ----
> Courtney Goeltzenleuchter (2):
>    i965: add XRGB to tiled_memcpy
>    i965: Enhance tiled_memcpy to support all levels
>
>   src/mesa/drivers/dri/i965/intel_tex_subimage.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> --
> 1.8.1.2
---- ->8- ----

Commit messages should not contain coverletter metadata, such
as the above snippet. This also applies to patch 2.

> Signed-off-by: Courtney Goeltzenleuchter <courtney at LunarG.com>
> ---
>   src/mesa/drivers/dri/i965/intel_tex_subimage.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_subimage.c b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> index 0384bcc..b1826fa 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> @@ -571,7 +571,8 @@ intel_texsubimage_tiled_memcpy(struct gl_context * ctx,
>          (texImage->TexFormat == MESA_FORMAT_A8 && format == GL_ALPHA)) {
>         cpp = 1;
>         mem_copy = memcpy;
> -   } else if (texImage->TexFormat == MESA_FORMAT_ARGB8888) {
> +   } else if ((texImage->TexFormat == MESA_FORMAT_ARGB8888)
> +         || (texImage->TexFormat == MESA_FORMAT_XRGB8888)) {
>         cpp = 4;
>         if (format == GL_BGRA) {
>            mem_copy = memcpy;
>

The code change itself looks good. I'm just having a hard time making sense
out of the commit message.