anv: Fix sanitization of stencil state when the depth test is disabled

Submitted by Alex Smith on Oct. 25, 2018, 10:24 a.m.

Details

Message ID 20181025102434.24689-1-asmith@feralinteractive.com
State New
Headers show
Series "anv: Fix sanitization of stencil state when the depth test is disabled" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alex Smith Oct. 25, 2018, 10:24 a.m.
When depth testing is disabled, we shouldn't pay attention to the
specified depthCompareOp, and just treat it as always passing. Before,
if the depth test is disabled, but depthCompareOp is VK_COMPARE_OP_NEVER
(e.g. from the app having zero-initialized the structure), then
sanitize_stencil_face() would have incorrectly changed passOp to
VK_STENCIL_OP_KEEP.

Signed-off-by: Alex Smith <asmith@feralinteractive.com>
---
 src/intel/vulkan/genX_pipeline.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 33f1f7832a..877a9fb850 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -755,9 +755,13 @@  sanitize_ds_state(VkPipelineDepthStencilStateCreateInfo *state,
 {
    *stencilWriteEnable = state->stencilTestEnable;
 
-   /* If the depth test is disabled, we won't be writing anything. */
-   if (!state->depthTestEnable)
+   /* If the depth test is disabled, we won't be writing anything. Make sure
+    * we treat it as always passing later on as well.
+    */
+   if (!state->depthTestEnable) {
       state->depthWriteEnable = false;
+      state->depthCompareOp = VK_COMPARE_OP_ALWAYS;
+   }
 
    /* The Vulkan spec requires that if either depth or stencil is not present,
     * the pipeline is to act as if the test silently passes.

Comments

Maybe we should just roll the depthTestEnable check in with the ds_aspects
& VK_IMAGE_ASPECT_DEPTH_BIT check right below it.  In either case,

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

On Thu, Oct 25, 2018 at 5:25 AM Alex Smith <asmith@feralinteractive.com>
wrote:

> When depth testing is disabled, we shouldn't pay attention to the
> specified depthCompareOp, and just treat it as always passing. Before,
> if the depth test is disabled, but depthCompareOp is VK_COMPARE_OP_NEVER
> (e.g. from the app having zero-initialized the structure), then
> sanitize_stencil_face() would have incorrectly changed passOp to
> VK_STENCIL_OP_KEEP.
>
> Signed-off-by: Alex Smith <asmith@feralinteractive.com>
> ---
>  src/intel/vulkan/genX_pipeline.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/vulkan/genX_pipeline.c
> b/src/intel/vulkan/genX_pipeline.c
> index 33f1f7832a..877a9fb850 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -755,9 +755,13 @@
> sanitize_ds_state(VkPipelineDepthStencilStateCreateInfo *state,
>  {
>     *stencilWriteEnable = state->stencilTestEnable;
>
> -   /* If the depth test is disabled, we won't be writing anything. */
> -   if (!state->depthTestEnable)
> +   /* If the depth test is disabled, we won't be writing anything. Make
> sure
> +    * we treat it as always passing later on as well.
> +    */
> +   if (!state->depthTestEnable) {
>        state->depthWriteEnable = false;
> +      state->depthCompareOp = VK_COMPARE_OP_ALWAYS;
> +   }
>
>     /* The Vulkan spec requires that if either depth or stencil is not
> present,
>      * the pipeline is to act as if the test silently passes.
> --
> 2.14.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Good point, I didn't notice they were both now doing the same thing. Merged
them together and pushed.

On Thu, 25 Oct 2018 at 18:00, Jason Ekstrand <jason@jlekstrand.net> wrote:

> Maybe we should just roll the depthTestEnable check in with the ds_aspects
> & VK_IMAGE_ASPECT_DEPTH_BIT check right below it.  In either case,
>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
>
> On Thu, Oct 25, 2018 at 5:25 AM Alex Smith <asmith@feralinteractive.com>
> wrote:
>
>> When depth testing is disabled, we shouldn't pay attention to the
>> specified depthCompareOp, and just treat it as always passing. Before,
>> if the depth test is disabled, but depthCompareOp is VK_COMPARE_OP_NEVER
>> (e.g. from the app having zero-initialized the structure), then
>> sanitize_stencil_face() would have incorrectly changed passOp to
>> VK_STENCIL_OP_KEEP.
>>
>> Signed-off-by: Alex Smith <asmith@feralinteractive.com>
>> ---
>>  src/intel/vulkan/genX_pipeline.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/intel/vulkan/genX_pipeline.c
>> b/src/intel/vulkan/genX_pipeline.c
>> index 33f1f7832a..877a9fb850 100644
>> --- a/src/intel/vulkan/genX_pipeline.c
>> +++ b/src/intel/vulkan/genX_pipeline.c
>> @@ -755,9 +755,13 @@
>> sanitize_ds_state(VkPipelineDepthStencilStateCreateInfo *state,
>>  {
>>     *stencilWriteEnable = state->stencilTestEnable;
>>
>> -   /* If the depth test is disabled, we won't be writing anything. */
>> -   if (!state->depthTestEnable)
>> +   /* If the depth test is disabled, we won't be writing anything. Make
>> sure
>> +    * we treat it as always passing later on as well.
>> +    */
>> +   if (!state->depthTestEnable) {
>>        state->depthWriteEnable = false;
>> +      state->depthCompareOp = VK_COMPARE_OP_ALWAYS;
>> +   }
>>
>>     /* The Vulkan spec requires that if either depth or stencil is not
>> present,
>>      * the pipeline is to act as if the test silently passes.
>> --
>> 2.14.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>