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

[Matt Turner]

On Tue, Nov 12, 2013 at 12:22 PM, Chad Versace
<chad.versace at 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.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

Let's drop all of this multi-patch table nonsense and just say what
each patch does in its commit message.