On 05/11/2012 01:37 PM, Paul Berry wrote: > On 11 May 2012 12:40, Chad Versace <chad.vers...@linux.intel.com > <mailto:chad.vers...@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.vers...@linux.intel.com _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
