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

[Nicolai Hähnle]

On 11.01.2016 14:30, Ilia Mirkin wrote:
> On Mon, Jan 11, 2016 at 2:21 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> 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.
>
> Batch queries like pipeline stats? That's what the index is for. Or is
> there something else?

Batch queries as in create_batch_query.


>> 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.
>
> wait means whatever you want it to mean. At least that's what the GL
> spec says. You can still block if wait = false. But yeah, ideally this
> function should *never* block, although the whole wait thing seems a
> bit unclear. I had originally wanted to just always write true for it
> in the core, but then decided that was a bit much.

Yeah, it's unclear, but I guess we have a common understanding that works :)

Cheers,
Nicolai

>>
>> 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?
>
> I think so. My plan is to make available check whether the fence
> attached to the query has passed, and for wait to stick in a sync into
> the pipeline. All done with a macro or two. (Macros allow configurable
> commands to be created on the GPU, evaluated by the graphics engine.)
>
>    -ilia
>