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 <ch...@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 <apinhe...@igalia.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
