[Mesa-dev] [PATCH 10/10] i965 gen6: Implement pass-through GS for transform feedback.

[Paul Berry]

On 6 December 2011 23:54, Kenneth Graunke <kenneth at whitecape.org> wrote:

> > +   if (intel->gen == 6) {
> > +      /* On Gen6, GS is used for transform feedback. */
> > +      key->need_gs_prog = ctx->TransformFeedback.CurrentObject->Active;
> > +   } else if (intel->gen >= 7) {
> > +      /* On Gen7 and later, we don't use GS (yet). */
> > +      key->need_gs_prog = false;
>
> Could you please put these in order?  6, 7+, 4-5 is just asking for OCD
> issues. :)  I'd probably move the >= 7 check to the top.
>

Oh, whoops.  Normally I am sufficiently OCD to notice stuff like this.  I
must be cured!

I assume when you say "I'd probably move the >= 7 check to the top" you
mean that the order you'd prefer is 7+, then 6, then 4-5.  That works for
me--it's nice to have the most recent stuff first.


> > diff --git a/src/mesa/drivers/dri/i965/gen6_gs_state.c
> b/src/mesa/drivers/dri/i965/gen6_gs_state.c
> > index d29f029..b041140 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_gs_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_gs_state.c
> > @@ -44,22 +44,36 @@ upload_gs_state(struct brw_context *brw)
> >     OUT_BATCH(0);
> >     ADVANCE_BATCH();
> >
> > -   // GS should never be used on Gen6.  Disable it.
> > -   assert(!brw->gs.prog_active);
> > -   BEGIN_BATCH(7);
> > -   OUT_BATCH(_3DSTATE_GS << 16 | (7 - 2));
> > -   OUT_BATCH(0); /* prog_bo */
> > -   OUT_BATCH((0 << GEN6_GS_SAMPLER_COUNT_SHIFT) |
> > -          (0 << GEN6_GS_BINDING_TABLE_ENTRY_COUNT_SHIFT));
> > -   OUT_BATCH(0); /* scratch space base offset */
> > -   OUT_BATCH((1 << GEN6_GS_DISPATCH_START_GRF_SHIFT) |
> > -          (0 << GEN6_GS_URB_READ_LENGTH_SHIFT) |
> > -          (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT));
> > -   OUT_BATCH((0 << GEN6_GS_MAX_THREADS_SHIFT) |
> > -          GEN6_GS_STATISTICS_ENABLE |
> > -          GEN6_GS_RENDERING_ENABLE);
> > -   OUT_BATCH(0);
> > -   ADVANCE_BATCH();
> > +   if (brw->gs.prog_active) {
> > +      BEGIN_BATCH(7);
> > +      OUT_BATCH(_3DSTATE_GS << 16 | (7 - 2));
> > +      OUT_BATCH(brw->gs.prog_offset);
> > +      OUT_BATCH(GEN6_GS_SPF_MODE | GEN6_GS_VECTOR_MASK_ENABLE);
> > +      OUT_BATCH(0); /* no scratch space */
> > +      OUT_BATCH((1 << GEN6_GS_DISPATCH_START_GRF_SHIFT) |
> > +             (brw->gs.prog_data->urb_read_length <<
> GEN6_GS_URB_READ_LENGTH_SHIFT));
> > +      OUT_BATCH(((brw->max_gs_threads - 1) <<
> GEN6_GS_MAX_THREADS_SHIFT) |
> > +             GEN6_GS_STATISTICS_ENABLE |
> > +             GEN6_GS_SO_STATISTICS_ENABLE |
> > +             0); //GEN6_GS_RENDERING_ENABLE);
>
> I'm rather surprised this works.  I thought you needed the
> GEN6_GS_RENDERING_ENABLE bit set in order to draw anything at all.
>
> The commented out code looks like it came from a half-baked patch of
> mine, so I'm guessing it's unintentional.  Still, do you have any idea
> why it would work?
>

Whoops.  You're right of course.  My best guess for why it worked is that
when the GS is enabled, this bit doesn't have much effect--it just gets
passed on to the GS program, which is supposed to use it to decide whether
to pass vertices down the pipeline.  Since I haven't written the part of
the GS program that disables rendering yet, the GS program was just
ignoring the incorrect bit and sending the vertices anyway.

Still, it's asking for trouble to set this bit incorrectly--the PRM clearly
states "This bit must be set if the thread will attempt to allocate a
handle. If clear, the GS thread must not allocate handles (e.g., when only
performing stream output without concurrent rendering)."  It's probably
sheer luck that I didn't crash the GPU.  I'll fix it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111207/8f913835/attachment.htm>