On Wed, Feb 22, 2017 at 1:24 AM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Wednesday, February 15, 2017 1:37:36 PM PST Robert Bragg wrote:
>> Instead of using the same backend interface as AMD_performance_monitor
>> this defines a dedicated INTEL_performance_query interface that is
>> modelled more on the ARB_query_buffer_object interface (considering the
>> similarity of the extensions) with the addition of vfuncs for
>> initializing and enumerating query and counter info.
>
> Patches 1 and 2's commit titles should start with "mesa: ".
Ah, yup okey.
>
>> Compared to the previous backend, some notable differences are:
>>
>> - The backend is free to represent counters using whatever data
>> structures are optimal/convenient since queries and counters are
>> enumerated via an iterator api instead of declaring them using
>> structures directly shared with the frontend.
>>
>> This is also done to help us support the full range of data and
>> semantic types available with INTEL_performance_query which is awkward
>> while using a structure shared with the AMD_performance_monitor
>> backend since neither extension's types are a subset of the other.
>>
>> - The backend must support waiting for a query instead of the frontend
>> simply using glFinish().
>>
>> - Objects go through 'Active' and 'Ready' states consistent with the
>> query object backend (hopefully making them more familiar). There is
>> no 'Ended' state (which used to show that a query has ended at least
>> once for a given object). There is a new 'Used' state similar to the
>> 'EverBound' state of query objects, set when a query is first begun
>> which implies that we are expecting to get results back for the object
>> at some point.
>
> That's a little different from EverBound, which is used to answer stupid
> glIsFoo() queries - where glGenFoo() doesn't actually "create" a Foo,
> but glBindFoo() does. An awkward concept.
Ok, makes sense now. I've updated the comment to note there's no
equivalent to EverBound needed here.
>
>> The INTEL_performance_query and AMD_performance_monitor extensions are
>> now completely orthogonal within Mesa main (though a driver could
>> optionally choose to implement both extensions within a unified backend
>> if that were convenient for the sake of sharing state/code).
>>
>> v2: (Samuel Pitoiset)
>> - init PerfQuery.NumQueries in frontend
>> - s/return_string/output_clipped_string/
>> - s/backed/backend/ typo
>> - remove redundant *bytesWritten = 0
>> v3:
>> - Add InitPerfQueryInfo for lazy probing of available queries
>>
>> Signed-off-by: Robert Bragg <rob...@sixbynine.org>
>> ---
>> src/mesa/main/dd.h | 41 +++
>> src/mesa/main/mtypes.h | 24 +-
>> src/mesa/main/performance_query.c | 625
>> ++++++++++++++------------------------
>> src/mesa/main/performance_query.h | 6 +-
>> 4 files changed, 295 insertions(+), 401 deletions(-)
>>
>> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
>> index 7ebd084ca3..e77df31cf2 100644
>> --- a/src/mesa/main/dd.h
>> +++ b/src/mesa/main/dd.h
>> @@ -780,6 +780,47 @@ struct dd_function_table {
>> /*@}*/
>>
>> /**
>> + * \name Performance Query objects
>> + */
>> + /*@{*/
>> + GLuint (*InitPerfQueryInfo)(struct gl_context *ctx);
>> + void (*GetPerfQueryInfo)(struct gl_context *ctx,
>> + int queryIndex,
>> + const char **name,
>> + GLuint *dataSize,
>> + GLuint *numCounters,
>> + GLuint *numActive);
>> + void (*GetPerfCounterInfo)(struct gl_context *ctx,
>> + int queryIndex,
>> + int counterIndex,
>> + const char **name,
>> + const char **desc,
>> + GLuint *offset,
>> + GLuint *data_size,
>> + GLuint *type_enum,
>> + GLuint *data_type_enum,
>> + GLuint64 *raw_max);
>> + struct gl_perf_query_object * (*NewPerfQueryObject)(struct gl_context
>> *ctx,
>> + int queryIndex);
>> + void (*DeletePerfQuery)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj);
>> + GLboolean (*BeginPerfQuery)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj);
>> + void (*EndPerfQuery)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj);
>> + void (*WaitPerfQuery)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj);
>> + GLboolean (*IsPerfQueryReady)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj);
>> + void (*GetPerfQueryData)(struct gl_context *ctx,
>> + struct gl_perf_query_object *obj,
>> + GLsizei dataSize,
>> + GLuint *data,
>> + GLuint *bytesWritten);
>> + /*@}*/
>> +
>> +
>> + /**
>> * \name GREMEDY debug/marker functions
>> */
>> /*@{*/
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index f3a24df589..e6cf1f4af6 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -1860,6 +1860,23 @@ struct gl_perf_monitor_group
>>
>>
>> /**
>> + * A query object instance as described in INTEL_performance_query.
>> + *
>> + * NB: We want to keep this and the corresponding backend structure
>> + * relatively lean considering that applications may expect to
>> + * allocate enough objects to be able to query around all draw calls
>> + * in a frame.
>> + */
>> +struct gl_perf_query_object
>> +{
>> + GLuint Id; /**< hash table ID/name */
>> + GLuint Used:1; /**< has been used for 1 or more queries */
>> + GLuint Active:1; /**< inside Begin/EndPerfQuery */
>> + GLuint Ready:1; /**< result is ready? */
>
> Please use "unsigned Id" and "bool Used:1" - we're trying to get away
> from GL type aliases when not directly API-facing.
Ah right I was generally aware of that but doing a skimming everything
with this in mind I found a few other little bits to clean up though I
ended having some second thoughts about these particular members:
This Id is a record of the GLuint ID given to the application, just
used for debugging currently. The value is returned by
_mesa_HashFindFreeKeyBlock() which is currently implemented in terms
of the GLuint type. One other place where we access the same ID for
debugging is via _mesa_HashWalk() which takes a callback expecting a
GLuint argument. I can still change, but when I thought about this it
felt like it was indeed a directly api facing value.
For the bitfields I started over thinking what it means to have a bool
bitfield since I doubted whether it could be assumed to be unsigned
and then wondered about the potential for a bug with some code trying
to compare a bitfield value == 1, or indexing an array. Does Mesa
require a c99 compiler, otherwise I don't think it's unheard of for
bool to end up as a signed int typedef. Anyway, besides the
overly-pedantic thought, I guessed you wouldn't really mind me using
"unsigned Used:1;" for the sake of avoiding GLuint. I don't think it
would make a practical difference since the struct will be naturally
padded to 8 bytes in all likelyhood either way. I'll prod you on IRC
to check if you really have a strong opinion here before I push.
>
> [snip]
>
>> - /* The specification does not state that this produces an error. */
>> + /* The specification does not state that this produces an error but
>> + * to be consistent with glGetFirstPerfQueryIdINTEL we generate an
>> + * INVALID_VALUE error */
>
> */ goes on its own line.
I found a few other comments to fixup similarly.
>
> With the "mesa: ..." prefix added, patches 1-2 are (weakly):
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
Thanks.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
