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