[Mesa-dev] [PATCH v2 5/5] i965/gen6: Initial implementation of MSAA.

[Chad Versace]

On 05/11/2012 01:37 PM, Paul Berry wrote:
> On 11 May 2012 12:40, Chad Versace <chad.versace at linux.intel.com <mailto:chad.versace at linux.intel.com>> wrote:
> 
>     On 05/10/2012 11:21 AM, Paul Berry wrote:
>     > This patch enables MSAA for Gen6, by modifying intel_mipmap_tree to
>     > understand multisampled buffers, adapting the rendering pipeline setup
>     > to enable multisampled rendering, and adding multisample resolve
>     > operations to brw_blorp_blit.cpp. Some preparation work is also
>     > included for Gen7, but it is not yet enabled.
>     >
>     > MSAA support is still fairly preliminary.  In particular, the
>     > following are not yet supported:
>     > - Fully general blits between MSAA and non-MSAA buffers.
>     > - Formats other than RGBA8, DEPTH24, and STENCIL8.
>     > - Centroid interpolation.
>     > - Coverage parameters (glSampleCoverage, GL_SAMPLE_ALPHA_TO_COVERAGE,
>     >   GL_SAMPLE_ALPHA_TO_ONE, GL_SAMPLE_COVERAGE, GL_SAMPLE_COVERAGE_VALUE,
>     >   GL_SAMPLE_COVERAGE_INVERT).
>     >
>     > Fixes piglit tests "EXT_framebuffer_multisample/accuracy" on
>     > i965/Gen6.
>     >
>     > v2:
>     > - In intel_alloc_renderbuffer_storage(), quantize the requested number
>     >   of samples to the next higher sample count supported by the
>     >   hardware.  This ensures that a query of GL_SAMPLES will return the
>     >   correct value.  It also ensures that MSAA is fully disabled on Gen7
>     >   for now (since Gen7 MSAA support doesn't work yet).
>     > - When reading from a non-MSAA surface, ensure that s_is_zero is true
>     >   so that we won't try to read from a nonexistent sample.
>     > ---
>     >  src/mesa/drivers/dri/i965/Makefile.sources         |    1 +
>     >  src/mesa/drivers/dri/i965/brw_blorp.cpp            |   10 +-
>     >  src/mesa/drivers/dri/i965/brw_blorp.h              |   30 ++-
>     >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp       |  332 +++++++++++++++++---
>     >  src/mesa/drivers/dri/i965/brw_context.h            |    8 +
>     >  src/mesa/drivers/dri/i965/brw_defines.h            |    7 +
>     >  src/mesa/drivers/dri/i965/brw_misc_state.c         |   33 +--
>     >  src/mesa/drivers/dri/i965/brw_state.h              |    4 +
>     >  src/mesa/drivers/dri/i965/brw_state_upload.c       |    2 +
>     >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c   |   16 +-
>     >  src/mesa/drivers/dri/i965/gen6_blorp.cpp           |   44 ++--
>     >  src/mesa/drivers/dri/i965/gen6_multisample_state.c |  102 ++++++
>     >  src/mesa/drivers/dri/i965/gen6_sf_state.c          |   15 +-
>     >  src/mesa/drivers/dri/i965/gen6_wm_state.c          |   12 +
>     >  src/mesa/drivers/dri/i965/gen7_blorp.cpp           |   20 +-
>     >  src/mesa/drivers/dri/i965/gen7_sf_state.c          |   14 +-
>     >  src/mesa/drivers/dri/i965/gen7_wm_state.c          |   18 +-
>     >  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c  |   16 +
>     >  src/mesa/drivers/dri/intel/intel_fbo.c             |   31 ++-
>     >  src/mesa/drivers/dri/intel/intel_mipmap_tree.c     |   52 +++-
>     >  src/mesa/drivers/dri/intel/intel_mipmap_tree.h     |   10 +-
>     >  src/mesa/drivers/dri/intel/intel_tex_image.c       |    3 +-
>     >  src/mesa/drivers/dri/intel/intel_tex_validate.c    |    3 +-
>     >  23 files changed, 662 insertions(+), 121 deletions(-)
>     >  create mode 100644 src/mesa/drivers/dri/i965/gen6_multisample_state.c
> 
>     I tried reviewing this one, but the patch does so much at once that
>     I quickly got lost. And, anyway, I don't understand brw shaders.
> 
>     Do you intend to push this today regardless of receiving review?
>     I see that you have gen7 patches queued up dependent on this series.
> 
> 
> Unfortunately, I was unable to find a way to break this patch and the previous one into easily digestible incremental chunks.  I developed them incrementally, but you wouldn't want to see my development increments--they were so full of missteps, bugs, reorganizations, and ground-up rewrites that reviewing them would be a complete nightmare.  When I cleaned up my development branch, I had to squash together a lot of patches to avoid introducing transient bugs and to keep related code together.  Squashing those things together resulted in the two mega-patches you see in this series.  I'm not proud of how big the patches are--it was the best I could do considering how much of the rendering pipeline MSAA touches, and how much work we have to go to in order to do MSAA blits.
> 
> To compound the problem, the "blorp" code largely deals with creating a custom WM program using GEN assembly.  Despite our best efforts, this sort of code is among the most difficult parts of the i965 driver to follow--witness the complexity of the Gen4/5 clip, sf, and gs programs, which most of us have had the good fortune not to be able to avoid during our time with Mesa (I had to understand the clip program because I thought there was a bug in it back when I was implementing clip distance, but it turned out to be a wild goose chase).  At the moment I think Eric and I (and possibly Ken) are the only regular Mesa contributors who really understand a lot of the subtleties of GEN assembly programming.
> 
> The next series is, fortunately, much more incremental than this series turned out to be, since it's just adapting the Gen6 blorp/msaa work for Gen7 rather than building it from scratch.  Unfortunately, anyone who wants to review the work in the next series is going to have to understand what's going on in this series, since the one builds on the other.
> 
> I'm open to suggestions about how to proceed.  Here are some possibilities:
> 
> a) Since the series adds a significant chunk of functionality, doesn't regress any piglit tests, and is a necessary prerequisite for the next series, go ahead and push it without review.  Disadvantage: sidestepping the review process like this is a bad trend to set, and it makes us lose out on the opportunity to find bugs.  Also, since these patches introduce a new component to the driver (the "blorp" component) there's a danger that if nobody reviews them, the new component will turn into a "code ghetto" that nobody feels comfortable working in except for me.  Chad, I remember how glad you were when I started looking at HiZ code so that you wouldn't be the only one who understood it anymore.  I don't want to create a parallel situation with MSAA.
> 
> b) Same as a), but in addition I spend some one-on-one time walking through the code with anyone who is interested, so that I'm not the only person who understands how it works.
> 
> c) Don't push the code yet, and instead have a code review meeting on Monday.  Hopefully the code will be a lot easier to follow in a meeting format, where I can answer questions on the fly.  Advantage: we get the benefits of code review, and those present in the meeting will hopefully be able to review the next patch series.  Plus, hopefully people will leave the meeting with a better understanding of GEN assembly programming than they went in with.  Disadvantage: no transparency to those outside of Intel.  If we decide to go with this approach, I really want to insist that we have the meeting on Monday, so that we don't delay these patches any longer than we have to.
> 
> d) I spend a day or two breaking down the last two patches in this series into a series of smaller increments that people can review.  Advantage: the review happens on the mailing list.  Disadvantage: splitting the code up into smaller increments is really not going to make it any more comprehensible than it is already; it will just make for a lot of small emails rather than a few big emails.  Also there's a risk of introducing temporary breakages (which will interfere with later bisection attempts), not to mention the fact that this will add further delay to getting this feature implemented.
> 
> I'm sensitive to the benefits of good code review and I would like to have people's "reviewed-by"s if possible.  But I'm also sensitive to the fact that we're extremely under the gun for this feature (MSAA was supposed to be a required feature of GL 3.0, and we released GL 3.0 support without it).  So I don't want to introduce any more delays than necessary.
> 
> My preference at the moment is for alternative b).  I would also be ok with alternative c) but I know that a lot of y'all really don't enjoy meetings :)

My short email was largely an attempt to evoke suggestion (b). I think that's the best approach. Hopefully I'm not the only one who signs up for the braindump.

I think (c) is the ideal situation, but everyone is so crunched for time that it may be difficult to get consensus for such a meeting.

I don't want you to waste your time with (d).

Monday morning, I think we should poke Ken and Eric to see if they want a MSAA braindump and try to do it sameday.

----
Chad Versace
chad.versace at linux.intel.com