[Mesa-dev] [PATCH] i965: Allow primitive restart on Haswell for additional primitive types.

[Paul Berry]

On 11 September 2012 15:34, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 09/11/2012 02:53 PM, Paul Berry wrote:
> > On 7 September 2012 13:04, Kenneth Graunke <kenneth at whitecape.org
> > <mailto:kenneth at whitecape.org>> wrote:
> >
> >     These are supported in hardware; no need for software fallbacks.
> >
> >     Not actually hit in any of our test cases, since they appear to count
> >     primitives generated which currently causes software fallbacks
> anyway.
> >
> >
> > How much effort would it be to update the test cases so that they
> > exercise this code?  It seems like it would be worth spending some
> > effort to make sure the hardware path gets tested, just in case there
> > are undiscovered hardware bugs :)
>
> I was using oglconform tests...I'd have to check the status of the
> piglit tests.  It'd definitely be best to test it, I agree.
>
> >     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
> >     <mailto:kenneth at whitecape.org>>
> >     ---
> >      src/mesa/drivers/dri/i965/brw_primitive_restart.c | 20
> >     ++++++++++----------
> >      1 file changed, 10 insertions(+), 10 deletions(-)
> >
> >     diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
> >     b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
> >     index 38b5243..fc36897 100644
> >     --- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
> >     +++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
> >     @@ -79,6 +79,7 @@ can_cut_index_handle_prims(struct gl_context *ctx,
> >                                 const struct _mesa_index_buffer *ib)
> >      {
> >         struct brw_context *brw = brw_context(ctx);
> >     +   struct intel_context *intel = intel_context(ctx);
> >
> >         if (brw->sol.counting_primitives_generated ||
> >             brw->sol.counting_primitives_written) {
> >     @@ -105,19 +106,18 @@ can_cut_index_handle_prims(struct gl_context
> *ctx,
> >            case GL_TRIANGLES:
> >            case GL_TRIANGLE_STRIP:
> >               /* Cut index supports these primitive types */
> >     -         break;
> >     -      default:
> >     -         /* Cut index does not support these primitive types */
> >     -      //case GL_LINE_LOOP:
> >     -      //case GL_TRIANGLE_FAN:
> >     -      //case GL_QUADS:
> >     -      //case GL_QUAD_STRIP:
> >     -      //case GL_POLYGON:
> >     -         return false;
> >     +         return true;
> >     +      case GL_LINE_LOOP:
> >     +      case GL_TRIANGLE_FAN:
> >     +      case GL_QUADS:
> >     +      case GL_QUAD_STRIP:
> >     +      case GL_POLYGON:
> >     +         /* Haswell supports cut index on additional primitive
> types */
> >     +         return intel->is_haswell;
> >            }
> >         }
> >
> >     -   return true;
> >     +   return false;
> >
> >
> > I like the fact that you're returning false for unrecognized primitive
> > types, since we know that in the future more primitive types will be
> > added (GL_LINES_ADJACENCY, GL_LINE_STRIP_ADJACENCY,
> > GL_TRIANGLES_ADJACENCY, GL_TRIANGLE_STRIP_ADJACENCY, and GL_PATCHES),
> > and it's probably safer not to enable hardware primitive restart on
> > those types until we can actually test them.
>
> Hmm.  I'd forgotten about those.  I suppose we could add them here now,
> since all hardware appears to support cut index for those primitive
> types.  (Well, GL_PATCHES is IVB+, but considering that tessellation
> support in general is IVB+, we can consider it always supported.)
>

Oh yeah, you're right.  Yes, this seems like a good idea.  In fact, now
that I look back at the table, it looks like Haswell supports primitive
restart on all OpenGL primitive types, and IVB and SNB support it on
everything but trifan, quadlist, quadstrip, polygon, and line loop.
Presumably future chip revisions will support a superset of what Haswell
supports, so if you want to simplify the logic and be more future proof,
you might consider changing the logic to:

if (intel->gen > 7 || intel->is_haswell) {
   /* HW supports primitive restart for all OpenGL primitive types */
   return true;
}

for (...) {
   switch (...) {
   case GL_LINE_LOOP:
   case GL_TRIANGLE_FAN:
   case GL_QUADS:
   case GL_QUAD_STRIP:
   case GL_POLYGON:
      /* HW doesn't support primitive restart for this primitive type */
      return false;
   }
}

return true;


>
> I'm not sure there's much downside to that.  Yeah, we haven't tested it,
> but we'll have to test things as part of implementing those primitive
> types in the future...
>
> Then again, the code does nothing at the moment...
>
> Kind of torn.
>
> > But I'm worried that we'll forget to revisit this code when we do get
> > around to adding them.  How would you feel about putting the future
> > primitive types into the switch statement, with a comment explaining why
> > we're not handling them, just so they'll show up in grep results when we
> > are working on those new primitive types?  For example maybe something
> > like this:
> >
> > case GL_LINES_ADJACENCY:
> >    ...
> > case GL_PATCHES:
> >    /* Not yet implemented, so for safety fall back to software primitive
> > restart */
> >    break;
> >
> > In any case,
> >
> > Reviewed-by: Paul Berry <stereotype441 at gmail.com
> > <mailto:stereotype441 at gmail.com>>
>
> I think I'm as likely to forget to update the code, even if I added them
> and made them greppable.  I guess my preference would be to either
> 1. just go ahead and say "yes we support them", or
> 2. leave them out
>

Yeah, good point.  I would be ok with #1 (I took that into account in my
suggestion above).


>
> On a related note, I'm actually concerned that this code is just plain
> broken.  Jordan, why is it looping from nr_prims to 0?  The contents of
> the loop in no way seem to depend on the counter, and the original
> code's "break" statement would only jump out of the switch, not the loop.
>
> It looks like it's trying to handle the case of multiple primitives,
> each with a different type, and wants to say no if any of those aren't
> supported.  I didn't think mixing primitive types was possible, though.
>  Is the loop just superfluous?
>

I haven't looked deeply enough into the VBO module to verify that mixing
primitive types is impossible, but I have no reason to doubt you :).  All
the same, I'm not sure I'm comfortable making this code depend on
that--after all, it's conceivable that someone will come along later and
add optimizations to the VBO module that combine together draw commands of
different primitive types.  As long as the data structure the VBO module
sends to us makes it *possible* to specify a different primitive type for
each part of the draw operation, I'd prefer to code defensively and assume
that it might happen.

Besides, it should be easy to fix the code to check the types of all the
constituent primitives.  I believe we would just need to change the for
loop and switch statement to:

for (i = 0; i < num_prims; ++i) {
   switch (prim[i].mode) {
      ...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120912/cb5f753c/attachment.html>