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

[Chad Versace]

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?

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

----
Chad Versace
chad.versace at linux.intel.com