Re: [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 

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

> Looks good to me too.
>
> Reviewed-by: Lionel Landwerlin 
>
> 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 
>>
>> Iago
>>
>> On Thu, 2018-01-04 at 18:13 +, 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 
>>> Cc: mesa-sta...@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 = _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 = _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 =
>>> + >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 

Re: [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 

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 

Iago

On Thu, 2018-01-04 at 18:13 +, 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 
Cc: mesa-sta...@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 = _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 = _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 =
+ >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);

___

Re: [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 

Iago

On Thu, 2018-01-04 at 18:13 +, 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 
> Cc: mesa-sta...@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 = _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 = _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 =
> + >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] [PATCH] anv: Take write mask into account in has_color_buffer_write_enabled

[Alex Smith]

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 
Cc: mesa-sta...@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 = _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 = 
_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 =
+ >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);
-- 
2.13.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev