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

[Eric Anholt]

Matt Turner <mattst88 at gmail.com> writes:

> 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.

Yes, please!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131112/371d0d8d/attachment.pgp>