[Mesa-dev] [PATCH 1/2 v3] i965: add XRGB to tiled_memcpy
Chad Versace
chad.versace at linux.intel.com
Tue Nov 12 12:22:06 PST 2013
+mesa-dev
On 11/12/2013 09:16 AM, Courtney Goeltzenleuchter wrote:
> On Mon, Nov 11, 2013 at 2:19 PM, Chad Versace
> <chad.versace at linux.intel.com>wrote:
>
>> 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.
>
>
> 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.
texture internal
size format baseline patch1 patch2
--------------------------------------------
1024x1024 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
>>
>> 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.
>>
>
>
>
More information about the mesa-dev
mailing list