On Wed, Dec 14, 2011 at 11:25 PM, Paul Berry <stereotype...@gmail.com> wrote: > On 14 December 2011 13:42, Marek Olšák <mar...@gmail.com> 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).
(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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev