[Mesa-dev] [PATCH 1/2] mesa: Set the correct ctx->NewState bitfield for rasterizer discard.

[Marek Olšák]

On Wed, Dec 14, 2011 at 11:25 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 14 December 2011 13:42, Marek Olšák <maraeo at 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