Re: [Mesa-dev] [PATCH v2 11/26] gallium/u_threaded: avoid syncs for get_query_result

[Marek Olšák]

This commit makes most query piglit tests crash. I've not investigated further.

Marek

On Mon, Nov 6, 2017 at 11:23 AM, Nicolai Hähnle  wrote:
> From: Nicolai Hähnle 
>
> Queries should still get marked as flushed when flushes are executed
> asynchronously in the driver thread.
>
> To this end, the management of the unflushed_queries list is moved into
> the driver thread.
>
> Reviewed-by: Marek Olšák 
> ---
>  src/gallium/auxiliary/util/u_threaded_context.c | 65 
> ++---
>  1 file changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_threaded_context.c 
> b/src/gallium/auxiliary/util/u_threaded_context.c
> index 0bb645e8522..4908ea8a7ba 100644
> --- a/src/gallium/auxiliary/util/u_threaded_context.c
> +++ b/src/gallium/auxiliary/util/u_threaded_context.c
> @@ -321,91 +321,107 @@ tc_create_batch_query(struct pipe_context *_pipe, 
> unsigned num_queries,
>  {
> struct threaded_context *tc = threaded_context(_pipe);
> struct pipe_context *pipe = tc->pipe;
>
> return pipe->create_batch_query(pipe, num_queries, query_types);
>  }
>
>  static void
>  tc_call_destroy_query(struct pipe_context *pipe, union tc_payload *payload)
>  {
> +   struct threaded_query *tq = threaded_query(payload->query);
> +
> +   if (tq->head_unflushed.next)
> +  LIST_DEL(>head_unflushed);
> +
> pipe->destroy_query(pipe, payload->query);
>  }
>
>  static void
>  tc_destroy_query(struct pipe_context *_pipe, struct pipe_query *query)
>  {
> struct threaded_context *tc = threaded_context(_pipe);
> -   struct threaded_query *tq = threaded_query(query);
> -
> -   if (tq->head_unflushed.next)
> -  LIST_DEL(>head_unflushed);
>
> tc_add_small_call(tc, TC_CALL_destroy_query)->query = query;
>  }
>
>  static void
>  tc_call_begin_query(struct pipe_context *pipe, union tc_payload *payload)
>  {
> pipe->begin_query(pipe, payload->query);
>  }
>
>  static boolean
>  tc_begin_query(struct pipe_context *_pipe, struct pipe_query *query)
>  {
> struct threaded_context *tc = threaded_context(_pipe);
> union tc_payload *payload = tc_add_small_call(tc, TC_CALL_begin_query);
>
> payload->query = query;
> return true; /* we don't care about the return value for this call */
>  }
>
> +struct tc_end_query_payload {
> +   struct threaded_context *tc;
> +   struct pipe_query *query;
> +};
> +
>  static void
>  tc_call_end_query(struct pipe_context *pipe, union tc_payload *payload)
>  {
> -   pipe->end_query(pipe, payload->query);
> +   struct tc_end_query_payload *p = (struct tc_end_query_payload *)payload;
> +   struct threaded_query *tq = threaded_query(p->query);
> +
> +   if (!tq->head_unflushed.next)
> +  LIST_ADD(>head_unflushed, >tc->unflushed_queries);
> +
> +   pipe->end_query(pipe, p->query);
>  }
>
>  static bool
>  tc_end_query(struct pipe_context *_pipe, struct pipe_query *query)
>  {
> struct threaded_context *tc = threaded_context(_pipe);
> struct threaded_query *tq = threaded_query(query);
> -   union tc_payload *payload = tc_add_small_call(tc, TC_CALL_end_query);
> +   struct tc_end_query_payload *payload =
> +  tc_add_struct_typed_call(tc, TC_CALL_end_query, tc_end_query_payload);
> +
> +   tc_add_small_call(tc, TC_CALL_end_query);
>
> +   payload->tc = tc;
> payload->query = query;
>
> tq->flushed = false;
> -   if (!tq->head_unflushed.next)
> -  LIST_ADD(>head_unflushed, >unflushed_queries);
>
> return true; /* we don't care about the return value for this call */
>  }
>
>  static boolean
>  tc_get_query_result(struct pipe_context *_pipe,
>  struct pipe_query *query, boolean wait,
>  union pipe_query_result *result)
>  {
> struct threaded_context *tc = threaded_context(_pipe);
> struct threaded_query *tq = threaded_query(query);
> struct pipe_context *pipe = tc->pipe;
>
> if (!tq->flushed)
>tc_sync_msg(tc, wait ? "wait" : "nowait");
>
> bool success = pipe->get_query_result(pipe, query, wait, result);
>
> if (success) {
>tq->flushed = true;
> -  if (tq->head_unflushed.next)
> +  if (tq->head_unflushed.next) {
> + /* This is safe because it can only happen after we sync'd. */
>   LIST_DEL(>head_unflushed);
> +  }
> }
> return success;
>  }
>
>  struct tc_query_result_resource {
> struct pipe_query *query;
> boolean wait;
> enum pipe_query_value_type result_type;
> int index;
> struct pipe_resource *resource;
> @@ -1806,42 +1822,60 @@ tc_create_video_buffer(struct pipe_context *_pipe,
> unreachable("Threaded context should not be enabled for video APIs");
> return NULL;
>  }
>
>
>  /
>   * draw, launch, clear, blit, copy, flush
>   */
>
>  struct tc_flush_payload {
> +   struct threaded_context *tc;
> struct pipe_fence_handle 

[Mesa-dev] [PATCH v2 11/26] gallium/u_threaded: avoid syncs for get_query_result

[Nicolai Hähnle]

From: Nicolai Hähnle 

Queries should still get marked as flushed when flushes are executed
asynchronously in the driver thread.

To this end, the management of the unflushed_queries list is moved into
the driver thread.

Reviewed-by: Marek Olšák 
---
 src/gallium/auxiliary/util/u_threaded_context.c | 65 ++---
 1 file changed, 48 insertions(+), 17 deletions(-)

diff --git a/src/gallium/auxiliary/util/u_threaded_context.c 
b/src/gallium/auxiliary/util/u_threaded_context.c
index 0bb645e8522..4908ea8a7ba 100644
--- a/src/gallium/auxiliary/util/u_threaded_context.c
+++ b/src/gallium/auxiliary/util/u_threaded_context.c
@@ -321,91 +321,107 @@ tc_create_batch_query(struct pipe_context *_pipe, 
unsigned num_queries,
 {
struct threaded_context *tc = threaded_context(_pipe);
struct pipe_context *pipe = tc->pipe;
 
return pipe->create_batch_query(pipe, num_queries, query_types);
 }
 
 static void
 tc_call_destroy_query(struct pipe_context *pipe, union tc_payload *payload)
 {
+   struct threaded_query *tq = threaded_query(payload->query);
+
+   if (tq->head_unflushed.next)
+  LIST_DEL(>head_unflushed);
+
pipe->destroy_query(pipe, payload->query);
 }
 
 static void
 tc_destroy_query(struct pipe_context *_pipe, struct pipe_query *query)
 {
struct threaded_context *tc = threaded_context(_pipe);
-   struct threaded_query *tq = threaded_query(query);
-
-   if (tq->head_unflushed.next)
-  LIST_DEL(>head_unflushed);
 
tc_add_small_call(tc, TC_CALL_destroy_query)->query = query;
 }
 
 static void
 tc_call_begin_query(struct pipe_context *pipe, union tc_payload *payload)
 {
pipe->begin_query(pipe, payload->query);
 }
 
 static boolean
 tc_begin_query(struct pipe_context *_pipe, struct pipe_query *query)
 {
struct threaded_context *tc = threaded_context(_pipe);
union tc_payload *payload = tc_add_small_call(tc, TC_CALL_begin_query);
 
payload->query = query;
return true; /* we don't care about the return value for this call */
 }
 
+struct tc_end_query_payload {
+   struct threaded_context *tc;
+   struct pipe_query *query;
+};
+
 static void
 tc_call_end_query(struct pipe_context *pipe, union tc_payload *payload)
 {
-   pipe->end_query(pipe, payload->query);
+   struct tc_end_query_payload *p = (struct tc_end_query_payload *)payload;
+   struct threaded_query *tq = threaded_query(p->query);
+
+   if (!tq->head_unflushed.next)
+  LIST_ADD(>head_unflushed, >tc->unflushed_queries);
+
+   pipe->end_query(pipe, p->query);
 }
 
 static bool
 tc_end_query(struct pipe_context *_pipe, struct pipe_query *query)
 {
struct threaded_context *tc = threaded_context(_pipe);
struct threaded_query *tq = threaded_query(query);
-   union tc_payload *payload = tc_add_small_call(tc, TC_CALL_end_query);
+   struct tc_end_query_payload *payload =
+  tc_add_struct_typed_call(tc, TC_CALL_end_query, tc_end_query_payload);
+
+   tc_add_small_call(tc, TC_CALL_end_query);
 
+   payload->tc = tc;
payload->query = query;
 
tq->flushed = false;
-   if (!tq->head_unflushed.next)
-  LIST_ADD(>head_unflushed, >unflushed_queries);
 
return true; /* we don't care about the return value for this call */
 }
 
 static boolean
 tc_get_query_result(struct pipe_context *_pipe,
 struct pipe_query *query, boolean wait,
 union pipe_query_result *result)
 {
struct threaded_context *tc = threaded_context(_pipe);
struct threaded_query *tq = threaded_query(query);
struct pipe_context *pipe = tc->pipe;
 
if (!tq->flushed)
   tc_sync_msg(tc, wait ? "wait" : "nowait");
 
bool success = pipe->get_query_result(pipe, query, wait, result);
 
if (success) {
   tq->flushed = true;
-  if (tq->head_unflushed.next)
+  if (tq->head_unflushed.next) {
+ /* This is safe because it can only happen after we sync'd. */
  LIST_DEL(>head_unflushed);
+  }
}
return success;
 }
 
 struct tc_query_result_resource {
struct pipe_query *query;
boolean wait;
enum pipe_query_value_type result_type;
int index;
struct pipe_resource *resource;
@@ -1806,42 +1822,60 @@ tc_create_video_buffer(struct pipe_context *_pipe,
unreachable("Threaded context should not be enabled for video APIs");
return NULL;
 }
 
 
 /
  * draw, launch, clear, blit, copy, flush
  */
 
 struct tc_flush_payload {
+   struct threaded_context *tc;
struct pipe_fence_handle *fence;
unsigned flags;
 };
 
 static void
+tc_flush_queries(struct threaded_context *tc)
+{
+   struct threaded_query *tq, *tmp;
+   LIST_FOR_EACH_ENTRY_SAFE(tq, tmp, >unflushed_queries, head_unflushed) {
+  LIST_DEL(>head_unflushed);
+
+  /* Memory release semantics: due to a possible race with
+   * tc_get_query_result, we must ensure that the linked list changes
+   * are visible before setting