On 14 December 2011 15:00, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">On Wed, Dec 14, 2011 at 11:25 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > On 14 December 2011 13:42, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: >> >> I think RASTERIZER_DISCARD has nothing to do with transform feedback. >> I think it's part of the same spec because it's not useful without it. >> As I understand it, _NEW_TRANSFORM_FEEDBACK is dirty when transform >> feedback buffer bindings are changed or just enabled/disabled. On the >> other hand, RASTERIZER_DISCARD enables or disables the rasterizer, so >> it should fall into the same category as face culling for example. (I >> even implemented it using face culling on r600) >> >> Also there would be no way to know whether _NEW_TRANSFORM_FEEDBACK >> changes just TFB buffer bindings, or just RASTERIZER_DISCARD, or both. >> >> Marek > > > I see where you are coming from--I could have implemented rasterizer discard > on i965 gen6 in the same way. > > However, I think there are three compelling reasons to consider rasterizer > discard to be related to transform feedback: > > (1) from a user perspective, it really only makes sense to use rasterizer > discard when transform feedback is active. Thus, it's highly likely that > when the rasterizer discard state is changed, transform feedback settings > will be changed too. > > (2) rasterizer discard functionality is described by the same set of > extensions that enable transform feedback (e.g. EXT_transform_feedback), so > presumably the inventors of these two features thought they were closely > related. > > (3) the enable bit that Mesa uses to keep track of the state of rasterizer > discard is in gl_context::TransformFeedback, not gl_context::Transform. > > Item (3) is the most important to me. One of the scarier things about i965 > driver development is that we have to manually keep track of which hardware > configuration commands correspond to which dirty bits. If we miss a > dependency, that causes a subtle bug in which a state change might not cause > the hardware state to be updated properly. These kinds of bugs are not well > exposed by Piglit tests, because most Piglit tests make a large number of > state changes followed by a draw operation, so a missing dependency might > easily go undetected. The thing that allows me to sleep at night is that > there is a nice one-to-one correspondence between almost all of the dirty > bits and corresponding substructures of gl_context. For example, the > _NEW_MODELVIEW dirty bit corresponds to gl_context::ModelView, > _NEW_PROJECTION corresponds to gl_context::Projection, and so on. That > means any time I am worried that I'm not handling dirty bits correctly, I > can visually inspect the code and make sure that the dirty bits I'm > consulting match up with which elements of gl_context I'm accessing. If we > leave the code as is, then there's a big undocumented exception to that > one-to-one correspondence, wherein > gl_context::TransformFeedback.RasterDiscard is covered by _NEW_TRANSFORM, > not _NEW_TRANSFORM_FEEDBACK, as one would logically guess based on where > it's located within gl_context. I'm not confident that I (or other > developers) will remember this exception once we're no longer in the thick > of implementing transform feedback and rasterizer discard. > > It seems to me that we have three possible approaches to choose from here: > > (a) Go ahead with this patch, and modify r600 code to recompute the face > culling mode when the _NEW_TRANSFORM_FEEDBACK dirty bit is set. > > (b) Don't apply this patch, and instead move RasterDiscard from > gl_context::TransformFeedback to gl_context::Transform, so that we restore > the one-to-one correspondence between dirty bits and substructures of > gl_context. > > (c) Do nothing, and rely on programmers to remember that RasterDiscard is an > exception to the usual correspondence between dirty bits and substructures > of gl_context. > > I'm really not comfortable with (c) because of the risk of future bugs. I > suppose I could be talked into (b) if there's popular support for it, but > it's not my favourite, because as I said earlier, I think there are actually > a lot of good reasons to think of rasterizer discard as related to transform > feedback. My preference is to do (a). </div></div>(d) Rework the _NEW_* flags such that they roughly match hardware state groups, not OpenGL state groups. Direct3D 11 and Gallium are two examples of how it could be done. I am for (b) or (d). I would have nothing against (a) if TFB buffer bindings were not covered by the same flag. It's mainly about the overhead of state changes, although I admitted there are r600-related reasons too. Also, Gallium will have rasterizer_discard in the rasterizer state (once the patches hit master) - that can be changed though. Marek </blockquote></div> I would be happy to review patches to do (d) if someone wants to take that on. Sadly, I do not have time to work on it myself right now, since I am under deadline pressure to finish OpenGL 3.0 support. As for your concerns about the overhead of state changes caused by putting TFB buffer bindings under the same flag as rasterizer discard, would those concerns be addressed by removing the FLUSH_VERTICES(ctx, _NEW_TRANSFORM_FEEDBACK) call from bind_buffer_range()? As you pointed out in another email, I was wrong to add that in the first place, since the user isn't allowed to change transform feedback buffer bindings while transform feedback is active. I'm planning to send out a patch to fix that ASAP.