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

[Roland Scheidegger]

Am 15.12.18 um 22:39 schrieb Ilia Mirkin:
> On Sat, Dec 15, 2018 at 4:12 PM Kenneth Graunke <kenneth at 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 at 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