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

[Kenneth Graunke]

On Monday, May 11, 2015 03:14:21 PM Neil Roberts wrote:
> 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

Ahh, I missed that it was in a can_do_pipelined_register_writes() block.
I agree - this should work out fine then.

I don't think you need to post a v4 - I looked through your branch, and
everything looks good.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150511/1ffcac6c/attachment.sig>