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

[Paul Berry]

On 14 December 2011 15:00, Marek Olšák <maraeo at gmail.com> wrote:

> 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
>

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