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

[Lionel Landwerlin]

Looks good to me too.

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

Thanks a lot!

-
Lionel

On 05/01/18 09:19, Iago Toral wrote:
> 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);
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev