Re: [Mesa-dev] [PATCH] i965: Don't bail on vertex element processing if we need draw params.

[Jason Ekstrand]

Bah... Forgot this:

Reviewed-by: Jason Ekstrand 

On Dec 20, 2016 13:12, "Jason Ekstrand"  wrote:

> On Dec 19, 2016 13:27, "Kenneth Graunke"  wrote:
>
> BaseVertex, BaseInstance, DrawID, and some edge flag conditions need
> vertex buffer and elements structs.  We can't bail early in this case.
>
> Gen4-7 already do this properly.  Gen8+ did not.
>
> Thanks to Ilia Mirkin for helping track this down.
>
> Cc: mesa-sta...@lists.freedesktop.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99144
> Reported-by
> :
> Pierre-Eric Pelloux-Prayer 
> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/gen8_draw_upload.c | 34
> ++--
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> index 69ba8e9..3177f9a 100644
> --- a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> +++ b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> @@ -110,6 +110,22 @@ gen8_emit_vertices(struct brw_context *brw)
>ADVANCE_BATCH();
> }
>
> +   /* Normally we don't need an element for the SGVS attribute because the
> +* 3DSTATE_VF_SGVS instruction lets you store the generated attribute
> in an
> +* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
> +* we're using draw parameters then we need an element for the those
> +* values.  Additionally if there is an edge flag element then the SGVS
> +* can't be inserted past that so we need a dummy element to ensure
> that
> +* the edge flag is the last one.
> +*/
> +   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
> +vs_prog_data->uses_baseinstance ||
> +((vs_prog_data->uses_instanceid ||
> +  vs_prog_data->uses_vertexid) &&
> + uses_edge_flag));
>
>
> Out of curiosity, why are we trying so hard to avoid an extra element?
>
> +   const unsigned nr_elements =
> +  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
> +
> /* If the VS doesn't read any inputs (calculating vertex position from
>  * a state variable for some reason, for example), emit a single pad
>  * VERTEX_ELEMENT struct and bail.
> @@ -117,7 +133,7 @@ gen8_emit_vertices(struct brw_context *brw)
>  * The stale VB state stays in place, but they don't do anything unless
>  * a VE loads from them.
>  */
> -   if (brw->vb.nr_enabled == 0) {
> +   if (nr_elements == 0) {
>
>
> Seems reasonable.
>
>BEGIN_BATCH(3);
>OUT_BATCH((_3DSTATE_VERTEX_ELEMENTS << 16) | (3 - 2));
>OUT_BATCH((0 << GEN6_VE0_INDEX_SHIFT) |
> @@ -172,22 +188,6 @@ gen8_emit_vertices(struct brw_context *brw)
>ADVANCE_BATCH();
> }
>
> -   /* Normally we don't need an element for the SGVS attribute because the
> -* 3DSTATE_VF_SGVS instruction lets you store the generated attribute
> in an
> -* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
> -* we're using draw parameters then we need an element for the those
> -* values.  Additionally if there is an edge flag element then the SGVS
> -* can't be inserted past that so we need a dummy element to ensure
> that
> -* the edge flag is the last one.
> -*/
> -   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
> -vs_prog_data->uses_baseinstance ||
> -((vs_prog_data->uses_instanceid ||
> -  vs_prog_data->uses_vertexid) &&
> - uses_edge_flag));
> -   const unsigned nr_elements =
> -  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
> -
> /* The hardware allows one more VERTEX_ELEMENTS than VERTEX_BUFFERS,
>  * presumably for VertexID/InstanceID.
>  */
> --
> 2.10.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Don't bail on vertex element processing if we need draw params.

[Jason Ekstrand]

On Dec 19, 2016 13:27, "Kenneth Graunke"  wrote:

BaseVertex, BaseInstance, DrawID, and some edge flag conditions need
vertex buffer and elements structs.  We can't bail early in this case.

Gen4-7 already do this properly.  Gen8+ did not.

Thanks to Ilia Mirkin for helping track this down.

Cc: mesa-sta...@lists.freedesktop.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99144
Reported-by: Pierre-Eric Pelloux-Prayer 
Signed-off-by: Kenneth Graunke 
---
 src/mesa/drivers/dri/i965/gen8_draw_upload.c | 34
++--
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
index 69ba8e9..3177f9a 100644
--- a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
+++ b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
@@ -110,6 +110,22 @@ gen8_emit_vertices(struct brw_context *brw)
   ADVANCE_BATCH();
}

+   /* Normally we don't need an element for the SGVS attribute because the
+* 3DSTATE_VF_SGVS instruction lets you store the generated attribute
in an
+* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
+* we're using draw parameters then we need an element for the those
+* values.  Additionally if there is an edge flag element then the SGVS
+* can't be inserted past that so we need a dummy element to ensure that
+* the edge flag is the last one.
+*/
+   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
+vs_prog_data->uses_baseinstance ||
+((vs_prog_data->uses_instanceid ||
+  vs_prog_data->uses_vertexid) &&
+ uses_edge_flag));


