Bug 89210 - GS statistics fail on SNB
Summary: GS statistics fail on SNB
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Iago Toral
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-18 18:18 UTC by Mark Janes
Modified: 2015-02-27 08:08 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch (option 1) (1.57 KB, patch)
2015-02-19 10:26 UTC, Iago Toral
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Janes 2015-02-18 18:18:33 UTC
In mesa e206785b5790c97743b7d3929563c21ad87aa765, ARB_pipeline_statistics_query was enabled.

Corresponding piglit tests pass everywhere EXCEPT the GS on SNB.

spec.ARB_pipeline_statistics_query.arb_pipeline_statistics_query-geom

GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB value was invalid.
  Expected: 8 - 8
  Observed: 2
Comment 1 Mark Janes 2015-02-18 18:20:18 UTC
bwidawsk suggests Iago Toral Quiroga be the first to investigate this bug, since he wrote the GS for SNB.
Comment 2 Matt Turner 2015-02-18 18:24:09 UTC
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.
Comment 3 Ben Widawsky 2015-02-19 04:00:47 UTC
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.
Comment 4 Iago Toral 2015-02-19 07:04:41 UTC
Sure, I'll have a look.
Comment 5 Iago Toral 2015-02-19 10:25:25 UTC
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.
Comment 6 Iago Toral 2015-02-19 10:26:37 UTC
Created attachment 113656 [details] [review]
Patch (option 1)

This fixes the piglit test that triggered the bug report.
Comment 7 Iago Toral 2015-02-19 12:14:06 UTC
(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.
Comment 8 Mark Janes 2015-02-19 21:11:42 UTC
I confirmed that this patch fixes the problems.  It creates no piglit regressions on other platforms.
Comment 9 Ben Widawsky 2015-02-19 22:29:56 UTC
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}
 };
Comment 10 Mark Janes 2015-02-20 00:46:30 UTC
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.
Comment 11 Iago Toral 2015-02-20 07:09:09 UTC
(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.
Comment 12 Iago Toral 2015-02-20 11:12:30 UTC
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>


bug/show.html.tmpl processed on Feb 25, 2017 at 20:34:59.
(provided by the Example extension).