[Mesa-dev] [PATCH 2/3] Model INTEL perf query backend after query object BE

[Kenneth Graunke]

On Wednesday, February 22, 2017 10:35:24 AM PST Robert Bragg wrote:
> On Wed, Feb 22, 2017 at 1:24 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Wednesday, February 15, 2017 1:37:36 PM PST Robert Bragg wrote:
[snip]
> >> 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.

We assume bool types become 0 or 1 when cast to integers, as guaranteed
by C99.  There are existing examples of bool:1 in NIR, i965, and vc4.
I think it should be OK in Mesa core.

But, feel free to use unsigned.  Or GLuint when you have a rationale
for doing so, as above.  A lot of people just use GL types everywhere
for no particular reason, but it makes some sense here.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170222/02a7d44b/attachment-0001.sig>