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

[Jason Ekstrand]

Seems reasonable.  Only one comment which you can feel free to ignore.

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

On Fri, Jan 5, 2018 at 7:23 AM, Lionel Landwerlin <
lionel.g.landwerlin at intel.com> wrote:

> 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)
>>>
>>
At some point, we really should add a new #define for this and stop using
UINT32_MAX.


>            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
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20180105/4ee7da7e/attachment.html>