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

[Jason Ekstrand]

On Mon, Nov 9, 2015 at 2:24 PM, Chad Versace <chad.versace at intel.com> wrote:
> 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

Actually, it doesn't...  All of the brw_fs_surface_builder code
assumes image formats which are all power-of-two so RGB isn't valid.
Maybe I should say something?
--Jason

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