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

[Alejandro Piñeiro]

On 19/11/16 08:48, Chris Wilson wrote:
> 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.

Ok.

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

Ok.

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

Ok, it was just a suggestion.

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

Sorry, I poorly explained myself. Yes, there is just one line of code.
But two lines if you count the comment. Current style, AFAIS, adds
brackets for that case too. See brw_clear.c line 197 for an example.

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

With that small thing (brackets) fixed:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>