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

[Ilia Mirkin]

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?

>
> 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.

>
> 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