[Mesa-dev] [PATCH V3 12/13] vbo: map indirect buffer and extract params if doing sw primitive restart

[Chris Forbes]

There's a test for the SW primitive restart interaction in piglit.

On Wed, Nov 13, 2013 at 9:45 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 11/09/2013 01:02 AM, Chris Forbes wrote:
>> V2: Check for mapping failure (thanks Brian)
>> V3: - Change error on mapping failure to OUT_OF_MEMORY (Brian)
>>     - Unconst; remove casting away of const.
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> ---
>>  src/mesa/vbo/vbo_primitive_restart.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/src/mesa/vbo/vbo_primitive_restart.c b/src/mesa/vbo/vbo_primitive_restart.c
>> index 75e760f..e0bd233 100644
>> --- a/src/mesa/vbo/vbo_primitive_restart.c
>> +++ b/src/mesa/vbo/vbo_primitive_restart.c
>> @@ -180,6 +180,39 @@ vbo_sw_primitive_restart(struct gl_context *ctx,
>>     GLboolean map_ib = ib->obj->Name && !ib->obj->Pointer;
>>     void *ptr;
>>
>> +   /* If there is an indirect buffer, map it and extract the draw params */
>> +   if (indirect && prims[0].is_indirect) {
>> +      struct _mesa_prim new_prim = *prims;
>> +      struct _mesa_index_buffer new_ib = *ib;
>> +      const uint32_t *indirect_params;
>> +      if (!ctx->Driver.MapBufferRange(ctx, 0, indirect->Size, GL_MAP_READ_BIT,
>> +            indirect)) {
>> +
>> +         /* something went wrong with mapping, give up */
>> +         _mesa_error(ctx, GL_OUT_OF_MEMORY,
>> +                     "failed to map indirect buffer for sw primitive restart");
>> +         return;
>
> I had to think about this a bit:
>
> If the current batch writes data into the indirect BO, and the
> application immediately tries to DrawIndirect from it without flushing...
>
> We should actually be okay, since intel_bufferobj_map_range will flush
> the batch for us:
>
>    if (!(access & GL_MAP_UNSYNCHRONIZED_BIT)) {
>       if (drm_intel_bo_references(brw->batch.bo, intel_obj->buffer)) {
>          if (access & GL_MAP_INVALIDATE_BUFFER_BIT) {
>             ...
>          } else {
>             perf_debug("Stalling on the GPU for mapping a busy buffer "
>                        "object\n");
>             intel_batchbuffer_flush(brw);
>          }
>       } else {
>          ...
>       }
>    }
>
> Nice.
>
> Have you at least done touch testing of SW primitive restart + draw
> indirect?  SW primitive restart is pretty broken in a lot of ways
> anyway, so as long as you tried /something/ and it worked I won't object.
>
> Patches 12 and 13 (with the relnotes fix) are also:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
>> +      }
>> +
>> +      assert(nr_prims == 1);
>> +      indirect_params = (const uint32_t *) ADD_POINTERS(indirect->Pointer,
>> +            new_prim.indirect_offset);
>> +
>> +      new_prim.is_indirect = 0;
>> +      new_prim.count = indirect_params[0];
>> +      new_prim.num_instances = indirect_params[1];
>> +      new_prim.start = indirect_params[2];
>> +      new_prim.basevertex = indirect_params[3];
>> +      new_prim.base_instance = indirect_params[4];
>> +
>> +      new_ib.count = new_prim.count;
>> +
>> +      prims = &new_prim;
>> +      ib = &new_ib;
>> +
>> +      ctx->Driver.UnmapBuffer(ctx, indirect);
>> +   }
>> +
>>     /* Find the sub-primitives. These are regions in the index buffer which
>>      * are split based on the primitive restart index value.
>>      */
>>