Out of curiosity, why are we trying so hard to avoid an extra element?

+   const unsigned nr_elements =
+  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
+
/* If the VS doesn't read any inputs (calculating vertex position from
 * a state variable for some reason, for example), emit a single pad
 * VERTEX_ELEMENT struct and bail.
@@ -117,7 +133,7 @@ gen8_emit_vertices(struct brw_context *brw)
 * The stale VB state stays in place, but they don't do anything unless
 * a VE loads from them.
 */
-   if (brw->vb.nr_enabled == 0) {
+   if (nr_elements == 0) {


Seems reasonable.

   BEGIN_BATCH(3);
   OUT_BATCH((_3DSTATE_VERTEX_ELEMENTS << 16) | (3 - 2));
   OUT_BATCH((0 << GEN6_VE0_INDEX_SHIFT) |
@@ -172,22 +188,6 @@ gen8_emit_vertices(struct brw_context *brw)
   ADVANCE_BATCH();
}

-   /* Normally we don't need an element for the SGVS attribute because the
-* 3DSTATE_VF_SGVS instruction lets you store the generated attribute
in an
-* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
-* we're using draw parameters then we need an element for the those
-* values.  Additionally if there is an edge flag element then the SGVS
-* can't be inserted past that so we need a dummy element to ensure that
-* the edge flag is the last one.
-*/
-   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
-vs_prog_data->uses_baseinstance ||
-((vs_prog_data->uses_instanceid ||
-  vs_prog_data->uses_vertexid) &&
- uses_edge_flag));
-   const unsigned nr_elements =
-  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
-
/* The hardware allows one more VERTEX_ELEMENTS than VERTEX_BUFFERS,
 * presumably for VertexID/InstanceID.
 */
--
2.10.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Don't bail on vertex element processing if we need draw params.

[Kenneth Graunke]

BaseVertex, BaseInstance, DrawID, and some edge flag conditions need
vertex buffer and elements structs.  We can't bail early in this case.

Gen4-7 already do this properly.  Gen8+ did not.

Thanks to Ilia Mirkin for helping track this down.

Cc: mesa-sta...@lists.freedesktop.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99144
Reported-by: Pierre-Eric Pelloux-Prayer 
Signed-off-by: Kenneth Graunke 
---
 src/mesa/drivers/dri/i965/gen8_draw_upload.c | 34 ++--
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/gen8_draw_upload.c 
b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
index 69ba8e9..3177f9a 100644
--- a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
+++ b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
@@ -110,6 +110,22 @@ gen8_emit_vertices(struct brw_context *brw)
   ADVANCE_BATCH();
}
 
+   /* Normally we don't need an element for the SGVS attribute because the
+* 3DSTATE_VF_SGVS instruction lets you store the generated attribute in an
+* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
+* we're using draw parameters then we need an element for the those
+* values.  Additionally if there is an edge flag element then the SGVS
+* can't be inserted past that so we need a dummy element to ensure that
+* the edge flag is the last one.
+*/
+   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
+vs_prog_data->uses_baseinstance ||
+((vs_prog_data->uses_instanceid ||
+  vs_prog_data->uses_vertexid) &&
+ uses_edge_flag));
+   const unsigned nr_elements =
+  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
+
/* If the VS doesn't read any inputs (calculating vertex position from
 * a state variable for some reason, for example), emit a single pad
 * VERTEX_ELEMENT struct and bail.
@@ -117,7 +133,7 @@ gen8_emit_vertices(struct brw_context *brw)
 * The stale VB state stays in place, but they don't do anything unless
 * a VE loads from them.
 */
-   if (brw->vb.nr_enabled == 0) {
+   if (nr_elements == 0) {
   BEGIN_BATCH(3);
   OUT_BATCH((_3DSTATE_VERTEX_ELEMENTS << 16) | (3 - 2));
   OUT_BATCH((0 << GEN6_VE0_INDEX_SHIFT) |
@@ -172,22 +188,6 @@ gen8_emit_vertices(struct brw_context *brw)
   ADVANCE_BATCH();
}
 
-   /* Normally we don't need an element for the SGVS attribute because the
-* 3DSTATE_VF_SGVS instruction lets you store the generated attribute in an
-* element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if
-* we're using draw parameters then we need an element for the those
-* values.  Additionally if there is an edge flag element then the SGVS
-* can't be inserted past that so we need a dummy element to ensure that
-* the edge flag is the last one.
-*/
-   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
-vs_prog_data->uses_baseinstance ||
-((vs_prog_data->uses_instanceid ||
-  vs_prog_data->uses_vertexid) &&
- uses_edge_flag));
-   const unsigned nr_elements =
-  brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data->uses_drawid;
-
/* The hardware allows one more VERTEX_ELEMENTS than VERTEX_BUFFERS,
 * presumably for VertexID/InstanceID.
 */
-- 
2.10.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev