[Mesa-dev] [PATCH] anv: Take write mask into account in has_color_buffer_write_enabled

[Iago Toral]

Looks good to me, unless Jason or Lionel say otherwise:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Iago

On Thu, 2018-01-04 at 18:13 +0000, Alex Smith wrote:
> If we have a color attachment, but its writes are masked, this would
> have still returned true. This is inconsistent with how
> HasWriteableRT
> in 3DSTATE_PS_BLEND is set, which does take the mask into account.
> 
> This could lead to PixelShaderHasUAV not being set in
> 3DSTATE_PS_EXTRA
> if the fragment shader does use UAVs, meaning the fragment shader may
> not be invoked because HasWriteableRT is false. Specifically, this
> was
> seen to occur when the shader also enables early fragment tests: the
> fragment shader was not invoked despite passing depth/stencil.
> 
> Fix by taking the color write mask into account in this function.
> This
> is consistent with how things are done on i965.
> 
> Signed-off-by: Alex Smith <asmith at feralinteractive.com>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/intel/vulkan/genX_pipeline.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_pipeline.c
> b/src/intel/vulkan/genX_pipeline.c
> index 0ae9ead587..cfc3bea426 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -1330,7 +1330,8 @@ emit_3dstate_gs(struct anv_pipeline *pipeline)
>  }
>  
>  static bool
> -has_color_buffer_write_enabled(const struct anv_pipeline *pipeline)
> +has_color_buffer_write_enabled(const struct anv_pipeline *pipeline,
> +                               const
> VkPipelineColorBlendStateCreateInfo *blend)
>  {
>     const struct anv_shader_bin *shader_bin =
>        pipeline->shaders[MESA_SHADER_FRAGMENT];
> @@ -1339,10 +1340,15 @@ has_color_buffer_write_enabled(const struct
> anv_pipeline *pipeline)
>  
>     const struct anv_pipeline_bind_map *bind_map = &shader_bin-
> >bind_map;
>     for (int i = 0; i < bind_map->surface_count; i++) {
> -      if (bind_map->surface_to_descriptor[i].set !=
> -          ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS)
> +      struct anv_pipeline_binding *binding = &bind_map-
> >surface_to_descriptor[i];
> +
> +      if (binding->set != ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS)
>           continue;
> -      if (bind_map->surface_to_descriptor[i].index != UINT32_MAX)
> +
> +      const VkPipelineColorBlendAttachmentState *a =
> +         &blend->pAttachments[binding->index];
> +
> +      if (binding->index != UINT32_MAX && a->colorWriteMask != 0)
>           return true;
>     }
>  
> @@ -1351,6 +1357,7 @@ has_color_buffer_write_enabled(const struct
> anv_pipeline *pipeline)
>  
>  static void
>  emit_3dstate_wm(struct anv_pipeline *pipeline, struct anv_subpass
> *subpass,
> +                const VkPipelineColorBlendStateCreateInfo *blend,
>                  const VkPipelineMultisampleStateCreateInfo
> *multisample)
>  {
>     const struct brw_wm_prog_data *wm_prog_data =
> get_wm_prog_data(pipeline);
> @@ -1395,7 +1402,7 @@ emit_3dstate_wm(struct anv_pipeline *pipeline,
> struct anv_subpass *subpass,
>           if (wm.PixelShaderComputedDepthMode != PSCDEPTH_OFF ||
>               wm_prog_data->has_side_effects ||
>               wm.PixelShaderKillsPixel ||
> -             has_color_buffer_write_enabled(pipeline))
> +             has_color_buffer_write_enabled(pipeline, blend))
>              wm.ThreadDispatchEnable = true;
>  
>           if (samples > 1) {
> @@ -1520,7 +1527,8 @@ emit_3dstate_ps(struct anv_pipeline *pipeline,
>  #if GEN_GEN >= 8
>  static void
>  emit_3dstate_ps_extra(struct anv_pipeline *pipeline,
> -                      struct anv_subpass *subpass)
> +                      struct anv_subpass *subpass,
> +                      const VkPipelineColorBlendStateCreateInfo
> *blend)
>  {
>     const struct brw_wm_prog_data *wm_prog_data =
> get_wm_prog_data(pipeline);
>  
> @@ -1575,7 +1583,7 @@ emit_3dstate_ps_extra(struct anv_pipeline
> *pipeline,
>         * attachments, we need to force-enable here.
>         */
>        if ((wm_prog_data->has_side_effects || wm_prog_data-
> >uses_kill) &&
> -          !has_color_buffer_write_enabled(pipeline))
> +          !has_color_buffer_write_enabled(pipeline, blend))
>           ps.PixelShaderHasUAV = true;
>  
>  #if GEN_GEN >= 9
> @@ -1705,10 +1713,11 @@ genX(graphics_pipeline_create)(
>     emit_3dstate_hs_te_ds(pipeline, pCreateInfo->pTessellationState);
>     emit_3dstate_gs(pipeline);
>     emit_3dstate_sbe(pipeline);
> -   emit_3dstate_wm(pipeline, subpass, pCreateInfo-
> >pMultisampleState);
> +   emit_3dstate_wm(pipeline, subpass, pCreateInfo->pColorBlendState,
> +                   pCreateInfo->pMultisampleState);
>     emit_3dstate_ps(pipeline, pCreateInfo->pColorBlendState);
>  #if GEN_GEN >= 8
> -   emit_3dstate_ps_extra(pipeline, subpass);
> +   emit_3dstate_ps_extra(pipeline, subpass, pCreateInfo-
> >pColorBlendState);
>     emit_3dstate_vf_topology(pipeline);
>  #endif
>     emit_3dstate_vf_statistics(pipeline);