On 12/14/2011 04:10 PM, Marek Olšák wrote: > On Thu, Dec 15, 2011 at 12:18 AM, Paul Berry <stereotype...@gmail.com> wrote: >> On 14 December 2011 15:00, Marek Olšák <mar...@gmail.com> wrote: >>> >>> 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 >> >> >> 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 > > Fixing bind_buffer_range wouldn't make the assumed overhead of > changing RASTERIZER_DISCARD just go away. I'd like RASTERIZER_DISCARD > to be kept out of the _NEW_TRANSFORM_FEEDBACK flag as long as Gallium > has it in the rasterizer state. Even _NEW_RASTERIZER_DISCARD would do > the job. i965 could create transform feedback state from > _NEW_TRANSFORM_FEEDBACK|_NEW_RASTERIZER_DISCARD, while Gallium could > use _NEW_TRANSFORM_FEEDBACK for buffer bindings only and > _NEW_RASTERIZER_DISCARD for the rasterizer state. > > It's the same issue as with _NEW_TEXTURE, which mixes a lot of > mutually unrelated states. > > Marek
I think _NEW_RASTERIZER_DISCARD is probably the best option for now. It's simple to do, and being fine grained, is easy to subscribe to in the right places without really any overhead. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
