[Mesa-dev] [PATCH] i965: Fast texture upload now supports all levels

[Chad Versace]

On 10/13/2013 08:33 PM, Ian Romanick wrote:
> On 10/13/2013 01:50 PM, Frank Henigman wrote:
>> On Fri, Oct 11, 2013 at 10:00 PM, Chad Versace
>> <chad.versace at linux.intel.com> wrote:
>>> On 10/11/2013 10:17 AM, Courtney Goeltzenleuchter wrote:
>>>>
>>>> Support all levels of a supported texture format.
>>>> ---
>>>>    src/mesa/drivers/dri/i965/intel_tex_subimage.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/intel_tex_subimage.c
>>>> b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
>>>> index 4aec05d..5e46760 100644
>>>> --- a/src/mesa/drivers/dri/i965/intel_tex_subimage.c
>>>> +++ b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
>>>> @@ -541,14 +541,13 @@ intel_texsubimage_tiled_memcpy(struct gl_context *
>>>> ctx,
>>>>       uint32_t cpp;
>>>>       mem_copy_fn mem_copy = NULL;
>>>>
>>>> -   /* This fastpath is restricted to specific texture types: level 0 of
>>>> +   /* This fastpath is restricted to specific texture types:
>>>>        * a 2D BGRA, RGBA, L8 or A8 texture. It could be generalized to
>>>> support
>>>>        * more types.
>>>>        */
>>>>       if (!brw->has_llc ||
>>>>           type != GL_UNSIGNED_BYTE ||
>>>>           texImage->TexObject->Target != GL_TEXTURE_2D ||
>>>> -       texImage->Level != 0 ||
>>>>           pixels == NULL ||
>>>>           _mesa_is_bufferobj(packing->BufferObj) ||
>>>>           packing->Alignment > 4 ||
>>>> @@ -616,6 +615,16 @@ intel_texsubimage_tiled_memcpy(struct gl_context *
>>>> ctx,
>>>>       DBG("%s: level=%d offset=(%d,%d) (w,h)=(%d,%d)\n",
>>>>           __FUNCTION__, texImage->Level, xoffset, yoffset, width, height);
>>>>
>>>> +   /* Adjust x and y offset based on miplevel
>>>> +    */
>>>> +   if (texImage->Level) {
>>>> +      GLuint xlevel, ylevel;
>>>> +      intel_miptree_get_image_offset(image->mt, texImage->Level, 0,
>>>> +                                  &xlevel, &ylevel);
>>>> +      xoffset += xlevel;
>>>> +      yoffset += ylevel;
>>>> +   }
>>>> +
>>>>       linear_to_tiled(
>>>>          xoffset * cpp, (xoffset + width) * cpp,
>>>>          yoffset, yoffset + height,
>>>>
>>>
>>> Usually when we commit performance patches like this, we state in the
>>> commit message what the observed relative performance gain.
>>>
>>> What gain did you see? Hardware? Benchmark? Kernel version? How many
>>> runs?
>>
>> We could quote from my patch, as this is just opening more paths into that code.
>> Or do you think this calls for different testing?
>
> I think what Chad is asking is whether there's some information like
> "Improves load time of application XYZ 12.3+4.5%" or similar.
>
> In the past, we've had problems with patches that just make vague claims
> of "improves performance" when we later find critical bugs in those
> patches... can we just revert the code, or is it going to run the
> performance of... something?
>
> For reference, see commit 329cd6a9b and this thread from mesa-dev:
>
> http://lists.freedesktop.org/archives/mesa-dev/2013-June/040811.html

Ian read my mind correctly. The commit message should say "Improves XYZ of
application ABC by 10.3+-1.2%", as well as state the hardware at a minimum,
and kernel version too if you're feeling gracious.

In the future, if someone discover that this patch introduces a bug, the commit
message's performance claim will prevent that someone from simply reverting the
code.