On Monday, May 11, 2015 03:14:21 PM Neil Roberts wrote:
> Kenneth Graunke <kenn...@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,
> - NeilAhh, 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 <kenn...@whitecape.org>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
