On Mon, Nov 9, 2015 at 2:24 PM, Chad Versace <chad.vers...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev