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

[Kenneth Graunke]

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

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

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?