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

[Alejandro Piñeiro]

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?
> 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?
> 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.

>         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).

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


> +
> +      brw_emit_pipe_control_write(brw, flags,
>                                    query->bo, 2 * sizeof(uint64_t),
>                                    available, 0);
>     }