[Mesa-dev] [PATCH 00/20] Auto-generate pack/unpack functions

Tue Nov 18 02:36:32 PST 2014

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

First, I think this is a great goal.  And while I haven't reviewed them 
in detail, just from skimming through them, these patch series seem to 
be a great cleanup and a lot of work went into it.  I certainly don't 
object to any of this.

But I have to say I find a bit unfortunate that so much effort is being 
put on implementing something specific to Mesa formats instead of taking 
this opportunity to devise a solution that would work both for gallium 
and Mesa formats.

Furthermore I see there is some interest speeding mesa using SSE2 
intrinsics, and of course format conversion is precisely one of the code 
paths that can great benefit from SIMD, but I have no doubt: the most 
efficient and sane way of leveraging SIMD with all these formats 
conversion is to JIT compile format conversion code tailored to the CPU 
in runtime.  There are just too many CPU variations to statically 
generate C code for every one of them.  And lot of the code to do this 
already exists in src/gallium/auxiliary/gallivm.  We should consider 
using it from src/mesa/*.

This gallium helper code was written outside mesa to avoid the GL 
dependency (there was one point some of this stuff had to run in XP 
kernel mode! thankfully not anymore), and because when gallium was 
written the idea was that all Mesa drivers would eventually migrate into 
it.  Alas, that didn't happen, and might never happen.  That's OK.  But 
in that case, I do believe we should find a way of sharing more of the 
stuff in src/gallium/auxiliary with all Mesa drivers, non-gallium 
classic drivers included.

In particular, we really should have a format conversion module that is 
capable of handling a superset of all Mesa and Gallium formats somwhere 
in src/util/  .  One might even leverage the auto-generated pack/unpack 
functions in src/gallium/auxiliary/u_format* though I wouldn't care if 
not, as long as the functionality is the same.

In short, we can't change the past, but I wish we could have more 
synergy in the future.

Jose

On 18/11/14 08:43, Iago Toral Quiroga wrote:
> This is the fist of two series of patches to address:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D84566&d=AAIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=9H2UxyGUngIpuFNlQEzL9VtIOmYVKVs2o4nyL_We6o0&s=xYhgcKEBxp3-d_ddjpFiNGRkqLo2BMUedLQ67ck2ce8&e=
>
> 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
>