Bug 89210

bwidawsk suggests Iago Toral Quiroga be the first to investigate this bug, since he wrote the GS for SNB.

I'm not sure if it's relevant, but in case it saves anyone some time:

Sandybridge's GS runs in SIMD4x1 mode, whereas everything else's GS runs in SIMD4x2. If we're calculating number of primitives generated based on number of shader invocations, that may account for the difference.

Is that the same thing as the DUAL_OBJECT dispatch stuff the GS supports?

I suspect it's just some simple errata. I am slightly worried the hardware might be using a different meaning of primitives, ie. triangle strips (which should be 2) as opposed to triangles, which should be 4 per triangle strip.

I just figured Iago is a good choice since he would be familiar with this GEN's hardware.

Sure, I'll have a look.

Ok, so the problem is that, at least in gen6, we need to compute the number of primitives emitted by the GS state manually adding assembly code to the generated GS program (see gen6_gs_visitor.cpp, prim_count field).

Right now we are counting full-primitives only, that is, we increment the primitive count when EndPrimitive() is called (explicitly or implicitly), so for things such as triangle strips we count just one primitive, not the individual triangles within the strip.

If this behavior is not what we expect then I think there are two ways to fix this:

1) Simply use CL_PRIMITIVE_COUNT to resolve this query (this is what we have been using all this time to resolve GL_PRIMITIVES_GENERATED).

2) Re-write the assembly code we generate for the GS stage so we handle things different for strip primitives: for N vertices we generate a primitive count N-2 for triangle strips and fans, N-1 (for line strips) and we leave the current implementation for other primitives.

If 1) is enough to fix all the needs we have regarding this I'd favor that approach, since that would be an easy fix. I'll attach a patch for that shortly too.

Created attachment 113656 [details] [review]
Patch (option 1)

This fixes the piglit test that triggered the bug report.

(In reply to Iago Toral from comment #6)
> Created attachment 113656 [details] [review] [review]
> Patch (option 1)
> 
> This fixes the piglit test that triggered the bug report.

Ben: can you confirm that this fixes all the problems? If that is the case (and if we are okay with option 1), I can send the patch for review to the mailing list if needed.

I confirmed that this patch fixes the problems.  It creates no piglit regressions on other platforms.

Hmm. Is this going to work for points and linestrips? Care to give it a shot?

diff --git a/tests/spec/arb_pipeline_statistics_query/pipeline_stats_geom.c b/tests/spec/arb_pipeline_statistics_query/pipeline_stats_geom.c
index 30d4bf0..bac1eec 100644
--- a/tests/spec/arb_pipeline_statistics_query/pipeline_stats_geom.c
+++ b/tests/spec/arb_pipeline_statistics_query/pipeline_stats_geom.c
@@ -70,7 +70,7 @@ const char *vs_src =
 const char *gs_src =
        "#version 150                                   \n"
        "layout(triangles) in;                          \n"
-       "layout(triangle_strip, max_vertices = 6) out;  \n"
+       "layout(points, max_vertices = 6) out;  \n"
        "in vec4 vertex_to_gs[3];                       \n"
        "void main()                                    \n"
        "{                                              \n"
@@ -104,8 +104,8 @@ static struct query queries[] = {
        {
         .query = GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB,
         .name = "GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB",
-        .min = NUM_PRIMS * 4,
-        .max = NUM_PRIMS * 4}
+        .min = NUM_PRIMS * 6,
+        .max = NUM_PRIMS * 6}
 };

Ben, your test of points passes as well.  I'd like to improve the test so that it tests more than just one geometry mode.  

I think it's safe to send the Iago's patch to the mailing list and push it.

(In reply to Mark Janes from comment #10)
> Ben, your test of points passes as well.  I'd like to improve the test so
> that it tests more than just one geometry mode.  

FWIW, I tested with line strips and it works fine too.

> I think it's safe to send the Iago's patch to the mailing list and push it.

Ok, I'll send it now.
Thanks.

Fixed in master with:

commit 2a06728ba0da5e4175843b1b53919d6167ca0aea
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Fri Feb 20 08:21:25 2015 +0100

    i965/gen6: Fix GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB

    In gen6 we need to compute the primitive count in the generated GS program.
    The current implementation only counts full primitives, that is, if the
    output primitive type is a triangle strip, it won't count individual
    triangles in the strip, only complete strips.
    
    If we want to count basic primitives instead we have two options: rework
    the assembly code we generate for strip primitives or simply use
    CL_INVOCATION_COUNT to resolve the query and let the hardware do that work
    for us. This patch implements the latter approach.
    
    Fixes the following piglit test:
    bin/arb_pipeline_statistics_query-geom -auto
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89210
    Tested-by: Mark Janes <mark.a.janes@intel.com>
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>