[Mesa-dev] [PATCH 4/7] i965/fs: Use image_format_info for doing image_load_store workarounds

[Chad Versace]

On Wed 04 Nov 2015, Jason Ekstrand wrote:
> ---
>  .../drivers/dri/i965/brw_fs_surface_builder.cpp    | 157 ++++++++++++++-------
>  1 file changed, 106 insertions(+), 51 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
> index 31ecb5b..d841ffe 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
> @@ -22,6 +22,9 @@
>   */
>  
>  #include "brw_fs_surface_builder.h"
> +#include "intel_mipmap_tree.h"
> +#include "brw_state.h"
> +#include "brw_image_load_store.h"
>  #include "brw_fs.h"
>  
>  using namespace brw;
> @@ -192,32 +195,80 @@ namespace {
>         * Return the per-channel bitfield widths for a given image format.
>         */
>        inline color_u
> -      get_bit_widths(mesa_format format)
> +      get_bit_widths(uint32_t brw_format)
>        {
> -         return color_u(_mesa_get_format_bits(format, GL_RED_BITS),
> -                        _mesa_get_format_bits(format, GL_GREEN_BITS),
> -                        _mesa_get_format_bits(format, GL_BLUE_BITS),
> -                        _mesa_get_format_bits(format, GL_ALPHA_BITS));
> +         return color_u(brw_image_format_info[brw_format].red_bits,
> +                        brw_image_format_info[brw_format].green_bits,
> +                        brw_image_format_info[brw_format].blue_bits,
> +                        brw_image_format_info[brw_format].alpha_bits);
>        }
>  
>        /**
>         * Return the per-channel bitfield shifts for a given image format.
>         */
>        inline color_u
> -      get_bit_shifts(mesa_format format)
> +      get_bit_shifts(uint32_t brw_format)
>        {
> -         const color_u widths = get_bit_widths(format);
> +         const color_u widths = get_bit_widths(brw_format);
>           return color_u(0, widths.r, widths.r + widths.g,
>                          widths.r + widths.g + widths.b);
>        }
>  
> +      inline unsigned
> +      get_format_bytes(uint32_t brw_format)
> +      {
> +         return (brw_image_format_info[brw_format].red_bits +
> +                 brw_image_format_info[brw_format].green_bits +
> +                 brw_image_format_info[brw_format].blue_bits +
> +                 brw_image_format_info[brw_format].alpha_bits) / 8;

For general formats, this calculation is wrong. I assume this function
is used only for RGBA-formats, though, which makes the function correct.

I have the same comment for other hunks in this patch too.

> +      }
> +
> +      inline unsigned
> +      get_format_num_components(uint32_t brw_format)
> +      {
> +         if (brw_image_format_info[brw_format].green_bits == 0) {
> +            return 1;
> +         } else if (brw_image_format_info[brw_format].blue_bits == 0) {
> +            return 2;
> +         } else {
> +            return 4;
> +         }

This function needs a case for alpha. Either

	} else if (brw_image_format_info[brw_format].alpha_bits == 0) {
		return 3;
	}

or

	} else {
		assert(brw_image_format_info[brw_format].alpha_bits > 0);
		return 4;
	}


The rest of the patch looked good to me.