Seems reasonable. Only one comment which you can feel free to ignore.
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
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 <lionel.g.landwer...@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 <ito...@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 <asm...@feralinteractive.com>
>>> 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 = &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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
