[Mesa-dev] [PATCH 05/10] i965: Rely on hardware contexts for query objects on Gen6+.

[Kenneth Graunke]

On 05/20/2013 10:31 AM, Paul Berry wrote:
> On 17 May 2013 10:17, Kenneth Graunke <kenneth at whitecape.org> wrote:
>     diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>     b/src/mesa/drivers/dri/i965/brw_context.c
>     index 2f5fedb..beade5c 100644
>     --- a/src/mesa/drivers/dri/i965/brw_context.c
>     +++ b/src/mesa/drivers/dri/i965/brw_context.c
>     @@ -88,6 +88,8 @@ static void brwInitDriverFunctions(struct
>     intel_screen *screen,
>
>          brwInitFragProgFuncs( functions );
>          brw_init_queryobj_functions(functions);
>     +   if (screen->gen >= 6)
>     +      gen6_reinit_queryobj_functions(functions);
>
>
> I find it confusing that we initialize the queryobj function pointers to
> the pre-gen6 functions and then immediately override some of them to
> gen6+ versions.
>
> How about splitting brw_init_queryobj_functions() into
> brw_init_common_queryobj_functions() and
> brw_init_pre_gen6_queryobj_functions(), and renaming
> gen6_reinit_queryobj_functions() to just gen6_init_queryobj_functions()?
>
> Then this code would change to:
>
> brw_init_common_queryobj_functions()
> if (screen->gen < 6)
>     brw_init_pre_gen6_queryobj_functions()
> else
>     gen6_init_queryobj_functions()

Sure.  I've done that for v2.

>          functions->QuerySamplesForFormat = brw_query_samples_for_format;
>          functions->BeginTransformFeedback = brw_begin_transform_feedback;
>     diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>     b/src/mesa/drivers/dri/i965/brw_context.h
>     index 9baf57b..9ef6aca 100644
>     --- a/src/mesa/drivers/dri/i965/brw_context.h
>     +++ b/src/mesa/drivers/dri/i965/brw_context.h
>     @@ -1164,6 +1164,9 @@ void brw_init_queryobj_functions(struct
>     dd_function_table *functions);
>       void brw_emit_query_begin(struct brw_context *brw);
>       void brw_emit_query_end(struct brw_context *brw);
>
>     +/** gen6_queryobj.c */
>     +void gen6_reinit_queryobj_functions(struct dd_function_table
>     *functions);
>     +
>       /*======================================================================
>        * brw_state_dump.c
>        */
>     diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c
>     b/src/mesa/drivers/dri/i965/brw_queryobj.c
>     index 40f926b..1c1e0b4 100644
>     --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
>     +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
>
>
> So is brw_queryobj.c now for pre-Gen6 only?  If so, can we please add a
> comment to that effect at the top of the file, and remove the remaining
> intel->gen checks (e.g. in write_timestamp())?  If not, can we somehow
> document which functions are Pre-gen6 and which aren't (e.g. with
> "assert(intel->gen < 6);" at the top of the pre-gen6 functions)?  As it
> stands this patch leaves the file in a condition where it's hard to tell
> without a deep understanding of the driver which functions are for which
> chip generations.

Not completely.  The timer query functions are still shared, as are the 
new/delete hooks.  I've added a bunch of assertions and comments to clarify.

[snip]
>          case GL_SAMPLES_PASSED_ARB:
>
>
> Below this code, in the GL_PRIMITIVES_GENERATED and
> GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN cases, can we change the
> comment so that instead of saying "We don't actually query the hardware
> for this value, so query->bo should always be NULL and execution should
> never reach here." it says something like "Transform feedback isn't
> supported pre-gen6"?
>
> Similarly, there's code in brw_begin_query() and brw_end_query() for
> handling GL_PRIMITIVES_GENERATED and
> GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN that is now unreachable because
> we don't support transform feedback prior to Gen6.  I think we should
> replace it with a comment to that affect and an assertion.

I've done basically just that in a follow-up patch which reworks a bunch 
of the transform feedback stuff.  Unless it's a big deal, I'll just plan 
on keeping it as a follow-up.

>     @@ -545,6 +502,9 @@ brw_emit_query_begin(struct brw_context *brw)
>          struct gl_context *ctx = &intel->ctx;
>          struct brw_query_object *query = brw->query.obj;
>
>     +   if (intel->hw_ctx)
>     +      return;
>     +
>
>
> The comment above brw_emit_query_begin() says that we record
> PS_DEPTH_COUNT at the beginning and end of each batch, regardless of
> whether hardware contexts are in use.  Can we please change the comment
> to reflect the new behavioukr?

Done.