[Mesa-dev] [PATCH 3/5] etnaviv: add support for occlusion queries

[Christian Gmeiner]

Am 19.10.2017 7:46 nachm. schrieb "Wladimir J. van der Laan" <
laanwj at gmail.com>:

On Tue, Oct 17, 2017 at 10:38:15PM +0200, Christian Gmeiner wrote:
> Passes most occlusion query piglits. The following piglits are broken:
> - spec at arb_occlusion_query@occlusion_query_meta_fragments
> - spec at arb_occlusion_query@occlusion_query_meta_save
> - spec at arb_occlusion_query2@render
>
> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>

Comments inline

> +static void
> +occlusion_stop(struct etna_hw_query *hq, struct etna_context *ctx)
> +{
> +   etna_set_state(ctx->stream, VIVS_GL_OCCLUSION_QUERY_CONTROL,
0x1DF5E76);

Does the actual value written matter here?
If so, it'd make sense to define a constant (or bit definitions in the
rnndb).
If not it's fine like this, or add a comment that it's just a "random"
nonce.


I tried random values and all of them worked - 0x1DF5E76 is the value used
by blob. Will add a comment about the value.


> +static const struct etna_hw_sample_provider occlusion_counter = {
> +   .query_type = PIPE_QUERY_OCCLUSION_COUNTER,
> +   .start = occlusion_start,
> +   .stop = occlusion_stop,
> +   .suspend = occlusion_suspend,
> +   .resume = occlusion_resume,
> +   .result = occlusion_counter_result,
> +};
> +
> +static const struct etna_hw_sample_provider occlusion_predicate = {
> +   .query_type = PIPE_QUERY_OCCLUSION_PREDICATE,
> +   .start = occlusion_start,
> +   .stop = occlusion_stop,
> +   .suspend = occlusion_suspend,
> +   .resume = occlusion_resume,
> +   .result = occlusion_predicate_result,
> +};
> +
> +static const struct etna_hw_sample_provider
occlusion_predicate_conservative = {
> +   .query_type = PIPE_QUERY_OCCLUSION_PREDICATE_CONSERVATIVE,
> +   .start = occlusion_start,
> +   .stop = occlusion_stop,
> +   .suspend = occlusion_suspend,
> +   .resume = occlusion_resume,
> +   .result = occlusion_predicate_result,
> +};

Is it intentional that this defines the same fields three times?

Why not return the same structure for all three cases? Is this expected to
change to
specific implementations soon in another patch?


There is one difference - how the sum is interpreted - uint64_t vs. bool
value. In general the code
is written in a way that we could easily add other hw sample providers.
I will change the current implementation to only have one sample provider
for all occlusion types
and handle the different result evaluation directly in occlusion_result(..).

Hope that is okay - will send out a new patch series asap.

greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171019/eb55a9a8/attachment.html>