I think I brought this up recently (perhaps this is even in response to my comments about this)... my suggestion was to introduce a draw flag that says whether the draw should be counted.
mipmap (and blit) should be using pipe->blit, which should never get counted anyways, so all that's left are clears. A flag in pipe_draw_info would solve it nicely. (Or another solution is to introduce caps to teach ->clear about all the various cases, and not worrying about impls that fall back to the clear_with_quad mechanism.) Either way, I don't think it's necessary to pause/resume around pipe->blit ops. -ilia On Mon, Aug 3, 2015 at 6:53 PM, Brian Paul <bri...@vmware.com> wrote: > glClear, blitting, mipmap generation, etc. implemented with quad drawing > should not effect queries like occlusion count. To accomplish that, this > patch implements a simple pause/resume mechanism for active queries. > > We now keep a list of active queries. When we "pause" queries we actually > end them. When we resume we get the query result and save it, then re-begin > the query. When the user requests the query result we add the pre-paused > value. > > As mentioned in the code, this is not a perfect/ideal solution but it fixes > Piglit's occlusion_query_meta_no_fragments and occlusion_query_meta_save > tests. In practice, it's pretty unusual to do meta operations between > glBeginQuery/EndQuery() pairs so these are corner cases. > --- > src/mesa/state_tracker/st_cb_blit.c | 7 ++ > src/mesa/state_tracker/st_cb_clear.c | 5 + > src/mesa/state_tracker/st_cb_queryobj.c | 199 > ++++++++++++++++++++++++++++++++ > src/mesa/state_tracker/st_cb_queryobj.h | 10 ++ > src/mesa/state_tracker/st_context.c | 3 + > src/mesa/state_tracker/st_context.h | 11 +- > src/mesa/state_tracker/st_gen_mipmap.c | 5 + > 7 files changed, 239 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/state_tracker/st_cb_blit.c > b/src/mesa/state_tracker/st_cb_blit.c > index 1396906..77fd908 100644 > --- a/src/mesa/state_tracker/st_cb_blit.c > +++ b/src/mesa/state_tracker/st_cb_blit.c > @@ -39,6 +39,7 @@ > #include "st_cb_bitmap.h" > #include "st_cb_blit.h" > #include "st_cb_fbo.h" > +#include "st_cb_queryobj.h" > #include "st_manager.h" > > #include "util/u_format.h" > @@ -193,6 +194,8 @@ st_BlitFramebuffer(struct gl_context *ctx, > blit.filter = pFilter; > blit.render_condition_enable = TRUE; > > + st_pause_queries(st); > + > if (mask & GL_COLOR_BUFFER_BIT) { > struct gl_renderbuffer_attachment *srcAtt = > &readFB->Attachment[readFB->_ColorReadBufferIndex]; > @@ -204,6 +207,7 @@ st_BlitFramebuffer(struct gl_context *ctx, > GLuint i; > > if (!srcObj || !srcObj->pt) { > + st_resume_queries(st); > return; > } > > @@ -239,6 +243,7 @@ st_BlitFramebuffer(struct gl_context *ctx, > GLuint i; > > if (!srcRb || !srcRb->surface) { > + st_resume_queries(st); > return; > } > > @@ -345,6 +350,8 @@ st_BlitFramebuffer(struct gl_context *ctx, > } > } > } > + > + st_resume_queries(st); > } > > > diff --git a/src/mesa/state_tracker/st_cb_clear.c > b/src/mesa/state_tracker/st_cb_clear.c > index 137fac8..fae28ee 100644 > --- a/src/mesa/state_tracker/st_cb_clear.c > +++ b/src/mesa/state_tracker/st_cb_clear.c > @@ -43,6 +43,7 @@ > #include "st_atom.h" > #include "st_cb_clear.h" > #include "st_cb_fbo.h" > +#include "st_cb_queryobj.h" > #include "st_format.h" > #include "st_program.h" > > @@ -255,6 +256,8 @@ clear_with_quad(struct gl_context *ctx, unsigned > clear_buffers) > x1, y1); > */ > > + st_pause_queries(st); > + > cso_save_blend(st->cso_context); > cso_save_stencil_ref(st->cso_context); > cso_save_depth_stencil_alpha(st->cso_context); > @@ -381,6 +384,8 @@ clear_with_quad(struct gl_context *ctx, unsigned > clear_buffers) > cso_restore_vertex_elements(st->cso_context); > cso_restore_aux_vertex_buffer_slot(st->cso_context); > cso_restore_stream_outputs(st->cso_context); > + > + st_resume_queries(st); > } > > > diff --git a/src/mesa/state_tracker/st_cb_queryobj.c > b/src/mesa/state_tracker/st_cb_queryobj.c > index 71222e8..080e18d 100644 > --- a/src/mesa/state_tracker/st_cb_queryobj.c > +++ b/src/mesa/state_tracker/st_cb_queryobj.c > @@ -33,6 +33,8 @@ > */ > > > +#include "util/u_debug.h" > + > #include "main/imports.h" > #include "main/context.h" > > @@ -80,6 +82,51 @@ st_DeleteQuery(struct gl_context *ctx, struct > gl_query_object *q) > } > > > +/** > + * Add a query to the active list. > + */ > +static void > +save_active_query(struct st_context *st, struct st_query_object *stq) > +{ > + /* We don't care about pausing/resuming time queries */ > + if (stq->base.Target != GL_TIME_ELAPSED && > + stq->base.Target != GL_TIMESTAMP) { > + struct st_query_list_node *node = CALLOC_STRUCT(st_query_list_node); > + > + if (node) { > + node->query = stq; > + insert_at_tail(&st->active_queries.list, &node->list); > + } > + } > +} > + > + > +/** > + * Remove a query from the active list. > + */ > +static void > +remove_active_query(struct st_context *st, struct st_query_object *stq) > +{ > + /* We don't care about pausing/resuming time queries */ > + if (stq->base.Target != GL_TIME_ELAPSED && > + stq->base.Target != GL_TIMESTAMP) { > + struct simple_node *n; > + > + foreach (n, &st->active_queries.list) { > + struct st_query_list_node *node = (struct st_query_list_node *) n; > + > + if (node->query == stq) { > + /* found it, remove it */ > + remove_from_list(&node->list); > + free(node); > + return; > + } > + } > + assert(!"query not found in remove_active_query()"); > + } > +} > + > + > static void > st_BeginQuery(struct gl_context *ctx, struct gl_query_object *q) > { > @@ -163,6 +210,8 @@ st_BeginQuery(struct gl_context *ctx, struct > gl_query_object *q) > } > } > assert(stq->type == type); > + > + save_active_query(st_context(ctx), stq); > } > > > @@ -183,6 +232,68 @@ st_EndQuery(struct gl_context *ctx, struct > gl_query_object *q) > > if (stq->pq) > pipe->end_query(pipe, stq->pq); > + > + remove_active_query(st_context(ctx), stq); > +} > + > + > +/** > + * Basically compute orig += new for query results. > + */ > +static void > +combine_query_results(union pipe_query_result *orig, > + const union pipe_query_result *new, > + unsigned query_target) > +{ > + switch (query_target) { > + case PIPE_QUERY_OCCLUSION_PREDICATE: > + case PIPE_QUERY_SO_OVERFLOW_PREDICATE: > + case PIPE_QUERY_GPU_FINISHED: > + orig->b |= new->b; > + break; > + case PIPE_QUERY_OCCLUSION_COUNTER: > + case PIPE_QUERY_TIMESTAMP: > + case PIPE_QUERY_TIME_ELAPSED: > + case PIPE_QUERY_PRIMITIVES_GENERATED: > + case PIPE_QUERY_PRIMITIVES_EMITTED: > + orig->u64 += new->u64; > + break; > + case PIPE_QUERY_SO_STATISTICS: > + orig->so_statistics.num_primitives_written > + += new->so_statistics.num_primitives_written; > + orig->so_statistics.primitives_storage_needed > + += new->so_statistics.primitives_storage_needed; > + break; > + case PIPE_QUERY_TIMESTAMP_DISJOINT: > + /* XXX is this right? */ > + orig->timestamp_disjoint.frequency = new->timestamp_disjoint.frequency; > + orig->timestamp_disjoint.disjoint = TRUE; > + break; > + case PIPE_QUERY_PIPELINE_STATISTICS: > + orig->pipeline_statistics.ia_vertices > + += new->pipeline_statistics.ia_vertices; > + orig->pipeline_statistics.vs_invocations > + += new->pipeline_statistics.vs_invocations; > + orig->pipeline_statistics.hs_invocations > + += new->pipeline_statistics.hs_invocations; > + orig->pipeline_statistics.ds_invocations > + += new->pipeline_statistics.ds_invocations; > + orig->pipeline_statistics.gs_invocations > + += new->pipeline_statistics.gs_invocations; > + orig->pipeline_statistics.gs_primitives > + += new->pipeline_statistics.gs_primitives; > + orig->pipeline_statistics.ps_invocations > + += new->pipeline_statistics.ps_invocations; > + orig->pipeline_statistics.cs_invocations > + += new->pipeline_statistics.cs_invocations; > + orig->pipeline_statistics.c_invocations > + += new->pipeline_statistics.c_invocations; > + orig->pipeline_statistics.c_primitives > + += new->pipeline_statistics.c_primitives; > + break; > + default: > + assert(!"Unexpected query type in combine_query_results()"); > + } > } > > > @@ -203,6 +314,9 @@ get_query_result(struct pipe_context *pipe, > if (!pipe->get_query_result(pipe, stq->pq, wait, &data)) > return FALSE; > > + /* accumulate any results from being paused with this result */ > + combine_query_results(&data, &stq->pre_paused_result, stq->type); > + > switch (stq->base.Target) { > case GL_VERTICES_SUBMITTED_ARB: > stq->base.Result = data.pipeline_statistics.ia_vertices; > @@ -305,3 +419,88 @@ void st_init_query_functions(struct dd_function_table > *functions) > functions->CheckQuery = st_CheckQuery; > functions->GetTimestamp = st_GetTimestamp; > } > + > + > +/** > + * This is used to temporarily suspend any active queries (occlusion, xfb, > + * etc) when we're about to do a "meta" operation, such as doing a scissored > + * glClear with polygon drawing. > + * We basically implement the pause/resume by stopping/re-beginning the query > + * and saving the pre-pause query result. The pre-pause result is added to > + * the later query when its result is returned. > + * > + * Note: this scheme is not perfect: > + * 1. It doesn't work correctly with predicated rendering. But it would be > + * pretty unusual to do a glClear/Blit/GenerateMipmap between the > + * glBeginQuery/EndQuery() pair for the occlusion test phase. > + * 2. If we implement GL_ARB_query_buffer_object, we'd have to stuff the > + * accumulated query result into the buffer, negating the performance > + * benefit of that feature. Again, it's quite unusual to have meta > + * operations inside glBeginQuery/EndQuery(). > + */ > +void > +st_pause_queries(struct st_context *st) > +{ > + struct simple_node *n; > + > + assert(!st->queries_paused); > + st->queries_paused = TRUE; > + > + /* Loop over active queries */ > + foreach (n, &st->active_queries.list) { > + struct st_query_list_node *qln = (struct st_query_list_node *) n; > + > + assert(qln->query->pq); > + if (!qln->query->pq) > + continue; > + > + /* end the query */ > + st->pipe->end_query(st->pipe, qln->query->pq); > + > + /* Note: we don't get the query result here. We postpone that until > + * st_resume_queries() to give the graphics pipe a little more time > + * to possibly finish the query to avoid blocking. > + */ > + } > +} > + > + > +/** > + * Counterpart to st_pause_queries(), called after finishing meta operations. > + * We "resume" the active queries by beginning new queries. > + */ > +void > +st_resume_queries(struct st_context *st) > +{ > + struct simple_node *n; > + > + assert(st->queries_paused); > + st->queries_paused = FALSE; > + > + /* Loop over active queries */ > + foreach (n, &st->active_queries.list) { > + struct st_query_list_node *qln = (struct st_query_list_node *) n; > + union pipe_query_result result; > + > + assert(qln->query->pq); > + if (!qln->query->pq) > + continue; > + > + /* get the result for the query which we ended in st_pause_queries() */ > + if (!st->pipe->get_query_result(st->pipe, qln->query->pq, > + TRUE, &result)) { > + debug_printf("get query result failed\n"); > + continue; > + } > + > + /* Accumulate this result with previously paused result. > + * Note that we could pause/resume a query many times so we have to > + * accumulate here. > + */ > + combine_query_results(&qln->query->pre_paused_result, &result, > + qln->query->type); > + > + /* re-begin the query */ > + st->pipe->begin_query(st->pipe, qln->query->pq); > + } > +} > diff --git a/src/mesa/state_tracker/st_cb_queryobj.h > b/src/mesa/state_tracker/st_cb_queryobj.h > index 2406321..6de3e9e 100644 > --- a/src/mesa/state_tracker/st_cb_queryobj.h > +++ b/src/mesa/state_tracker/st_cb_queryobj.h > @@ -42,6 +42,11 @@ struct st_query_object > /* Begin TIMESTAMP query for GL_TIME_ELAPSED_EXT queries */ > struct pipe_query *pq_begin; > > + /* Used for pausing/resuming queries. When a query result is requested, > + * we have to add this to the returned result. > + */ > + union pipe_query_result pre_paused_result; > + > unsigned type; /**< PIPE_QUERY_x */ > }; > > @@ -59,5 +64,10 @@ st_query_object(struct gl_query_object *q) > extern void > st_init_query_functions(struct dd_function_table *functions); > > +extern void > +st_pause_queries(struct st_context *st); > + > +extern void > +st_resume_queries(struct st_context *st); > > #endif > diff --git a/src/mesa/state_tracker/st_context.c > b/src/mesa/state_tracker/st_context.c > index 72c23ca..224bd0d 100644 > --- a/src/mesa/state_tracker/st_context.c > +++ b/src/mesa/state_tracker/st_context.c > @@ -305,6 +305,9 @@ st_create_context_priv( struct gl_context *ctx, struct > pipe_context *pipe, > _mesa_initialize_dispatch_tables(ctx); > _mesa_initialize_vbo_vtxfmt(ctx); > > + /* Init query object list */ > + make_empty_list(&st->active_queries.list); > + > return st; > } > > diff --git a/src/mesa/state_tracker/st_context.h > b/src/mesa/state_tracker/st_context.h > index 81d5480..ce9815c 100644 > --- a/src/mesa/state_tracker/st_context.h > +++ b/src/mesa/state_tracker/st_context.h > @@ -28,6 +28,7 @@ > #ifndef ST_CONTEXT_H > #define ST_CONTEXT_H > > +#include "util/simple_list.h" > #include "main/mtypes.h" > #include "pipe/p_state.h" > #include "state_tracker/st_api.h" > @@ -74,7 +75,11 @@ struct st_tracked_state { > void (*update)( struct st_context *st ); > }; > > - > +struct st_query_list_node > +{ > + struct simple_node list; > + struct st_query_object *query; > +}; > > struct st_context > { > @@ -215,6 +220,10 @@ struct st_context > int32_t read_stamp; > > struct st_config_options options; > + > + /** List of active queries */ > + struct st_query_list_node active_queries; > + boolean queries_paused; > }; > > > diff --git a/src/mesa/state_tracker/st_gen_mipmap.c > b/src/mesa/state_tracker/st_gen_mipmap.c > index 26e1c21..956794d 100644 > --- a/src/mesa/state_tracker/st_gen_mipmap.c > +++ b/src/mesa/state_tracker/st_gen_mipmap.c > @@ -40,6 +40,7 @@ > #include "st_context.h" > #include "st_texture.h" > #include "st_gen_mipmap.h" > +#include "st_cb_queryobj.h" > #include "st_cb_texture.h" > > > @@ -145,6 +146,8 @@ st_generate_mipmap(struct gl_context *ctx, GLenum target, > last_layer = util_max_layer(pt, baseLevel); > } > > + st_pause_queries(st); > + > /* Try to generate the mipmap by rendering/texturing. If that fails, > * use the software fallback. > */ > @@ -153,6 +156,8 @@ st_generate_mipmap(struct gl_context *ctx, GLenum target, > _mesa_generate_mipmap(ctx, target, texObj); > } > > + st_resume_queries(st); > + > /* Fill in the Mesa gl_texture_image fields */ > for (dstLevel = baseLevel + 1; dstLevel <= lastLevel; dstLevel++) { > const uint srcLevel = dstLevel - 1; > -- > 1.9.1 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev