[Mesa-dev] [PATCH 10/41] intel: Refactor intel_miptree_copy_teximage()

[Kenneth Graunke]

On 11/18/2011 04:04 PM, Chad Versace wrote:
> On 11/18/2011 03:42 PM, Kenneth Graunke wrote:
>> On 11/17/2011 07:58 PM, Chad Versace wrote:
>>> Extract the body of the inner loop into a new function,
>>> intel_miptree_copy_slice().
>>>
>>> This is in preparation for adding support for separate stencil and HiZ to
>>> intel_miptree_copy_teximage(). When copying a slice of a depthstencil
>>> miptree that uses separate stencil, we will also need to copy the
>>> corresponding slice of the stencil miptree. The easiest way to do this
>>> will be to call intel_miptree_copy_slice() recursively. Analogous
>>> reasoning applies to copying a slice of a depth miptree with HiZ.
>>>
>>> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>>> ---
>>>  src/mesa/drivers/dri/intel/intel_mipmap_tree.c |  122 +++++++++++++-----------
>>>  1 files changed, 66 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> index 7f9e606..8f10101 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> @@ -354,6 +354,69 @@ intel_miptree_get_image_offset(struct intel_mipmap_tree *mt,
>>>     }
>>>  }
>>>  
>>> +static void
>>> +intel_miptree_copy_slice(struct intel_context *intel,
>>> +			 struct intel_mipmap_tree *dst_mt,
>>> +			 struct intel_mipmap_tree *src_mt,
>>> +			 int level,
>>> +			 int face,
>>> +			 int depth)
>>> +
>>> +{
>>> +   gl_format format = src_mt->format;
>>> +   uint32_t width = src_mt->level[level].width;
>>> +   uint32_t height = src_mt->level[level].height;
>>> +
>>> +   assert(depth < src_mt->level[level].depth);
>>> +
>>> +   if (dst_mt->compressed) {
>>> +      uint32_t align_w, align_h;
>>> +      intel_get_texture_alignment_unit(format,
>>> +				       &align_w, &align_h);
>>> +      height = ALIGN(height, align_h) / align_h;
>>> +      width = ALIGN(width, align_w);
>>> +   }
>>
>> This wasn't originally inside the loop; you've effectively moved it
>> there.  Since intel_get_texture_alignment_unit actually does some work
>> these days, I'm wondering if this could be a performance hit.
> 
> Patch 36/41 "Store miptree alignment units in the miptree" remove this call
> intel_get_texture_alignment_unit(). Does that solve your performance fear?

Oh... :)  *approval*

For both patches (10 and 36):
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I definitely like storing the values in the miptree itself.

>> At any rate, it doesn't seem necessary...I'd probably just add
>> height/width function parameters and move this hunk back.
> 
> It feels really strange to me to pass width and height parameters
> into intel_miptree_copy_slice(). I imagine someone in the future
> encountering the function and being puzzled: "The slice specifier
> (level, face, depth) is already passed into intel_miptree_copy_slice().
> The width and height are determined by the slice, so why are width and height
> also passed? Are we perhaps not copying the entire slice?" I'd rather not
> make a potentially confusing design choice for the sake of a potential
> performance penalty that will be rendered moot in a future commit.