[Mesa-dev] [PATCH 7/9] gallium: add a way to store query result into buffer

[Nicolai Hähnle]

On 11.01.2016 14:01, Ilia Mirkin wrote:
> On Mon, Jan 11, 2016 at 1:53 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> On 10.01.2016 00:14, Ilia Mirkin wrote:
>>>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>    src/gallium/docs/source/context.rst  |  5 +++++
>>>    src/gallium/include/pipe/p_context.h | 24 ++++++++++++++++++++++++
>>>    src/gallium/include/pipe/p_defines.h |  2 ++
>>>    3 files changed, 31 insertions(+)
>>>
>>> diff --git a/src/gallium/docs/source/context.rst
>>> b/src/gallium/docs/source/context.rst
>>> index 9a32716..a97308a 100644
>>> --- a/src/gallium/docs/source/context.rst
>>> +++ b/src/gallium/docs/source/context.rst
>>> @@ -325,6 +325,11 @@ returned).  Otherwise, if the ``wait`` parameter is
>>> FALSE, the call
>>>    will not block and the return value will be TRUE if the query has
>>>    completed or FALSE otherwise.
>>>
>>> +``get_query_result_resource`` is used to store the result of a query into
>>> +a resource without synchronizing with the CPU. This write will optionally
>>> +wait for the query to complete, and will optionally write whether the
>>> value
>>> +is available instead of the value itself.
>>> +
>>>    The interface currently includes the following types of queries:
>>>
>>>    ``PIPE_QUERY_OCCLUSION_COUNTER`` counts the number of fragments which
>>> diff --git a/src/gallium/include/pipe/p_context.h
>>> b/src/gallium/include/pipe/p_context.h
>>> index be7447d..4f21d37 100644
>>> --- a/src/gallium/include/pipe/p_context.h
>>> +++ b/src/gallium/include/pipe/p_context.h
>>> @@ -150,6 +150,30 @@ struct pipe_context {
>>>                                   struct pipe_query *q,
>>>                                   boolean wait,
>>>                                   union pipe_query_result *result);
>>> +
>>> +   /**
>>> +    * Get results of a query, storing into resource.
>>> +    * \param wait  if true, this query will block until the result is
>>> ready
>>> +    * \param result_type  the type of the value being stored:
>>> +    *                       0 - i32
>>> +    *                       1 - u32
>>> +    *                       2 - i64
>>> +    *                       3 - u64
>>
>>
>> Magic numbers :(
>
> Documented! :)
>
>> Maybe use an enum? Or maybe the signed vs. unsigned distinction isn't really
>> needed and you can go with the pipe_index_buffer precedence of using
>> result_size instead (4 for i32, 8 for i64).
>
> Seemed overkill to create defines or an enum for this. But I suppose I
> can if others also like having enums for small lists of values used in
> just a handful of places.
>
> The GL logic clamps i32 and u32 to their respective ranges (on the
> upper end). I suspect this is done for a reason and a similar clamping
> should be done by this function as well. If we all collectively agree
> that we don't care about clamping, result_size sounds good to me.

No, you're right, this matters for number of samples passed and so on. I 
know I'm being annoying, but I think this should be an enum.

Upon further thought, two more issues:

1) Interaction with batch queries. For now, you'll probably just want to 
document that get_query_result_resource cannot be used with them.

2) Do we want to be more precise about what 'wait' means? AFAIU, 
get_query_result_resource should _never_ block the CPU, because it 
should be implemented by some command sent to the GPU (perhaps a small 
compute shader or transform feedback thing). The only way this command 
would not have the query results available are if (a) the downstream 
part of the pipeline is still running some other command or (b) the 
driver decides to use a separate ring buffer to implement this.

So in a hardware implementation, the different between wait = true vs. 
false is that you'd issue a pipeline flush before the 
get_query_result_resource commands. Right?

Cheers,
Nicolai

>
>    -ilia
>