[1/3] st/mesa: Make an enum for pipeline statistics query result indices.

Submitted by Kenneth Graunke on Dec. 15, 2018, 9:44 a.m.

Details

Message ID 20181215094456.15435-1-kenneth@whitecape.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Dec. 15, 2018, 9:44 a.m.
Gallium handles pipeline statistics queries as a single query
(PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
Sometimes it's useful to refer to each of those values individually,
rather than as a group.  To avoid hardcoding numbers, we define a new
enum for each value.  Here, the name and enum value correspond to the
index in the struct pipe_query_data_pipeline_statistics result.

Cc: Roland Scheidegger <sroland@vmware.com>
---
 src/gallium/include/pipe/p_defines.h    | 17 +++++++++++++++++
 src/mesa/state_tracker/st_cb_queryobj.c | 22 +++++++++++-----------
 2 files changed, 28 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/include/pipe/p_defines.h b/src/gallium/include/pipe/p_defines.h
index 6d96f1ccb5b..21005955a36 100644
--- a/src/gallium/include/pipe/p_defines.h
+++ b/src/gallium/include/pipe/p_defines.h
@@ -568,6 +568,23 @@  enum pipe_query_type {
    PIPE_QUERY_DRIVER_SPECIFIC = 256,
 };
 
+/**
+ * Index for PIPE_QUERY_PIPELINE_STATISTICS subqueries.
+ */
+enum pipe_statistics_query_index {
+   PIPE_STAT_QUERY_IA_VERTICES,
+   PIPE_STAT_QUERY_IA_PRIMITIVES,
+   PIPE_STAT_QUERY_VS_INVOCATIONS,
+   PIPE_STAT_QUERY_GS_INVOCATIONS,
+   PIPE_STAT_QUERY_GS_PRIMITIVES,
+   PIPE_STAT_QUERY_C_INVOCATIONS,
+   PIPE_STAT_QUERY_C_PRIMITIVES,
+   PIPE_STAT_QUERY_PS_INVOCATIONS,
+   PIPE_STAT_QUERY_HS_INVOCATIONS,
+   PIPE_STAT_QUERY_DS_INVOCATIONS,
+   PIPE_STAT_QUERY_CS_INVOCATIONS,
+};
+
 /**
  * Conditional rendering modes
  */
diff --git a/src/mesa/state_tracker/st_cb_queryobj.c b/src/mesa/state_tracker/st_cb_queryobj.c
index 69e6004c3f1..82f53243336 100644
--- a/src/mesa/state_tracker/st_cb_queryobj.c
+++ b/src/mesa/state_tracker/st_cb_queryobj.c
@@ -386,37 +386,37 @@  st_StoreQueryResult(struct gl_context *ctx, struct gl_query_object *q,
    } else if (stq->type == PIPE_QUERY_PIPELINE_STATISTICS) {
       switch (q->Target) {
       case GL_VERTICES_SUBMITTED_ARB:
-         index = 0;
+         index = PIPE_STAT_QUERY_IA_VERTICES;
          break;
       case GL_PRIMITIVES_SUBMITTED_ARB:
-         index = 1;
+         index = PIPE_STAT_QUERY_IA_PRIMITIVES;
          break;
       case GL_VERTEX_SHADER_INVOCATIONS_ARB:
-         index = 2;
+         index = PIPE_STAT_QUERY_VS_INVOCATIONS;
          break;
       case GL_GEOMETRY_SHADER_INVOCATIONS:
-         index = 3;
+         index = PIPE_STAT_QUERY_GS_INVOCATIONS;
          break;
       case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
-         index = 4;
+         index = PIPE_STAT_QUERY_GS_PRIMITIVES;
          break;
       case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
-         index = 5;
+         index = PIPE_STAT_QUERY_C_INVOCATIONS;
          break;
       case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
-         index = 6;
+         index = PIPE_STAT_QUERY_C_PRIMITIVES;
          break;
       case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
-         index = 7;
+         index = PIPE_STAT_QUERY_PS_INVOCATIONS;
          break;
       case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
-         index = 8;
+         index = PIPE_STAT_QUERY_HS_INVOCATIONS;
          break;
       case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
-         index = 9;
+         index = PIPE_STAT_QUERY_DS_INVOCATIONS;
          break;
       case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
-         index = 10;
+         index = PIPE_STAT_QUERY_CS_INVOCATIONS;
          break;
       default:
          unreachable("Unexpected target");

Comments

On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org> wrote:
> Gallium handles pipeline statistics queries as a single query
> (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> Sometimes it's useful to refer to each of those values individually,
> rather than as a group.  To avoid hardcoding numbers, we define a new
> enum for each value.  Here, the name and enum value correspond to the
> index in the struct pipe_query_data_pipeline_statistics result.

This later-in-the-series desire to be able to get just one value back
from Gallium is an API change which would break any existing d3d1x
state trackers. I realize you're not changing any drivers, but in my
mind, it's preferable not to have ambiguous APIs where some drivers do
one thing, others do another. For NVIDIA, we have to fetch the
counters individually too, but we just do them all. It's ~free to do
so, and we only do one set of synchronization for all of them.

Anyways, I'd rather not have this ambiguous thing of "you could return
some or all" situation. We should be more definitive. I'd recommend
adding a PIPE_QUERY_PIPELINE_STATISTICS_ONE to differentiate [or
PIPE_QUERY_PIPELINE_STATISTIC if you want to be clever and cause lots
of typos], along with a CAP for support.

>
> Cc: Roland Scheidegger <sroland@vmware.com>
> ---
>  src/gallium/include/pipe/p_defines.h    | 17 +++++++++++++++++
>  src/mesa/state_tracker/st_cb_queryobj.c | 22 +++++++++++-----------
>  2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/include/pipe/p_defines.h b/src/gallium/include/pipe/p_defines.h
> index 6d96f1ccb5b..21005955a36 100644
> --- a/src/gallium/include/pipe/p_defines.h
> +++ b/src/gallium/include/pipe/p_defines.h
> @@ -568,6 +568,23 @@ enum pipe_query_type {
>     PIPE_QUERY_DRIVER_SPECIFIC = 256,
>  };
>
> +/**
> + * Index for PIPE_QUERY_PIPELINE_STATISTICS subqueries.
> + */
> +enum pipe_statistics_query_index {
> +   PIPE_STAT_QUERY_IA_VERTICES,
> +   PIPE_STAT_QUERY_IA_PRIMITIVES,
> +   PIPE_STAT_QUERY_VS_INVOCATIONS,
> +   PIPE_STAT_QUERY_GS_INVOCATIONS,
> +   PIPE_STAT_QUERY_GS_PRIMITIVES,
> +   PIPE_STAT_QUERY_C_INVOCATIONS,
> +   PIPE_STAT_QUERY_C_PRIMITIVES,
> +   PIPE_STAT_QUERY_PS_INVOCATIONS,
> +   PIPE_STAT_QUERY_HS_INVOCATIONS,
> +   PIPE_STAT_QUERY_DS_INVOCATIONS,
> +   PIPE_STAT_QUERY_CS_INVOCATIONS,
> +};
> +
>  /**
>   * Conditional rendering modes
>   */
> diff --git a/src/mesa/state_tracker/st_cb_queryobj.c b/src/mesa/state_tracker/st_cb_queryobj.c
> index 69e6004c3f1..82f53243336 100644
> --- a/src/mesa/state_tracker/st_cb_queryobj.c
> +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> @@ -386,37 +386,37 @@ st_StoreQueryResult(struct gl_context *ctx, struct gl_query_object *q,
>     } else if (stq->type == PIPE_QUERY_PIPELINE_STATISTICS) {
>        switch (q->Target) {
>        case GL_VERTICES_SUBMITTED_ARB:
> -         index = 0;
> +         index = PIPE_STAT_QUERY_IA_VERTICES;
>           break;
>        case GL_PRIMITIVES_SUBMITTED_ARB:
> -         index = 1;
> +         index = PIPE_STAT_QUERY_IA_PRIMITIVES;
>           break;
>        case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> -         index = 2;
> +         index = PIPE_STAT_QUERY_VS_INVOCATIONS;
>           break;
>        case GL_GEOMETRY_SHADER_INVOCATIONS:
> -         index = 3;
> +         index = PIPE_STAT_QUERY_GS_INVOCATIONS;
>           break;
>        case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> -         index = 4;
> +         index = PIPE_STAT_QUERY_GS_PRIMITIVES;
>           break;
>        case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> -         index = 5;
> +         index = PIPE_STAT_QUERY_C_INVOCATIONS;
>           break;
>        case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> -         index = 6;
> +         index = PIPE_STAT_QUERY_C_PRIMITIVES;
>           break;
>        case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> -         index = 7;
> +         index = PIPE_STAT_QUERY_PS_INVOCATIONS;
>           break;
>        case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> -         index = 8;
> +         index = PIPE_STAT_QUERY_HS_INVOCATIONS;
>           break;
>        case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> -         index = 9;
> +         index = PIPE_STAT_QUERY_DS_INVOCATIONS;
>           break;
>        case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> -         index = 10;
> +         index = PIPE_STAT_QUERY_CS_INVOCATIONS;
>           break;
>        default:
>           unreachable("Unexpected target");
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Saturday, December 15, 2018 9:10:46 AM PST Ilia Mirkin wrote:
> On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org> wrote:
> > Gallium handles pipeline statistics queries as a single query
> > (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> > Sometimes it's useful to refer to each of those values individually,
> > rather than as a group.  To avoid hardcoding numbers, we define a new
> > enum for each value.  Here, the name and enum value correspond to the
> > index in the struct pipe_query_data_pipeline_statistics result.
> 
> This later-in-the-series desire to be able to get just one value back
> from Gallium is an API change which would break any existing d3d1x
> state trackers. I realize you're not changing any drivers, but in my
> mind, it's preferable not to have ambiguous APIs where some drivers do
> one thing, others do another. For NVIDIA, we have to fetch the
> counters individually too, but we just do them all. It's ~free to do
> so, and we only do one set of synchronization for all of them.

Yes, I suppose you're right in that the existing interface is designed
to return all 11 counters, and I'm essesntially implementing it wrong
because I didn't understand it.  It seemed like more of an inconsistency
in get_query_results vs get_query_results_resource, where one of those
knew what value to fetch, and the other did not.

While it may not be that expensive to return 11 values, it isn't free.
The ARB_pipeline_statistics_query extension is designed to help port
D3D1x apps to OpenGL, but it provides GL-style single-value queries
rather than a single query that returns a struct.  So, if a D3D1x
translator naively tries to fetch all 11 counters, it would have to
do 11 different GL queries...each of which would map to Gallium's
return-all-11 interface...resulting in 121 counter reads.

> Anyways, I'd rather not have this ambiguous thing of "you could return
> some or all" situation. We should be more definitive. I'd recommend
> adding a PIPE_QUERY_PIPELINE_STATISTICS_ONE to differentiate [or
> PIPE_QUERY_PIPELINE_STATISTIC if you want to be clever and cause lots
> of typos], along with a CAP for support.

I'd like to have an interface that better serves the in-tree state
trackers.  st/mesa wants 1 value, but it seems st/nine wants 2.  So
the single interface isn't ideal for nine, either, I guess.

If people would prefer that we add a new query type and capability bit,
I can do that, but I probably won't bother implementing the all-11 mode
in my driver, then.

--Ken
On Sat, Dec 15, 2018 at 4:12 PM Kenneth Graunke <kenneth@whitecape.org> wrote:
>
> On Saturday, December 15, 2018 9:10:46 AM PST Ilia Mirkin wrote:
> > On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org> wrote:
> > > Gallium handles pipeline statistics queries as a single query
> > > (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> > > Sometimes it's useful to refer to each of those values individually,
> > > rather than as a group.  To avoid hardcoding numbers, we define a new
> > > enum for each value.  Here, the name and enum value correspond to the
> > > index in the struct pipe_query_data_pipeline_statistics result.
> >
> > This later-in-the-series desire to be able to get just one value back
> > from Gallium is an API change which would break any existing d3d1x
> > state trackers. I realize you're not changing any drivers, but in my
> > mind, it's preferable not to have ambiguous APIs where some drivers do
> > one thing, others do another. For NVIDIA, we have to fetch the
> > counters individually too, but we just do them all. It's ~free to do
> > so, and we only do one set of synchronization for all of them.
>
> Yes, I suppose you're right in that the existing interface is designed
> to return all 11 counters, and I'm essesntially implementing it wrong
> because I didn't understand it.  It seemed like more of an inconsistency
> in get_query_results vs get_query_results_resource, where one of those
> knew what value to fetch, and the other did not.

I implemented the direct-write-to-resource logic, and there was no
use-case for fetching them all, and you had to support a GL API where
you had to be able to place each value into a precise location. I
didn't want to alter the other API at the time as I didn't feel it
hurt too much.

>
> While it may not be that expensive to return 11 values, it isn't free.
> The ARB_pipeline_statistics_query extension is designed to help port
> D3D1x apps to OpenGL, but it provides GL-style single-value queries
> rather than a single query that returns a struct.  So, if a D3D1x
> translator naively tries to fetch all 11 counters, it would have to
> do 11 different GL queries...each of which would map to Gallium's
> return-all-11 interface...resulting in 121 counter reads.

Yep. Not ideal. (Which was never my argument anyways -- having a way
to return a single value would definitely be better.)

>
> > Anyways, I'd rather not have this ambiguous thing of "you could return
> > some or all" situation. We should be more definitive. I'd recommend
> > adding a PIPE_QUERY_PIPELINE_STATISTICS_ONE to differentiate [or
> > PIPE_QUERY_PIPELINE_STATISTIC if you want to be clever and cause lots
> > of typos], along with a CAP for support.
>
> I'd like to have an interface that better serves the in-tree state
> trackers.  st/mesa wants 1 value, but it seems st/nine wants 2.  So
> the single interface isn't ideal for nine, either, I guess.

Historically we've tried to avoid creating breakage for VMware where
it wasn't necessary.

>
> If people would prefer that we add a new query type and capability bit,
> I can do that, but I probably won't bother implementing the all-11 mode
> in my driver, then.

I think that's fine. It's already behind some PIPE_CAP. The state
tracker could then do like

if ONE: get_one();
if ALL: get_all(); select one();

Cheers,

  -ilia
Am 15.12.18 um 22:39 schrieb Ilia Mirkin:
> On Sat, Dec 15, 2018 at 4:12 PM Kenneth Graunke <kenneth@whitecape.org> wrote:

>>

>> On Saturday, December 15, 2018 9:10:46 AM PST Ilia Mirkin wrote:

>>> On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org> wrote:

>>>> Gallium handles pipeline statistics queries as a single query

>>>> (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.

>>>> Sometimes it's useful to refer to each of those values individually,

>>>> rather than as a group.  To avoid hardcoding numbers, we define a new

>>>> enum for each value.  Here, the name and enum value correspond to the

>>>> index in the struct pipe_query_data_pipeline_statistics result.

>>>

>>> This later-in-the-series desire to be able to get just one value back

>>> from Gallium is an API change which would break any existing d3d1x

>>> state trackers. I realize you're not changing any drivers, but in my

>>> mind, it's preferable not to have ambiguous APIs where some drivers do

>>> one thing, others do another. For NVIDIA, we have to fetch the

>>> counters individually too, but we just do them all. It's ~free to do

>>> so, and we only do one set of synchronization for all of them.

>>

>> Yes, I suppose you're right in that the existing interface is designed

>> to return all 11 counters, and I'm essesntially implementing it wrong

>> because I didn't understand it.  It seemed like more of an inconsistency

>> in get_query_results vs get_query_results_resource, where one of those

>> knew what value to fetch, and the other did not.

> 

> I implemented the direct-write-to-resource logic, and there was no

> use-case for fetching them all, and you had to support a GL API where

> you had to be able to place each value into a precise location. I

> didn't want to alter the other API at the time as I didn't feel it

> hurt too much.

> 

>>

>> While it may not be that expensive to return 11 values, it isn't free.

>> The ARB_pipeline_statistics_query extension is designed to help port

>> D3D1x apps to OpenGL, but it provides GL-style single-value queries

>> rather than a single query that returns a struct.  So, if a D3D1x

>> translator naively tries to fetch all 11 counters, it would have to

>> do 11 different GL queries...each of which would map to Gallium's

>> return-all-11 interface...resulting in 121 counter reads.

> 

> Yep. Not ideal. (Which was never my argument anyways -- having a way

> to return a single value would definitely be better.)

> 

>>

>>> Anyways, I'd rather not have this ambiguous thing of "you could return

>>> some or all" situation. We should be more definitive. I'd recommend

>>> adding a PIPE_QUERY_PIPELINE_STATISTICS_ONE to differentiate [or

>>> PIPE_QUERY_PIPELINE_STATISTIC if you want to be clever and cause lots

>>> of typos], along with a CAP for support.

>>

>> I'd like to have an interface that better serves the in-tree state

>> trackers.  st/mesa wants 1 value, but it seems st/nine wants 2.  So

>> the single interface isn't ideal for nine, either, I guess.

> 

> Historically we've tried to avoid creating breakage for VMware where

> it wasn't necessary.

> 

>>

>> If people would prefer that we add a new query type and capability bit,

>> I can do that, but I probably won't bother implementing the all-11 mode

>> in my driver, then.

> 

> I think that's fine. It's already behind some PIPE_CAP. The state

> tracker could then do like

> 

> if ONE: get_one();

> if ALL: get_all(); select one();

> 

> Cheers,

> 

>   -ilia

> 


Yes, it's unfortunate d3d returns all values and GL returns just one.
Although FWIW outside of test suites we haven't really seen much use of
pipeline statistics queries, I'd guess that's the same for GL. Maybe
they are used for development/debugging/profiling of apps, but otherwise
it appears they don't see much (if any) use - as such they probably
aren't all that performance critical.

I think being explicit in the interface (in some way) if all or one
value is requested is a good idea (and not all drivers implementing
requesting all is ok).

(Patch 1 looks fine regradless to me fwiw.)

Roland
The series looks good. Another way to distinguish between return one and
return all is to use "index". index <= 11 returns one. index == ~0 returns
all. This is the least intrusive.

st/mesa and gallium/hud always want to get one.
st/nine and util/u_helpers always want to get all.

Marek

On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org>
wrote:

> Gallium handles pipeline statistics queries as a single query
> (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> Sometimes it's useful to refer to each of those values individually,
> rather than as a group.  To avoid hardcoding numbers, we define a new
> enum for each value.  Here, the name and enum value correspond to the
> index in the struct pipe_query_data_pipeline_statistics result.
>
> Cc: Roland Scheidegger <sroland@vmware.com>
> ---
>  src/gallium/include/pipe/p_defines.h    | 17 +++++++++++++++++
>  src/mesa/state_tracker/st_cb_queryobj.c | 22 +++++++++++-----------
>  2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/include/pipe/p_defines.h
> b/src/gallium/include/pipe/p_defines.h
> index 6d96f1ccb5b..21005955a36 100644
> --- a/src/gallium/include/pipe/p_defines.h
> +++ b/src/gallium/include/pipe/p_defines.h
> @@ -568,6 +568,23 @@ enum pipe_query_type {
>     PIPE_QUERY_DRIVER_SPECIFIC = 256,
>  };
>
> +/**
> + * Index for PIPE_QUERY_PIPELINE_STATISTICS subqueries.
> + */
> +enum pipe_statistics_query_index {
> +   PIPE_STAT_QUERY_IA_VERTICES,
> +   PIPE_STAT_QUERY_IA_PRIMITIVES,
> +   PIPE_STAT_QUERY_VS_INVOCATIONS,
> +   PIPE_STAT_QUERY_GS_INVOCATIONS,
> +   PIPE_STAT_QUERY_GS_PRIMITIVES,
> +   PIPE_STAT_QUERY_C_INVOCATIONS,
> +   PIPE_STAT_QUERY_C_PRIMITIVES,
> +   PIPE_STAT_QUERY_PS_INVOCATIONS,
> +   PIPE_STAT_QUERY_HS_INVOCATIONS,
> +   PIPE_STAT_QUERY_DS_INVOCATIONS,
> +   PIPE_STAT_QUERY_CS_INVOCATIONS,
> +};
> +
>  /**
>   * Conditional rendering modes
>   */
> diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
> b/src/mesa/state_tracker/st_cb_queryobj.c
> index 69e6004c3f1..82f53243336 100644
> --- a/src/mesa/state_tracker/st_cb_queryobj.c
> +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> @@ -386,37 +386,37 @@ st_StoreQueryResult(struct gl_context *ctx, struct
> gl_query_object *q,
>     } else if (stq->type == PIPE_QUERY_PIPELINE_STATISTICS) {
>        switch (q->Target) {
>        case GL_VERTICES_SUBMITTED_ARB:
> -         index = 0;
> +         index = PIPE_STAT_QUERY_IA_VERTICES;
>           break;
>        case GL_PRIMITIVES_SUBMITTED_ARB:
> -         index = 1;
> +         index = PIPE_STAT_QUERY_IA_PRIMITIVES;
>           break;
>        case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> -         index = 2;
> +         index = PIPE_STAT_QUERY_VS_INVOCATIONS;
>           break;
>        case GL_GEOMETRY_SHADER_INVOCATIONS:
> -         index = 3;
> +         index = PIPE_STAT_QUERY_GS_INVOCATIONS;
>           break;
>        case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> -         index = 4;
> +         index = PIPE_STAT_QUERY_GS_PRIMITIVES;
>           break;
>        case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> -         index = 5;
> +         index = PIPE_STAT_QUERY_C_INVOCATIONS;
>           break;
>        case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> -         index = 6;
> +         index = PIPE_STAT_QUERY_C_PRIMITIVES;
>           break;
>        case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> -         index = 7;
> +         index = PIPE_STAT_QUERY_PS_INVOCATIONS;
>           break;
>        case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> -         index = 8;
> +         index = PIPE_STAT_QUERY_HS_INVOCATIONS;
>           break;
>        case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> -         index = 9;
> +         index = PIPE_STAT_QUERY_DS_INVOCATIONS;
>           break;
>        case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> -         index = 10;
> +         index = PIPE_STAT_QUERY_CS_INVOCATIONS;
>           break;
>        default:
>           unreachable("Unexpected target");
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
That seems like a reasonable interface to me.

But, I don't think it's backwards compatible.  Today, don't state
trackers set index = 0 and expect all 11 to be returned?  We could
easily change the in-tree state trackers, but not sure about the
other ones.

We could always encode the index differently, but at that point, I
wonder if it would be cleaner to just add a new query type like Ilia
suggested.

Marek, what would you prefer?

--Ken

On Friday, December 21, 2018 1:24:32 PM PST Marek Olšák wrote:
> The series looks good. Another way to distinguish between return one and
> return all is to use "index". index <= 11 returns one. index == ~0 returns
> all. This is the least intrusive.
> 
> st/mesa and gallium/hud always want to get one.
> st/nine and util/u_helpers always want to get all.
> 
> Marek
> 
> On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org>
> wrote:
> 
> > Gallium handles pipeline statistics queries as a single query
> > (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> > Sometimes it's useful to refer to each of those values individually,
> > rather than as a group.  To avoid hardcoding numbers, we define a new
> > enum for each value.  Here, the name and enum value correspond to the
> > index in the struct pipe_query_data_pipeline_statistics result.
> >
> > Cc: Roland Scheidegger <sroland@vmware.com>
> > ---
> >  src/gallium/include/pipe/p_defines.h    | 17 +++++++++++++++++
> >  src/mesa/state_tracker/st_cb_queryobj.c | 22 +++++++++++-----------
> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/gallium/include/pipe/p_defines.h
> > b/src/gallium/include/pipe/p_defines.h
> > index 6d96f1ccb5b..21005955a36 100644
> > --- a/src/gallium/include/pipe/p_defines.h
> > +++ b/src/gallium/include/pipe/p_defines.h
> > @@ -568,6 +568,23 @@ enum pipe_query_type {
> >     PIPE_QUERY_DRIVER_SPECIFIC = 256,
> >  };
> >
> > +/**
> > + * Index for PIPE_QUERY_PIPELINE_STATISTICS subqueries.
> > + */
> > +enum pipe_statistics_query_index {
> > +   PIPE_STAT_QUERY_IA_VERTICES,
> > +   PIPE_STAT_QUERY_IA_PRIMITIVES,
> > +   PIPE_STAT_QUERY_VS_INVOCATIONS,
> > +   PIPE_STAT_QUERY_GS_INVOCATIONS,
> > +   PIPE_STAT_QUERY_GS_PRIMITIVES,
> > +   PIPE_STAT_QUERY_C_INVOCATIONS,
> > +   PIPE_STAT_QUERY_C_PRIMITIVES,
> > +   PIPE_STAT_QUERY_PS_INVOCATIONS,
> > +   PIPE_STAT_QUERY_HS_INVOCATIONS,
> > +   PIPE_STAT_QUERY_DS_INVOCATIONS,
> > +   PIPE_STAT_QUERY_CS_INVOCATIONS,
> > +};
> > +
> >  /**
> >   * Conditional rendering modes
> >   */
> > diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
> > b/src/mesa/state_tracker/st_cb_queryobj.c
> > index 69e6004c3f1..82f53243336 100644
> > --- a/src/mesa/state_tracker/st_cb_queryobj.c
> > +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> > @@ -386,37 +386,37 @@ st_StoreQueryResult(struct gl_context *ctx, struct
> > gl_query_object *q,
> >     } else if (stq->type == PIPE_QUERY_PIPELINE_STATISTICS) {
> >        switch (q->Target) {
> >        case GL_VERTICES_SUBMITTED_ARB:
> > -         index = 0;
> > +         index = PIPE_STAT_QUERY_IA_VERTICES;
> >           break;
> >        case GL_PRIMITIVES_SUBMITTED_ARB:
> > -         index = 1;
> > +         index = PIPE_STAT_QUERY_IA_PRIMITIVES;
> >           break;
> >        case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> > -         index = 2;
> > +         index = PIPE_STAT_QUERY_VS_INVOCATIONS;
> >           break;
> >        case GL_GEOMETRY_SHADER_INVOCATIONS:
> > -         index = 3;
> > +         index = PIPE_STAT_QUERY_GS_INVOCATIONS;
> >           break;
> >        case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> > -         index = 4;
> > +         index = PIPE_STAT_QUERY_GS_PRIMITIVES;
> >           break;
> >        case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> > -         index = 5;
> > +         index = PIPE_STAT_QUERY_C_INVOCATIONS;
> >           break;
> >        case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> > -         index = 6;
> > +         index = PIPE_STAT_QUERY_C_PRIMITIVES;
> >           break;
> >        case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> > -         index = 7;
> > +         index = PIPE_STAT_QUERY_PS_INVOCATIONS;
> >           break;
> >        case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> > -         index = 8;
> > +         index = PIPE_STAT_QUERY_HS_INVOCATIONS;
> >           break;
> >        case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> > -         index = 9;
> > +         index = PIPE_STAT_QUERY_DS_INVOCATIONS;
> >           break;
> >        case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> > -         index = 10;
> > +         index = PIPE_STAT_QUERY_CS_INVOCATIONS;
> >           break;
> >        default:
> >           unreachable("Unexpected target");
> > --
> > 2.19.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
On Fri, Dec 21, 2018, 6:28 PM Kenneth Graunke <kenneth@whitecape.org wrote:

> That seems like a reasonable interface to me.
>
> But, I don't think it's backwards compatible.  Today, don't state
> trackers set index = 0 and expect all 11 to be returned?  We could
> easily change the in-tree state trackers, but not sure about the
> other ones.
>
> We could always encode the index differently, but at that point, I
> wonder if it would be cleaner to just add a new query type like Ilia
> suggested.
>
> Marek, what would you prefer?
>

Backward compatibility is not required. Gallium is not a stable API. In
tree state trackers can be fixed easily. We shouldn't worry too much about
closed source state trackers.

Marek


> --Ken
>
> On Friday, December 21, 2018 1:24:32 PM PST Marek Olšák wrote:
> > The series looks good. Another way to distinguish between return one and
> > return all is to use "index". index <= 11 returns one. index == ~0
> returns
> > all. This is the least intrusive.
> >
> > st/mesa and gallium/hud always want to get one.
> > st/nine and util/u_helpers always want to get all.
> >
> > Marek
> >
> > On Sat, Dec 15, 2018 at 4:45 AM Kenneth Graunke <kenneth@whitecape.org>
> > wrote:
> >
> > > Gallium handles pipeline statistics queries as a single query
> > > (PIPE_QUERY_PIPELINE_STATISTICS) which returns a struct with 11 values.
> > > Sometimes it's useful to refer to each of those values individually,
> > > rather than as a group.  To avoid hardcoding numbers, we define a new
> > > enum for each value.  Here, the name and enum value correspond to the
> > > index in the struct pipe_query_data_pipeline_statistics result.
> > >
> > > Cc: Roland Scheidegger <sroland@vmware.com>
> > > ---
> > >  src/gallium/include/pipe/p_defines.h    | 17 +++++++++++++++++
> > >  src/mesa/state_tracker/st_cb_queryobj.c | 22 +++++++++++-----------
> > >  2 files changed, 28 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/gallium/include/pipe/p_defines.h
> > > b/src/gallium/include/pipe/p_defines.h
> > > index 6d96f1ccb5b..21005955a36 100644
> > > --- a/src/gallium/include/pipe/p_defines.h
> > > +++ b/src/gallium/include/pipe/p_defines.h
> > > @@ -568,6 +568,23 @@ enum pipe_query_type {
> > >     PIPE_QUERY_DRIVER_SPECIFIC = 256,
> > >  };
> > >
> > > +/**
> > > + * Index for PIPE_QUERY_PIPELINE_STATISTICS subqueries.
> > > + */
> > > +enum pipe_statistics_query_index {
> > > +   PIPE_STAT_QUERY_IA_VERTICES,
> > > +   PIPE_STAT_QUERY_IA_PRIMITIVES,
> > > +   PIPE_STAT_QUERY_VS_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_GS_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_GS_PRIMITIVES,
> > > +   PIPE_STAT_QUERY_C_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_C_PRIMITIVES,
> > > +   PIPE_STAT_QUERY_PS_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_HS_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_DS_INVOCATIONS,
> > > +   PIPE_STAT_QUERY_CS_INVOCATIONS,
> > > +};
> > > +
> > >  /**
> > >   * Conditional rendering modes
> > >   */
> > > diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
> > > b/src/mesa/state_tracker/st_cb_queryobj.c
> > > index 69e6004c3f1..82f53243336 100644
> > > --- a/src/mesa/state_tracker/st_cb_queryobj.c
> > > +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> > > @@ -386,37 +386,37 @@ st_StoreQueryResult(struct gl_context *ctx,
> struct
> > > gl_query_object *q,
> > >     } else if (stq->type == PIPE_QUERY_PIPELINE_STATISTICS) {
> > >        switch (q->Target) {
> > >        case GL_VERTICES_SUBMITTED_ARB:
> > > -         index = 0;
> > > +         index = PIPE_STAT_QUERY_IA_VERTICES;
> > >           break;
> > >        case GL_PRIMITIVES_SUBMITTED_ARB:
> > > -         index = 1;
> > > +         index = PIPE_STAT_QUERY_IA_PRIMITIVES;
> > >           break;
> > >        case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> > > -         index = 2;
> > > +         index = PIPE_STAT_QUERY_VS_INVOCATIONS;
> > >           break;
> > >        case GL_GEOMETRY_SHADER_INVOCATIONS:
> > > -         index = 3;
> > > +         index = PIPE_STAT_QUERY_GS_INVOCATIONS;
> > >           break;
> > >        case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> > > -         index = 4;
> > > +         index = PIPE_STAT_QUERY_GS_PRIMITIVES;
> > >           break;
> > >        case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> > > -         index = 5;
> > > +         index = PIPE_STAT_QUERY_C_INVOCATIONS;
> > >           break;
> > >        case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> > > -         index = 6;
> > > +         index = PIPE_STAT_QUERY_C_PRIMITIVES;
> > >           break;
> > >        case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> > > -         index = 7;
> > > +         index = PIPE_STAT_QUERY_PS_INVOCATIONS;
> > >           break;
> > >        case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> > > -         index = 8;
> > > +         index = PIPE_STAT_QUERY_HS_INVOCATIONS;
> > >           break;
> > >        case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> > > -         index = 9;
> > > +         index = PIPE_STAT_QUERY_DS_INVOCATIONS;
> > >           break;
> > >        case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> > > -         index = 10;
> > > +         index = PIPE_STAT_QUERY_CS_INVOCATIONS;
> > >           break;
> > >        default:
> > >           unreachable("Unexpected target");
> > > --
> > > 2.19.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
> >
>
>
On Fri, Dec 21, 2018, 19:16 Marek Olšák <maraeo@gmail.com wrote:

>
>
> On Fri, Dec 21, 2018, 6:28 PM Kenneth Graunke <kenneth@whitecape.org
> wrote:
>
>> That seems like a reasonable interface to me.
>>
>> But, I don't think it's backwards compatible.  Today, don't state
>> trackers set index = 0 and expect all 11 to be returned?  We could
>> easily change the in-tree state trackers, but not sure about the
>> other ones.
>>
>> We could always encode the index differently, but at that point, I
>> wonder if it would be cleaner to just add a new query type like Ilia
>> suggested.
>>
>> Marek, what would you prefer?
>>
>
> Backward compatibility is not required. Gallium is not a stable API. In
> tree state trackers can be fixed easily. We shouldn't worry too much about
> closed source state trackers.
>

Fwiw my take is that while it's fine to change apis around (we do this all
the time), we should avoid causing a loss of functionality just because no
in-tree state tracker uses it. I think having a forward-looking gallium API
greatly facilitated GL 3 and 4 bringup of gallium drivers, even though
there wasn't necessarily an in-tree way to access all the functionality at
the time.

As long as all the previously accessible functionality remains, I think
we're fine.
Am 22.12.18 um 01:21 schrieb Ilia Mirkin:
> 

> 

> On Fri, Dec 21, 2018, 19:16 Marek Olšák <maraeo@gmail.com

> <mailto:maraeo@gmail.com> wrote:

> 

> 

> 

>     On Fri, Dec 21, 2018, 6:28 PM Kenneth Graunke <kenneth@whitecape.org

>     <mailto:kenneth@whitecape.org> wrote:

> 

>         That seems like a reasonable interface to me.

> 

>         But, I don't think it's backwards compatible.  Today, don't state

>         trackers set index = 0 and expect all 11 to be returned?  We could

>         easily change the in-tree state trackers, but not sure about the

>         other ones.

> 

>         We could always encode the index differently, but at that point, I

>         wonder if it would be cleaner to just add a new query type like Ilia

>         suggested.

> 

>         Marek, what would you prefer?

> 

> 

>     Backward compatibility is not required. Gallium is not a stable API.

>     In tree state trackers can be fixed easily. We shouldn't worry too

>     much about closed source state trackers.

> 

> 

> Fwiw my take is that while it's fine to change apis around (we do this

> all the time), we should avoid causing a loss of functionality just

> because no in-tree state tracker uses it. I think having a

> forward-looking gallium API greatly facilitated GL 3 and 4 bringup of

> gallium drivers, even though there wasn't necessarily an in-tree way to

> access all the functionality at the time.

> 

> As long as all the previously accessible functionality remains, I think

> we're fine.


Yes, I agree. Certainly it isn't a problem for us to change the supplied
index to the query.
Although it is usually better if interface changes are not just
semantically different, but also syntactically, to avoid surprises - you
know there's always just this odd place in the code which for some
reason you failed to change, and a compile error is much better than
strange failures later (this is of course even more true for out-of-tree
state trackers). But this might not always be feasible, keeping the
interface nice should take priority. (There's not really any hard rules
for gallium interface changes, that's just my take on it.)

Roland



> _______________________________________________

> mesa-dev mailing list

> mesa-dev@lists.freedesktop.org