<div dir="ltr"><div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">Am 19.10.2017 7:46 nachm. schrieb "Wladimir J. van der Laan" <<a href="mailto:laanwj@gmail.com" target="_blank">laanwj@gmail.com</a>>:<br type="attribution"><blockquote class="gmail-m_-6233892616580220865quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-m_-6233892616580220865quoted-text">On Tue, Oct 17, 2017 at 10:38:15PM +0200, Christian Gmeiner wrote:<br>
> Passes most occlusion query piglits. The following piglits are broken:<br>
> - spec@arb_occlusion_query@occlu<wbr>sion_query_meta_fragments<br>
> - spec@arb_occlusion_query@occlu<wbr>sion_query_meta_save<br>
> - spec@arb_occlusion_query2@rend<wbr>er<br>
><br>
> Signed-off-by: Christian Gmeiner <<a href="mailto:christian.gmeiner@gmail.com" target="_blank">christian.gmeiner@gmail.com</a>><br>
<br>
</div>Comments inline<br>
<div class="gmail-m_-6233892616580220865quoted-text"><br>
> +static void<br>
> +occlusion_stop(struct etna_hw_query *hq, struct etna_context *ctx)<br>
> +{<br>
> +   etna_set_state(ctx->stream, VIVS_GL_OCCLUSION_QUERY_CONTRO<wbr>L, 0x1DF5E76);<br>
<br>
</div>Does the actual value written matter here?<br>
If so, it'd make sense to define a constant (or bit definitions in the rnndb).<br>
If not it's fine like this, or add a comment that it's just a "random" nonce.<br>
</blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">I tried random values and all of them worked - <span style="font-family:sans-serif">0x1DF5E76 is the value used by blob. Will add a comment about the value.</span></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail-m_-6233892616580220865quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-m_-6233892616580220865elided-text"><br>
> +static const struct etna_hw_sample_provider occlusion_counter = {<br>
> +   .query_type = PIPE_QUERY_OCCLUSION_COUNTER,<br>
> +   .start = occlusion_start,<br>
> +   .stop = occlusion_stop,<br>
> +   .suspend = occlusion_suspend,<br>
> +   .resume = occlusion_resume,<br>
> +   .result = occlusion_counter_result,<br>
> +};<br>
> +<br>
> +static const struct etna_hw_sample_provider occlusion_predicate = {<br>
> +   .query_type = PIPE_QUERY_OCCLUSION_PREDICATE<wbr>,<br>
> +   .start = occlusion_start,<br>
> +   .stop = occlusion_stop,<br>
> +   .suspend = occlusion_suspend,<br>
> +   .resume = occlusion_resume,<br>
> +   .result = occlusion_predicate_result,<br>
> +};<br>
> +<br>
> +static const struct etna_hw_sample_provider occlusion_predicate_conservati<wbr>ve = {<br>
> +   .query_type = PIPE_QUERY_OCCLUSION_PREDICATE<wbr>_CONSERVATIVE,<br>
> +   .start = occlusion_start,<br>
> +   .stop = occlusion_stop,<br>
> +   .suspend = occlusion_suspend,<br>
> +   .resume = occlusion_resume,<br>
> +   .result = occlusion_predicate_result,<br>
> +};<br>
<br>
</div>Is it intentional that this defines the same fields three times?<br>
<br>
Why not return the same structure for all three cases? Is this expected to change to<br>
specific implementations soon in another patch?<br></blockquote></div></div></div><div dir="auto"><br></div><div>There is one difference - how the sum is interpreted - uint64_t vs. bool value. In general the code</div><div>is written in a way that we could easily add other hw sample providers.</div>I will change the current implementation to only have one sample provider for all occlusion types<div> and handle the different result evaluation directly in occlusion_result(..).</div></div><div dir="auto"><br></div><div>Hope that is okay - will send out a new patch series asap.<br></div><div dir="auto"><br></div><div dir="auto">greets<br>--<br>Christian Gmeiner, MSc<br><br><a href="https://christian-gmeiner.info">https://christian-gmeiner.info</a></div>
</div>