On Wed, Sep 16, 2015 at 4:30 PM, Chad Versace <chad.vers...@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.pho...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
