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

[Paul Berry]

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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111214/18216685/attachment.html>