[Mesa-dev] [PATCH] i965: Implement bounds checking for transform feedback output.

Ian Romanick idr at freedesktop.org
Fri Dec 16 11:09:34 PST 2011


On 12/16/2011 10:44 AM, Paul Berry wrote:
> On 16 December 2011 10:04, Paul Berry <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>> wrote:
>
>     On 15 December 2011 15:20, Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>> wrote:
>
>         Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>         <mailto:kenneth at whitecape.org>>
>         ---
>           src/mesa/drivers/dri/i965/brw_context.c |    1 +
>           src/mesa/drivers/dri/i965/brw_context.h |    3 ++
>           src/mesa/drivers/dri/i965/brw_gs_emit.c |   10 ++++++++
>           src/mesa/drivers/dri/i965/gen6_sol.c    |   38
>         +++++++++++++++++++++++++++++++
>           4 files changed, 52 insertions(+), 0 deletions(-)
>
>         diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>         b/src/mesa/drivers/dri/i965/brw_context.c
>         index 7d360b0..fd60853 100644
>         --- a/src/mesa/drivers/dri/i965/brw_context.c
>         +++ b/src/mesa/drivers/dri/i965/brw_context.c
>         @@ -117,6 +117,7 @@ static void brwInitDriverFunctions( struct
>         dd_function_table *functions )
>             brw_init_queryobj_functions(functions);
>
>             functions->PrepareExecBegin = brwPrepareExecBegin;
>         +   functions->BeginTransformFeedback =
>         brw_begin_transform_feedback;
>             functions->EndTransformFeedback = brw_end_transform_feedback;
>           }
>
>         diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>         b/src/mesa/drivers/dri/i965/brw_context.h
>         index 8e52488..20623d4 100644
>         --- a/src/mesa/drivers/dri/i965/brw_context.h
>         +++ b/src/mesa/drivers/dri/i965/brw_context.h
>         @@ -1073,6 +1073,9 @@ brw_fprog_uses_noperspective(const struct
>         gl_fragment_program *fprog);
>
>           /* gen6_sol.c */
>           void
>         +brw_begin_transform_feedback(struct gl_context *ctx,
>         +                            struct gl_transform_feedback_object
>         *obj);
>         +void
>           brw_end_transform_feedback(struct gl_context *ctx,
>                                     struct gl_transform_feedback_object
>         *obj);
>
>         diff --git a/src/mesa/drivers/dri/i965/brw_gs_emit.c
>         b/src/mesa/drivers/dri/i965/brw_gs_emit.c
>         index 72d4eca..5dd3734 100644
>         --- a/src/mesa/drivers/dri/i965/brw_gs_emit.c
>         +++ b/src/mesa/drivers/dri/i965/brw_gs_emit.c
>         @@ -352,6 +352,15 @@ gen6_sol_program(struct brw_gs_compile *c,
>         struct brw_gs_prog_key *key,
>                 */
>                brw_MOV(p, get_element_ud(c->reg.header, 5),
>                        get_element_ud(c->reg.SVBI, 0));
>         +
>         +      /* Make sure that the buffers have enough room for all
>         the vertices. */
>         +      brw_ADD(p, get_element_ud(c->reg.temp, 0),
>         +                get_element_ud(c->reg.SVBI, 0),
>         brw_imm_ud(num_verts));
>         +      brw_CMP(p, vec1(brw_null_reg()), BRW_CONDITIONAL_L,
>         +                get_element_ud(c->reg.temp, 0),
>         +                get_element_ud(c->reg.SVBI, 4));
>         +      brw_IF(p, BRW_EXECUTE_1);
>         +
>
>
> Whoops, one other correction.  There's an off-by-one error--this should
> be BRW_CONDITIONAL_LE.
>
> For example, let's say we're outputting triangles, the transform
> feedback buffer is large enough to hold 6 vertices, and we've output 3
> vertices already.  In that case num_verts is 3, SVBI.0 is 3, and SVBI.4
> is 6.  The above logic compares SVBI.0 + num_verts < SVBI.4 (6 < 6), so
> it concludes that there isn't room for the second triangle.  It should
> be computing SVBI.0 + num_verts <= SVBI.4 (6 <= 6), because there is
> just barely room for the second triangle.

Do we have piglit test cases for these edge conditions?  If we don't, we 
need them. :)

