[Mesa-dev] [PATCH 7/7] i965: Reduce vertex state reemission

[Kristian Høgsberg Kristensen]

Ian Romanick <idr at freedesktop.org> writes:

> On 12/15/2015 12:28 AM, Kristian Høgsberg Kristensen wrote:
>> We can inspect VS prog_data for iterations i > 0, and only flag
>> BRW_NEW_VERTICES when one of our system values change.
>> 
>> This change also flags BRW_NEW_VERTICES in one case we were missing
>> before: if we're doing an indirect draw, prims[i].basevertex is always 0
>> and the real base vertex value is in the indirect parameter
>> buffer. Thus, if a program uses base vertex or base instance, and the
>> draw call is indirect, flag BRW_NEW_VERTICES.  A new piglit test,
>> spec/ARB_shader_draw_parameters/drawid-indirect-vertexid tests this.
>> ---
>>  src/mesa/drivers/dri/i965/brw_draw.c | 44 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 40 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
>> index b0710c67..9e400ca 100644
>> --- a/src/mesa/drivers/dri/i965/brw_draw.c
>> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
>> @@ -491,9 +491,44 @@ brw_try_draw_prims(struct gl_context *ctx,
>>           }
>>        }
>>  
>> -      brw->draw.params.gl_basevertex =
>> +      /* Determine if we need to flag BRW_NEW_VERTICES for updating the
>> +       * gl_BaseVertexARB, gl_BaseInstanceARB or gl_DrawIDARB values. As
>> +       * above, we don't need to check first iteration, since the flag is set
>> +       * before the loop. We also can't rely on vs prog_data in the first
>> +       * iteration, but after drawing once, we've uploaded the programs and
>> +       * can look at prog_data.
>> +       *
>> +       * Despite the prims[] name, eache iteration correspond to a draw call
>                                       each            corresponds
>
>> +       * from a glMulti* style draw call. We need to re-upload vertex state if
>> +       *
>> +       *  1) the program uses gl_DrawIDARB (changes every iteration),
>> +       *
>> +       *  2) the program uses gl_BaseVertexARB or gl_BaseInstanceARB and the
>> +       *     draw call is indirect (meaning we can't check if the value change
>> +       *     or not), or
>> +       *
>> +       *  3) the program uses gl_BaseVertexARB or gl_BaseInstanceARB and the
>> +       *  value changed
>> +       */
>> +      const int new_basevertex =
>>           prims[i].indexed ? prims[i].basevertex : prims[i].start;
>> -      brw->draw.params.gl_baseinstance = prims[i].base_instance;
>> +      const int new_baseinstance = prims[i].base_instance;
>> +      if (i > 0) {
>> +         const bool uses_draw_parameters =
>> +            brw->vs.prog_data->uses_basevertex ||
>> +            brw->vs.prog_data->uses_baseinstance;
>> +
>> +         if (brw->vs.prog_data->uses_drawid ||
>> +             (uses_draw_parameters && prims[i].is_indirect) ||
>> +             (brw->vs.prog_data->uses_basevertex &&
>> +              brw->draw.params.gl_basevertex != new_basevertex) ||
>> +             (brw->vs.prog_data->uses_baseinstance &&
>> +              brw->draw.params.gl_baseinstance != new_baseinstance))
>> +            brw->ctx.NewDriverState |= BRW_NEW_VERTICES;
>> +      }
>> +
>> +      brw->draw.params.gl_basevertex = new_basevertex;
>> +      brw->draw.params.gl_baseinstance = new_baseinstance;
>>        drm_intel_bo_unreference(brw->draw.draw_params_bo);
>>  
>>        if (prims[i].is_indirect) {
>> @@ -512,10 +547,11 @@ brw_try_draw_prims(struct gl_context *ctx,
>>        }
>>  
>>        /* gl_DrawID always needs its own vertex buffer since it's not part of
>> -       * the indirect parameter buffer. */
>> +       * the indirect parameter buffer.
>> +       */
>
> Lol
>
>>        brw->draw.gl_drawid = prims[i].draw_id;
>>        drm_intel_bo_unreference(brw->draw.draw_id_bo);
>> -      brw->ctx.NewDriverState |= BRW_NEW_VERTICES;
>> +      brw->draw.draw_id_bo = NULL;
>
> It seems odd that this change is in this patch.  Should it have always
> been after the drm_intel_bo_unreference call?

Yea... looks like I squashed these two changes into the wrong commit.

Kristian

>>  
>>        if (brw->gen < 6)
>>  	 brw_set_prim(brw, &prims[i]);
>>