<div dir="ltr">Jose,<br>I haven't had time to fully review Iago and Samuel's code, so I can't 100% comment on it right now. However, let me make a few comments on the "overarching plan" as it were.<br><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 18, 2014 at 2:36 AM, Jose Fonseca <span dir="ltr"><<a href="mailto:jfonseca@vmware.com" target="_blank">jfonseca@vmware.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>> The idea is that we have a lot of format conversion code scattered through<br>
> different files in the repository, a lot of that is redundant / duplicated,<br>
> so this intends to address that issue.<br>
<br></span>
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.<br>
<br>
<br>
<br>
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.<br></blockquote><div><br></div><div>That is the end goal. Unfortunately, getting there requires a lot of work. Probably more work on the mesa side than on the gallium side. A big part of the problem was that there was a lot of code for format conversion and it was scattered all over mesa/main directory. A lot of stuff needs to be cleaned up an unified inside mesa/main before things can be unified with gallium. Much of that work is now done.<br><br></div><div>One of the things that I would like to see happen after this stuff lands is to convert the mesa pack/unpack functions to take a width, height, and stride. Then they would have exactly the same function signature as the gallium conversion functions and merging will be much easier. Then we can work on moving the format handling code into a helper library which, for the moment, I'll call libmesaformat. Then both gallium and mesa classic can pull from libmesaformat and we can kill all of the redundant code. Whose autogenerator framework we end up keeping is kind of immaterial, they're not that hard to write.<br><br>One of the decisions that has to be made there (probably a topic for another thread) is how we would want to structure the format metadata. Mesa and gallium both have completely different ways of structuring it and we need to unify that if we're going to unify the conversion code. Personally, I think gallium's is cleaner and more expressive, but it lacks the GL information that core mesa needs. But, like I said, that's a topic for another thread.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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/*.<br></blockquote><div><br></div><div>Yes, there were some patches. However, given my experiments in the past, I'm skeptical as to how much of a real benefit it would be to SSE-accelerate all the format conversion. When I did my first major rework a couple of months ago, I experimented with using the SSSE3 shuffle operation for doing swizzling of the C implementation. The net result of that experiment is that using SSSE3 had a very marginal benefit over a good C implementation such as the one we now have. The real problem we had was that the current format conversion stuff was doing the pessimal thing in a lot of cases; a lot of that stupid is now gone. So, if someone wants to work on that, I'm curious to see their results, but I'm not holding out for it.<br><br></div><div>As far as doing a JIT compile, I've thought of it. Daniel Stone recently mentioned the idea of using ORC for doing things like this and it might be a good fit. However, be fore we can do that, we need a framework for these things (which we now have, thanks to this series). How is that done in gallivm? Is it done using LLVM? If so, it might be a non-starter. I don't want to rekindle any debates, but you're going to have trouble convincing some people that format conversion is a good enough reason for a hard dependency on LLVM.<br></div><div><br></div><div>Another idea that has been put forward would be to, whenever possible, push the unconverted data to the GPU as a linear texture and use the GPU to do the format conversion. This would quite possibly be better than either a straight CPU path or an optimized JIT'ed path.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
<br>
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.<br></blockquote><div><br></div><div>Absolutely! That's 100% the end goal here. One of the additions of this series is a new pseudo-enum type called MESA_ARRAY_FORMAT. Between that and the MESA_FORMAT enum you can, with a single 32-bit integer, represent all of the mesa formats, gallium formats, and the entire combinatorial explosion of GL formats. The "master conversion function" mentioned several times in this series is capable of converting between any of these formats. Also, this is a mesa enum, not a GL enum. The only knowledge the conversion functions have of GL is a single function that converts a GL format/type pair into a MESA_FORMAT or MESA_ARRAY_FORMAT. From there on, it's entirely GL agnostic.<br><br></div><div>I hope this clears up some of your reservations.<br></div><div>--Jason<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In short, we can't change the past, but I wish we could have more synergy in the future.<br>
<br>
<br>
Jose<span><br>
<br>
<br>
<br>
<br>
On 18/11/14 08:43, Iago Toral Quiroga wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
This is the fist of two series of patches to address:<br>
</span><a href="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=" target="_blank">https://urldefense.proofpoint.<u></u>com/v2/url?u=https-3A__bugs.<u></u>freedesktop.org_show-5Fbug.<u></u>cgi-3Fid-3D84566&d=AAIGaQ&c=<u></u>Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-<u></u>YihVMNtXt-uEs&r=<u></u>zfmBZnnVGHeYde45pMKNnVyzeaZbdI<u></u>qVLprmZCM2zzE&m=<u></u>9H2UxyGUngIpuFNlQEzL9VtIOmYVKV<u></u>s2o4nyL_We6o0&s=xYhgcKEBxp3-d_<u></u>ddjpFiNGRkqLo2BMUedLQ67ck2ce8&<u></u>e=</a><div><div><br>
<br>
The idea is that we have a lot of format conversion code scattered through<br>
different files in the repository, a lot of that is redundant / duplicated,<br>
so this intends to address that issue.<br>
<br>
The goal of this first series is to address auto-generation of our pack/unpack<br>
functions (format_pack.c and format_unpack.c). Currently, we have a ton of<br>
hand-coded pack/unpack functions for lots of formats, but we can auto-generate<br>
most of that code instead, so this series handles this.<br>
<br>
This is based on initial work by Jason Ekstrand.<br>
<br>
Tested on i965, classic swrast and gallium (radeon, nouveau, llvmpipe) without<br>
regressions.<br>
<br>
For software drivers we worked with a trimmed set of piglit tests (related to<br>
format conversion), ~5700 tests selected with the following filter:<br>
<br>
-t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix<br>
-t fbo -t frame<br>
<br>
Summary of the patches:<br>
* Patches 1-7 are general fixes to the current code that were found while<br>
working on this.<br>
* Patches 8-16 implement auto-generation of pack/unpack functions.<br>
* Patches 17-20 make use of the auto-generated pack/unpack functions in<br>
various places to simplify the current code.<br>
<br>
Notice that some of the fixes in patches 1-7 will become obsolete as soon as<br>
we auto-generate the pack/unpack functions, but we thought it would make sense<br>
to keep them in the patch set anyway since we started from that base and they<br>
should be correct fixes to the currently existing code.<br>
<br>
Iago Toral Quiroga (1):<br>
swrast: Remove unused variable.<br>
<br>
Jason Ekstrand (9):<br>
mesa/format_utils: Fix a bug in one of the format helper functions<br>
mesa: Fix packing/unpacking of MESA_FORMAT_R5G6B5_UNORM<br>
mesa/colormac: Remove an unused macro<br>
mesa: Fix A1R5G5B5 packing/unpacking<br>
mesa/format_utils: Prefix and expose the conversion helper functions<br>
mesa: Add a concept of an array format<br>
mesa: Add a _mesa_is_format_color_format helper<br>
mesa: Autogenerate most of format_pack.c<br>
mesa: Autogenerate format_unpack.c<br>
<br>
Samuel Iglesias Gonsalvez (10):<br>
mesa: Fix get_texbuffer_format().<br>
mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp<br>
properly<br>
mesa: Add _mesa_pack_uint_rgba_row() format conversion function<br>
mesa: Add non-normalized formats support for ubyte packing functions<br>
mesa/format_pack: Add _mesa_pack_int_rgba_row()<br>
mesa/formats: add new mesa formats and their pack/unpack functions.<br>
mesa: use format conversion functions in swrast<br>
mesa/pack: use autogenerated format_pack functions<br>
mesa/main/pack_tmp.h: Add float conversion support<br>
mesa/pack: refactor _mesa_pack_rgba_span_float()<br>
<br>
src/mesa/Makefile.am | 18 +<br>
src/mesa/Makefile.sources | 4 +-<br>
src/mesa/main/colormac.h | 3 -<br>
src/mesa/main/format_convert.<u></u>py | 71 +<br>
src/mesa/main/format_info.py | 41 +<br>
src/mesa/main/format_pack.c | 2994 ------------------------<br>
src/mesa/main/format_pack.c.<u></u>mako | 1147 ++++++++++<br>
src/mesa/main/format_pack.h | 6 +<br>
src/mesa/main/format_unpack.c | 4400 ------------------------------<u></u>------<br>
src/mesa/main/format_unpack.c.<u></u>mako | 914 ++++++++<br>
src/mesa/main/format_utils.c | 248 +-<br>
src/mesa/main/format_utils.h | 105 +<br>
src/mesa/main/formats.c | 193 +-<br>
src/mesa/main/formats.csv | 13 +<br>
src/mesa/main/formats.h | 73 +<br>
src/mesa/main/pack.c | 2111 +++--------------<br>
src/mesa/main/pack_tmp.h | 76 +-<br>
src/mesa/main/run_mako.py | 7 +<br>
src/mesa/main/teximage.c | 4 +-<br>
src/mesa/main/texstore.c | 2 +-<br>
src/mesa/swrast/s_drawpix.c | 3 -<br>
src/mesa/swrast/s_texfetch.c | 13 +<br>
src/mesa/swrast/s_texfetch_<u></u>tmp.h | 1359 +----------<br>
23 files changed, 3222 insertions(+), 10583 deletions(-)<br>
create mode 100644 src/mesa/main/format_convert.<u></u>py<br>
delete mode 100644 src/mesa/main/format_pack.c<br>
create mode 100644 src/mesa/main/format_pack.c.<u></u>mako<br>
delete mode 100644 src/mesa/main/format_unpack.c<br>
create mode 100644 src/mesa/main/format_unpack.c.<u></u>mako<br>
create mode 100644 src/mesa/main/run_mako.py<br>
<br>
</div></div></blockquote><div><div>
<br>
______________________________<u></u>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/<u></u>mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div></div></div>