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

[Paul Berry]

On 7 September 2012 13:04, Kenneth Graunke <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 :)


>
> Signed-off-by: Kenneth Graunke <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.

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>


>  }
>
>  /**
> --
> 1.7.11.4
>

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120911/0791716d/attachment.html>