[Mesa-dev] [PATCH V2 10/11] genX/cmd_buffer: Enable fast depth clears

[Chad Versace]

On Mon 26 Sep 2016, Nanley Chery wrote:
> From: Nanley Chery <nanleychery at gmail.com>
> 
> Provides an FPS increase of ~30% on the Sascha triangle and multisampling
> demos.
> 
> Clears that happen within a render pass via vkCmdClearAttachments are safe
> even if the clear color changes. This is because the meta implementation does
> not use LOAD_OP_CLEAR which avoids any conflicts with 3DSTATE_CLEAR_PARAMS.
> 
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> 
> ---
> 
> v2. Update granularity comment for accuracy
> 
>  src/intel/vulkan/anv_pass.c        | 13 +++++++++++++
>  src/intel/vulkan/gen8_cmd_buffer.c |  6 ++++++
>  src/intel/vulkan/genX_cmd_buffer.c |  4 +---
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_pass.c b/src/intel/vulkan/anv_pass.c
> index 69c3c7e..595c2ea 100644
> --- a/src/intel/vulkan/anv_pass.c
> +++ b/src/intel/vulkan/anv_pass.c
> @@ -155,5 +155,18 @@ void anv_GetRenderAreaGranularity(
>      VkRenderPass                                renderPass,
>      VkExtent2D*                                 pGranularity)
>  {
> +   ANV_FROM_HANDLE(anv_render_pass, pass, renderPass);
> +
> +   /* This granularity satisfies HiZ fast clear alignment requirements
> +    * for all sample counts.
> +    */
> +   for (unsigned i = 0; i < pass->subpass_count; ++i) {
> +      if (pass->subpasses[i].depth_stencil_attachment !=
> +          VK_ATTACHMENT_UNUSED) {
> +         *pGranularity = (VkExtent2D) { .width = 8, .height = 4 };
> +         return;
> +      }
> +   }
> +
>     *pGranularity = (VkExtent2D) { 1, 1 };
>  }
> diff --git a/src/intel/vulkan/gen8_cmd_buffer.c b/src/intel/vulkan/gen8_cmd_buffer.c
> index 14e6a7b..96e972c 100644
> --- a/src/intel/vulkan/gen8_cmd_buffer.c
> +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> @@ -479,6 +479,12 @@ genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer,
>               cmd_state->render_area.extent.height % px_dim.h)
>              return;
>        }
> +
> +      anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_CLEAR_PARAMS), cp) {
> +         cp.DepthClearValueValid = true;
> +         cp.DepthClearValue =
> +            cmd_buffer->state.attachments[ds].clear_value.depthStencil.depth;
> +      }
>        break;
>     case BLORP_HIZ_OP_DEPTH_RESOLVE:
>        if (cmd_buffer->state.pass->attachments[ds].store_op !=
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> index 2cb1539..290fefc 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1320,9 +1320,6 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer)
>     } else {
>        anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_STENCIL_BUFFER), sb);
>     }
> -
> -   /* Clear the clear params. */
> -   anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_CLEAR_PARAMS), cp);

We may need to preserve emission of 3DSTATE_CLEAR_PARAMS here. Two reasons:

    Reason 1. If hiz is enabled in the 3DSTATE_DEPTH_BUFFER, and the hiz
       surface has some bits in the clear state, and 3DSTATE_CLEAR_PARAMS.DepthClearValueValid is 0,
       and we emit a draw call, what does the hardware do when it
       accesses a cleard pixel? I don't want to find out.

    Reason 2. The PRM says we have to (though, to be honest, I don't trust the PRM's logic).

        From the Skylake PRM >> Vol7: 3D-Media-GPGUP >> Section: Hierarchical Depth Buffer:
        | 
        |  If HiZ is enabled, you must initialize the clear value by either:
        | 
        |     1. Perform a depth clear pass to initialize the clear value.
        |     2. Send a 3dstate_clear_params packet with valid = 1.
        | 
        |  Without one of these events, context switching will fail, as it will try
        |  to save off a clear value even though no valid clear value has been set.
        |  When context restore happens, HW will restore an uninitialized clear value.
    
        Even though the hardware docs claim we need 3DSTATE_CLEAR_PARAMS when hiz is
        enabled, the docs are vague about the consequences. Does context switching
        really fail, as claimed by #1? Or does context switching actually succeed, but
        context restore gives us an invalid clear value (which doesn't hurt us), as
        claimed by #2? Oh hw docs... :/

As a consequence of that reasoning, we should set 3DSTATE_CLEAR_PARAMS.DepthClearValueValid = 1 
whenever hiz is enabled, even if we don't care about the actual clear value.