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

[Paul Berry]

On 19 December 2011 19:07, Marek Olšák <maraeo at gmail.com> wrote:

> On Thu, Dec 15, 2011 at 7:21 PM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 15 December 2011 08:02, Eric Anholt <eric at anholt.net> wrote:
> >>
> >> On Thu, 15 Dec 2011 00:00:49 +0100, Marek Olšák <maraeo at gmail.com>
> wrote:
> >> > On Wed, Dec 14, 2011 at 11:25 PM, Paul Berry <stereotype441 at gmail.com
> >
> >> > wrote:
> >> > > (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.
> >>
> >> The problem is that everyone disagrees on what "hardware state group" a
> >> piece of state is in.  On i965, rasterizer discard is really in the
> >> transform feedback state -- the SOL (transform feedback) unit on gen7,
> >> and the GS on gen6.
> >
> >
> > I have been thinking about this more this morning, and I have an idea for
> > how to accomplish (d) that I think would address this problem.  It's not
> a
> > trivial change, but it's something we could implement incrementally, so
> we
> > apply it to rasterizer discard now, and over time extend it to cover
> other
> > pieces of state.  Here's the idea:
> >
> > The key problem is that there are so many distinct pieces of state that
> we
> > could never possibly assign a separate bit to each one--we would run out
> of
> > space in the bitfield.  So instead of having core Mesa decide how they
> are
> > grouped (and, inevitably, wind up grouping them in a way that doesn't
> work
> > well for some drivers), let each driver decide how they are grouped.  The
> > drivers communicate this grouping to core Mesa by populating a new data
> > structure (at initialization time) called ctx->StateFlags.
> ctx->StateFlags
> > has an entry for each distinct piece of state, which tells which bits in
> > ctx->NewState should be set when that state changes.
> >
> > So, for example, in BeginTransformFeedback() and EndTransformFeedback(),
> > instead of doing this:
> >
> > FLUSH_VERTICES(ctx, _NEW_TRANSFORM_FEEDBACK);
> >
> > We would do this:
> >
> > FLUSH_VERTICES(ctx, ctx->StateFlags->TransformFeedback_Active);
> >
> > In PauseTransformFeedback() and ResumeTransformFeedback() we would do:
> >
> > FLUSH_VERTICES(ctx, ctx->StateFlags->TransformFeedback_Paused);
> >
> > And in enable.c, when rasterizer discard is turned on or off, we would
> do:
> >
> > FLUSH_VERTICES(ctx, ctx->StateFlags->RasterizerDiscard);
> >
> > In the i965 driver, where all of these features map to the GS stage of
> the
> > pipeline, we would initialize TransformFeedback_Active,
> > TransformFeedback_Paused, and RasterizerDiscard all to the same value.
> In
> > the r600 driver, where rasterizer discard is implemented using face
> culling,
> > StateFlags->RasterizerDiscard would indicate a totally different bit than
> > those used for transform feedback.
> >
> > In the short term, we could implement this technique just for rasterizer
> > discard, to address the differences between r600 and i965 that we're
> > discussing in this email thread.  In the long term, our goal would be to
> > replace all of the _NEW_* constants with a fine-grained set of values in
> > StateFlags.  Once we've done that, each driver can set up StateFlags in a
> > way that precisely matches how state is grouped for that particular
> piece of
> > hardware.
> >
> > What do y'all think?  If there's support for this idea I'd be glad to
> make
> > an RFC patch.
>
> I like this idea very much. However, there is a catch that some _NEW
> flags are used for core Mesa, others are used for the vbo module,
> others are used for generating fixed function programs, others just
> notify about a change in constant buffers (e.g. the gl_* variables in
> GLSL) and so on, and finally, most are consumed by drivers to generate
> states. I am sure we can do this incrementally though and doing the
> easy stuff first.
>
> Marek
>

Oh wow, I knew that some of the _NEW flags were used in core mesa, but I
had no idea how many of them were affected until I did some grepping this
morning.  It looks like the only _NEW flags that *aren't* used by core
mesa, vbo, or some such are _NEW_POLYGONSTIPPLE and
_NEW_TRANSFORM_FEEDBACK.  And since ctx->NewState is only 32 bits wide and
has 31 bits assigned, this leaves us with almost zero wiggle room for
driver-specific state flags.

I just spent a bunch of time talking with Ian and Kenneth about this, and
we're gravitating back toward the idea similar to what we originally called
(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).  However, there's a problem with that: the stuff in
gl_context::Transform is stuff that can be saved and restored using
PushAttrib and PopAttrib.  So we can't put rasterizer discard in there
either.  In fact, most of the substructures in gl_context correspond to
something that can be pushed and popped, so there is really no good home
for rasterizer discard.

So here's our new proposal:

1. Move RasterDiscard to the toplevel of gl_context

2. Since we are running out of bits in ctx->NewState, don't create a new
bit for it; use _NEW_TRANSFORM (as we are currently doing).  However, to
clarify what's going on, create a new

#define _NEW_RASTER_DISCARD _NEW_TRANSFORM

so that we can say _NEW_RASTER_DISCARD in all of the rasterizer discard
places in the code.

This should address Marek's concerns about performance (since rasterizer
discard won't be covered by the same dirty bit as transform feedback
settings).  And it should address my concerns about keeping track of which
pieces of context correspond to which dirty bits, because it will be
intuitive that _NEW_RASTER_DISCARD corresponds to ctx->RasterDiscard.

I'll submit a patch this afternoon.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111220/135c229b/attachment.htm>