[Mesa-dev] [PATCH] i965: Order write of query availablity with earlier writes

[Chris Wilson]

On Sat, Nov 05, 2016 at 11:48:57PM +0100, Alejandro Piñeiro wrote:
> On 05/11/16 19:57, Chris Wilson wrote:
> > Currently we signal the availabilty of the query result using an
> typo: availability (this also affects commit message)
> > unordered pipe-control write. As it is unordered, it may be executed
> > before the write of the query result itself - and so an observer may
> > read the query result too early. 
> From that "may", I assume that the problem this patch tries to fix
> doesn't happen 100% of the times, and that depends on a race condition.
> Is that true?

It depends on the observation being executed within the period of
the writes being inflight to see undefined behaviour. The piglit tests
get executed within the same batch (with the pipecontrols being inside
the fifo at the same time) and do experience the read always being
ordered before the write.

> > Fix this by requesting that the write
> > of the availablity flag is ordered after earlier pipe control writes.
> >
> > Testcase: piglit/arb_query_buffer_object-qbo/*async*
> What that means? That this patch fixes those tests? or just that the
> functionality is tested by those piglit tests?

Both.

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  src/mesa/drivers/dri/i965/gen6_queryobj.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > index bbd3c44..f6b90f7 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > @@ -60,8 +60,16 @@ set_query_availability(struct brw_context *brw, struct brw_query_object *query,
> >      */
> >     if (brw->ctx.Extensions.ARB_query_buffer_object &&
> 
> Just before this if there is a really big comment explaining what the
> code is doing, and why it is as it is. Probably it would be good to
> update, specially when it mentions CS stalls, that you use below.

And? I think the comments I added compliment the comment above,
explaining why it has to be so.
 
> >         brw_is_query_pipelined(query)) {
> > -      brw_emit_pipe_control_write(brw,
> > -                                  PIPE_CONTROL_WRITE_IMMEDIATE,
> > +      unsigned flags = PIPE_CONTROL_WRITE_IMMEDIATE;
> > +
> > +      if (available)
> > +         /* Order available *after* the query results */
> > +         flags |= PIPE_CONTROL_FLUSH_ENABLE;
> 
> As it is two lines of code, it needs brackets (as reference, the
> previous code technically didn't need the brackets for the
> brw_emit_pipe_control_write call, but it had it, as it was written using
> more than one line of code).

There's only one line of code here.

> > +      else
> > +         /* Make it unavailable *before* any pipelined reads */
> > +         flags |= PIPE_CONTROL_CS_STALL;
> Ditto.

Ditto.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre