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.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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
