[Mesa-dev] [PATCH 2/2 v3] i965: Use predicate enable bit for conditional rendering w/o stalling

[Neil Roberts]

Kenneth Graunke <kenneth at whitecape.org> writes:

> It might be nice to create a brw_load_register_mem64 function, for
> symmetry with brw_store_register_mem64 - we might want to reuse it
> elsewhere someday.

Ok, that sounds sensible.

> One interesting quirk: the two halves of your register write may land
> in two separate batchbuffers, since it's done with two BEGIN_BATCH /
> ADVANCE_BATCH blocks (each of which only reserves space for one LRM).

Ah right, yes, good point.

> */ goes on its own line, here and elsewhere.

Oops, yes.

>> +   } else {
>> +      if (brw->ctx.Query.CondRenderQuery) {
>> +         perf_debug("Conditional rendering is implemented in software and may "
>> +                    "stall.\n");
>> +      }
>> +
>> +      return _mesa_check_conditional_render(&brw->ctx);
>
> I'd put this in the same block as the perf_debug and just do 'return
> true' here - we can save the function call and redundant query check in
> the common case (and this is a really hot path).

Ok, sounds good.

>> @@ -333,6 +335,9 @@ intelInitExtensions(struct gl_context *ctx)
>>           ctx->Extensions.ARB_transform_feedback2 = true;
>>           ctx->Extensions.ARB_transform_feedback3 = true;
>>           ctx->Extensions.ARB_transform_feedback_instanced = true;
>> +
>> +         if (brw->intelScreen->cmd_parser_version >= 2)
>> +            brw->predicate.supported = true;
>
> So, this is insufficient for Haswell.  There was not a version bump when
> it actually started working (I think Daniel assumed we didn't need it,
> since we were already attempting to write registers ourselves.)
>
> I think the best plan of action is to submit a kernel patch bumping the
> command parser version number to 4, then change this to:
>
>    const int cmd_parser_version = brw->intelScreen->cmd_parser_version;
>
>    if (cmd_parser_version >= (brw->is_haswell ? 4 : 2))
>       brw->predicate.supported = true;

I don't think this is necessary. It's a bit hard to tell from the patch
diff but this hunk is inside an if statement like this:

   if (brw->gen >= 7) {
      /* ... */
      if (can_do_pipelined_register_writes(brw)) {
         /* ... transform feedback stuff ... */
         if (brw->intelScreen->cmd_parser_version >= 2)
            brw->predicate.supported = true;
      }
      /* ... */
   }

Therefore it will only try to use the predicate register if the command
parser is at least version 2 *and* we can do register writes. I don't
think there is any version of the kernel where this wouldn't correctly
detect whether it can write to the predicate register.

If you agree with this then I'll post a v4 with all the other changes.
In the meantime there is a WIP branch of it here:

https://github.com/bpeel/mesa/tree/wip/conditional-render

Many thanks for the detailed review.

Regards,
- Neil