>                /* For each vertex, generate code to output each varying
>         using the
>                 * appropriate binding table entry.
>                 */
>         @@ -392,6 +401,7 @@ gen6_sol_program(struct brw_gs_compile *c,
>         struct brw_gs_prog_key *key,
>                              get_element_ud(c->reg.header, 5),
>         brw_imm_ud(1));
>                   }
>                }
>         +      brw_ENDIF(p);
>
>                /* Now, reinitialize the header register from R0 to
>         restore the parts of
>                 * the register that we overwrote while streaming out
>         transform feedback
>         diff --git a/src/mesa/drivers/dri/i965/gen6_sol.c
>         b/src/mesa/drivers/dri/i965/gen6_sol.c
>         index b11bce2..56d4a6a 100644
>         --- a/src/mesa/drivers/dri/i965/gen6_sol.c
>         +++ b/src/mesa/drivers/dri/i965/gen6_sol.c
>         @@ -26,6 +26,7 @@
>           * Code to initialize the binding table entries used by
>         transform feedback.
>           */
>
>         +#include "main/macros.h"
>           #include "brw_context.h"
>           #include "intel_buffer_objects.h"
>           #include "intel_batchbuffer.h"
>         @@ -101,6 +102,43 @@ const struct brw_tracked_state
>         gen6_sol_surface = {
>           };
>
>           void
>         +brw_begin_transform_feedback(struct gl_context *ctx,
>         +                            struct gl_transform_feedback_object
>         *obj)
>         +{
>         +   struct intel_context *intel = intel_context(ctx);
>         +   const struct gl_shader_program *vs_prog =
>         +      ctx->Shader.CurrentVertexProgram;
>         +   const struct gl_transform_feedback_info *linked_xfb_info =
>         + &vs_prog->LinkedTransformFeedback;
>         +   struct gl_transform_feedback_object *xfb_obj =
>         +      ctx->TransformFeedback.CurrentObject;
>         +
>         +   unsigned max_index = 0xffffffff;
>         +
>         +   /* Compute the maximum number of vertices that we can write
>         without
>         +    * overflowing any of the buffers currently being used for
>         feedback.
>         +    */
>         +   for (int i = 0; i < MAX_FEEDBACK_ATTRIBS; ++i) {
>
>
>     Minor nit: MAX_FEEDBACK_ATTRIBS (32) is the correct bound for
>     generic Mesa code, but in i965, we only support 4 buffers, so this
>     loop bound should probably be BRW_MAX_SOL_BUFFERS.
>
>         +      unsigned stride = linked_xfb_info->BufferStride[i];
>         +
>         +      /* Skip any inactive buffers, which have a stride of 0. */
>         +      if (stride == 0)
>         +        continue;
>         +
>         +      unsigned max_for_this_buffer = xfb_obj->Size[i] / (4 *
>         stride);
>         +      max_index = MIN2(max_index, max_for_this_buffer);
>         +   }
>         +
>         +   /* Initialize the SVBI 0 register to zero and set the
>         maximum index. */
>         +   BEGIN_BATCH(4);
>         +   OUT_BATCH(_3DSTATE_GS_SVB_INDEX << 16 | (4 - 2));
>         +   OUT_BATCH(0); /* SVBI 0 */
>         +   OUT_BATCH(0);
>         +   OUT_BATCH(max_index);
>         +   ADVANCE_BATCH();
>         +}
>         +
>         +void
>           brw_end_transform_feedback(struct gl_context *ctx,
>                                     struct gl_transform_feedback_object
>         *obj)
>           {
>         --
>         1.7.7.3
>
>         _______________________________________________
>         mesa-dev mailing list
>         mesa-dev at lists.freedesktop.org
>         <mailto:mesa-dev at lists.freedesktop.org>
>         http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>     Other than that one minor comment, this patch is:
>
>     Reviewed-by: Paul Berry <stereotype441 at gmail.com
>     <mailto:stereotype441 at gmail.com>>


More information about the mesa-dev mailing list