Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
Hi Chad, Does Matt's explanation help? I wanted to tell the performance story and show the progression of performance enhancements with these two patches. Thanks, Courtney On Mon, Nov 11, 2013 at 3:01 PM, Matt Turner matts...@gmail.com wrote: On Mon, Nov 11, 2013 at 1:19 PM, Chad Versace chad.vers...@linux.intel.com wrote: On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote: MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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. The first column is before, the second is after patch 1, and the third is after patch 2. Instead of doing before-after patch 1 in patch 1's commit, and after patch 1-after patch 2 in patch 2's commit, he just pasted the same data in and added THIS PATCH above the column that corresponds to the patch. -- Courtney Goeltzenleuchter LunarG ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
+mesa-dev On 11/12/2013 09:16 AM, Courtney Goeltzenleuchter wrote: On Mon, Nov 11, 2013 at 2:19 PM, Chad Versace chad.vers...@linux.intel.comwrote: On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote: MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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. The first two rows were measured using a 1024x1024 texture. Then comes the 512x512 two rows and finally the 256x256 measurements. How's this? THIS PATCH 1024x1024 texture size 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 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 600.23 597.00 619.95 GL_RGB 255.50 440.62 611.28 256x256 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 489.08 487.80 587.42 GL_RGB 229.03 376.63 585.00 This formatting makes more sense. The THIS PATCH thing still doesn't make sense out-of-context, though. If someone uses git-show or git-log to look at this patch, the presence of three columns and THIS PATCH is not self-explanatory. In short, each commit message should either be self-explaining in isolation, or refer to another patch for additional context. Otherwise, the commit message doesn't help where it supposed to help: when someone looks at your patch after you're no longer around. How about something like this instead? I think this table would make sense in the commit message for both your patches. Numbers are MB/sec. textureinternal size format baseline patch1 patch2 1024x1024 GL_RGBA 628.15627.15 615.90 GL_RGB 265.95456.35 611.53 512x512GL_RGBA 600.23597.00 619.95 GL_RGB 255.50440.62 611.28 256x256GL_RGBA 489.08487.80 587.42 GL_RGB 229.03376.63 585.00 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 court...@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 ==
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
On Tue, Nov 12, 2013 at 12:22 PM, Chad Versace chad.vers...@linux.intel.com wrote: +mesa-dev On 11/12/2013 09:16 AM, Courtney Goeltzenleuchter wrote: On Mon, Nov 11, 2013 at 2:19 PM, Chad Versace chad.vers...@linux.intel.comwrote: On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote: MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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. The first two rows were measured using a 1024x1024 texture. Then comes the 512x512 two rows and finally the 256x256 measurements. How's this? THIS PATCH 1024x1024 texture size 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 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 600.23 597.00 619.95 GL_RGB 255.50 440.62 611.28 256x256 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 489.08 487.80 587.42 GL_RGB 229.03 376.63 585.00 This formatting makes more sense. The THIS PATCH thing still doesn't make sense out-of-context, though. If someone uses git-show or git-log to look at this patch, the presence of three columns and THIS PATCH is not self-explanatory. In short, each commit message should either be self-explaining in isolation, or refer to another patch for additional context. Otherwise, the commit message doesn't help where it supposed to help: when someone looks at your patch after you're no longer around. How about something like this instead? I think this table would make sense in the commit message for both your patches. Numbers are MB/sec. textureinternal size format baseline patch1 patch2 1024x1024 GL_RGBA 628.15627.15 615.90 Let's drop all of this multi-patch table nonsense and just say what each patch does in its commit message. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
On 11/12/2013 12:50 PM, Matt Turner wrote: On Tue, Nov 12, 2013 at 12:22 PM, Chad Versace Let's drop all of this multi-patch table nonsense and just say what each patch does in its commit message. Sounds good to me. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
Matt Turner matts...@gmail.com writes: On Tue, Nov 12, 2013 at 12:22 PM, Chad Versace chad.vers...@linux.intel.com wrote: +mesa-dev On 11/12/2013 09:16 AM, Courtney Goeltzenleuchter wrote: On Mon, Nov 11, 2013 at 2:19 PM, Chad Versace chad.vers...@linux.intel.comwrote: On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote: MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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. The first two rows were measured using a 1024x1024 texture. Then comes the 512x512 two rows and finally the 256x256 measurements. How's this? THIS PATCH 1024x1024 texture size 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 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 600.23 597.00 619.95 GL_RGB 255.50 440.62 611.28 256x256 texture size internal-format Before (MB/sec) XRGB (MB/sec) mipmap (MB/sec) GL_RGBA 489.08 487.80 587.42 GL_RGB 229.03 376.63 585.00 This formatting makes more sense. The THIS PATCH thing still doesn't make sense out-of-context, though. If someone uses git-show or git-log to look at this patch, the presence of three columns and THIS PATCH is not self-explanatory. In short, each commit message should either be self-explaining in isolation, or refer to another patch for additional context. Otherwise, the commit message doesn't help where it supposed to help: when someone looks at your patch after you're no longer around. How about something like this instead? I think this table would make sense in the commit message for both your patches. Numbers are MB/sec. textureinternal size format baseline patch1 patch2 1024x1024 GL_RGBA 628.15627.15 615.90 Let's drop all of this multi-patch table nonsense and just say what each patch does in its commit message. Yes, please! pgpb8eGMEIMO5.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote:
MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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 court...@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_ARGB) {
+ } else if ((texImage-TexFormat == MESA_FORMAT_ARGB)
+ || (texImage-TexFormat == MESA_FORMAT_XRGB)) {
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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
On Mon, Nov 11, 2013 at 1:19 PM, Chad Versace chad.vers...@linux.intel.com wrote: On 11/07/2013 01:59 PM, Courtney Goeltzenleuchter wrote: MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 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. The first column is before, the second is after patch 1, and the third is after patch 2. Instead of doing before-after patch 1 in patch 1's commit, and after patch 1-after patch 2 in patch 2's commit, he just pasted the same data in and added THIS PATCH above the column that corresponds to the patch. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
MESA_FORMAT_XRGB is equivalent to MESA_FORMAT_ARGB 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_ 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_ 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 source, mipmap
THIS PATCH
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.
Benchmark has been sent to mesa-dev list: teximage_enh
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
Signed-off-by: Courtney Goeltzenleuchter court...@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_ARGB) {
+ } else if ((texImage-TexFormat == MESA_FORMAT_ARGB)
+ || (texImage-TexFormat == MESA_FORMAT_XRGB)) {
cpp = 4;
if (format == GL_BGRA) {
mem_copy = memcpy;
--
1.8.1.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
