On Tue, 2014-11-18 at 13:41 -0800, Jason Ekstrand wrote: > Iago, > > Most of this looks pretty good to me. The one primary concern I have > is in the handling of integer formats. I made the comment in a couple > of patches, but I'll make it in general here. In a lot of the code, > when you convert from integer formats to float, you treat them as if > they are normalized. Can you explain why you are doing this? It > seems very wrong to me.
Right, I have been discussing this with Samuel and it does look wrong. He will change the code and run a new piglit run to verify the changes. > > One other issue is that I couldn't actually get it to compile. This > is probably due to the fact that I always build out-of-tree, so > sourcedir and builddir are not the same. Not really sure what's going > on there. Mmm... that's weird. I think I remember seeing a patch that added a new file and could be the source of that issue. We will look into it. > > Other than that, It's looking pretty good. I'll try and get to > reviewing your second patch series tomorrow. Since my R-B obviously > doesn't mean much on the code I wrote I'll try and dig up a second > reviewer as well. Yes, that makes sense. Thanks for looking into these patches so fast! Iago > --Jason > > > On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga > <ito...@igalia.com> wrote: > This is the fist of two series of patches to address: > https://bugs.freedesktop.org/show_bug.cgi?id=84566 > > The idea is that we have a lot of format conversion code > scattered through > different files in the repository, a lot of that is > redundant / duplicated, > so this intends to address that issue. > > The goal of this first series is to address auto-generation of > our pack/unpack > functions (format_pack.c and format_unpack.c). Currently, we > have a ton of > hand-coded pack/unpack functions for lots of formats, but we > can auto-generate > most of that code instead, so this series handles this. > > This is based on initial work by Jason Ekstrand. > > Tested on i965, classic swrast and gallium (radeon, nouveau, > llvmpipe) without > regressions. > > For software drivers we worked with a trimmed set of piglit > tests (related to > format conversion), ~5700 tests selected with the following > filter: > > -t format -t color -t tex -t image -t swizzle -t clamp -t rgb > -t lum -t pix > -t fbo -t frame > > Summary of the patches: > * Patches 1-7 are general fixes to the current code that were > found while > working on this. > * Patches 8-16 implement auto-generation of pack/unpack > functions. > * Patches 17-20 make use of the auto-generated pack/unpack > functions in > various places to simplify the current code. > > Notice that some of the fixes in patches 1-7 will become > obsolete as soon as > we auto-generate the pack/unpack functions, but we thought it > would make sense > to keep them in the patch set anyway since we started from > that base and they > should be correct fixes to the currently existing code. > > Iago Toral Quiroga (1): > swrast: Remove unused variable. > > Jason Ekstrand (9): > mesa/format_utils: Fix a bug in one of the format helper > functions > mesa: Fix packing/unpacking of MESA_FORMAT_R5G6B5_UNORM > mesa/colormac: Remove an unused macro > mesa: Fix A1R5G5B5 packing/unpacking > mesa/format_utils: Prefix and expose the conversion helper > functions > mesa: Add a concept of an array format > mesa: Add a _mesa_is_format_color_format helper > mesa: Autogenerate most of format_pack.c > mesa: Autogenerate format_unpack.c > > Samuel Iglesias Gonsalvez (10): > mesa: Fix get_texbuffer_format(). > mesa: Fix _mesa_swizzle_and_convert integer conversions to > clamp > properly > mesa: Add _mesa_pack_uint_rgba_row() format conversion > function > mesa: Add non-normalized formats support for ubyte packing > functions > mesa/format_pack: Add _mesa_pack_int_rgba_row() > mesa/formats: add new mesa formats and their pack/unpack > functions. > mesa: use format conversion functions in swrast > mesa/pack: use autogenerated format_pack functions > mesa/main/pack_tmp.h: Add float conversion support > mesa/pack: refactor _mesa_pack_rgba_span_float() > > src/mesa/Makefile.am | 18 + > src/mesa/Makefile.sources | 4 +- > src/mesa/main/colormac.h | 3 - > src/mesa/main/format_convert.py | 71 + > src/mesa/main/format_info.py | 41 + > src/mesa/main/format_pack.c | 2994 > ------------------------ > src/mesa/main/format_pack.c.mako | 1147 ++++++++++ > src/mesa/main/format_pack.h | 6 + > src/mesa/main/format_unpack.c | 4400 > ------------------------------------ > src/mesa/main/format_unpack.c.mako | 914 ++++++++ > src/mesa/main/format_utils.c | 248 +- > src/mesa/main/format_utils.h | 105 + > src/mesa/main/formats.c | 193 +- > src/mesa/main/formats.csv | 13 + > src/mesa/main/formats.h | 73 + > src/mesa/main/pack.c | 2111 +++-------------- > src/mesa/main/pack_tmp.h | 76 +- > src/mesa/main/run_mako.py | 7 + > src/mesa/main/teximage.c | 4 +- > src/mesa/main/texstore.c | 2 +- > src/mesa/swrast/s_drawpix.c | 3 - > src/mesa/swrast/s_texfetch.c | 13 + > src/mesa/swrast/s_texfetch_tmp.h | 1359 +---------- > 23 files changed, 3222 insertions(+), 10583 deletions(-) > create mode 100644 src/mesa/main/format_convert.py > delete mode 100644 src/mesa/main/format_pack.c > create mode 100644 src/mesa/main/format_pack.c.mako > delete mode 100644 src/mesa/main/format_unpack.c > create mode 100644 src/mesa/main/format_unpack.c.mako > create mode 100644 src/mesa/main/run_mako.py > > -- > 1.9.1 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev