[Mesa-dev] [PATCH V2 8/8] i965: Rename intel_miptree_get_dimensions_for_image()

[Anuj Phogat]

On Wed, Sep 16, 2015 at 4:30 PM, Chad Versace <chad.versace at intel.com> wrote:
> On Wed 19 Aug 2015, Anuj Phogat wrote:
>> This function isn't specific to miptrees. So, drop the "miptree"
>> from function name.
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/intel_fbo.c          | 2 +-
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c  | 6 +++---
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h  | 4 ++--
>>  src/mesa/drivers/dri/i965/intel_tex_image.c    | 3 +--
>>  src/mesa/drivers/dri/i965/intel_tex_validate.c | 3 +--
>>  5 files changed, 8 insertions(+), 10 deletions(-)
>
>
>> @@ -928,8 +928,8 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>>  }
>>
>>  void
>> -intel_miptree_get_dimensions_for_image(struct gl_texture_image *image,
>> -                                       int *width, int *height, int *depth)
>> +intel_get_image_dims(struct gl_texture_image *image,
>> +                     int *width, int *height, int *depth)
>>  {
>>     switch (image->TexObject->Target) {
>>     case GL_TEXTURE_1D_ARRAY:
>
> True, the function isn't specific to miptrees. But it *is* specific to
> Intel's RENDER_SURFACE_STATE, as it translates the image's (width,
> height, depth), from the perspective of the OpenGL API, to the needs of
> Intel hardware.
>
> Now that 'miptree' is removed from the function name, the function name
> looks like a mere getter. In that case, it's not clear why the caller
> cannot just access image->width, image->height, and image->depth
> directly.
>
> So that we all don't forget why this function exists next year, please copy
> into the function the relevant portions of this comment from
> intel_miptree_create_layout():
>
>       /* For a 1D Array texture the OpenGL API will treat the height0
>        * parameter as the number of array slices. For Intel hardware, we treat
>        * the 1D array as a 2D Array with a height of 1.
>        *
>        * So, when we first come through this path to create a 1D Array
>        * texture, height0 stores the number of slices, and depth0 is 1. In
>        * this case, we want to swap height0 and depth0.
>        *
>        * Since some miptrees will be created based on the base miptree, we may
>        * come through this path and see height0 as 1 and depth0 being the
>        * number of slices. In this case we don't need to do the swap.
>        */
>
> With such a comment, I think this patch will be ok.
>
I'll send out V3 with added comment.

> By the way, the height<->depth adjustment that intel_miptree_create_layout()
> performs directly beneath that comment, that adjustment duplicates the
> height<-> adjustment done by intel_get_image_dims(). That means you may be able
> to eliminate intel_get_image_dims() completely, and rely on
> intel_miptree_create_layout() to do the adjustment for you. I say "may" because
> I haven't investigated it closely enough to be confident.
After looking at the usage of intel_get_image_dims() at multiple places, I
couldn't see an easy way to remove the function. So, I'll keep it unchanged
at the moment.