<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 23, 2018 at 1:40 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Thu, 2018-02-22 at 11:22 -0800, Gustavo Lima Chaves wrote: > v2: > An attempt to support SpvExecutionModeStencilRefReplacingEXT's > behavior > also follows, with the interpretation to said mode being we prevent > writes to the built-in FragStencilRefEXT variable when the execution > mode isn't set. > > v3: > A more cautious reading of 1db44252d01bf7539452ccc2b5210c74b8dcd573 > led > me to a missing change that would stop (what I later discovered were) > GPU hangs on the CTS test written to exercize this. > --- > src/compiler/shader_info.h | 2 ++ > src/compiler/spirv/spirv_to_nir.c | 4 ++++ > src/compiler/spirv/vtn_variables.c | 4 ++++ > src/intel/vulkan/anv_extensions.py | 2 ++ > src/intel/vulkan/anv_pipeline.c | 1 + > src/intel/vulkan/genX_pipeline.c | 1 + > 6 files changed, 14 insertions(+) > > diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h > index 6de707f672..f99cbc27a7 100644 > --- a/src/compiler/shader_info.h > +++ b/src/compiler/shader_info.h > @@ -162,6 +162,8 @@ typedef struct shader_info { > > bool pixel_center_integer; > > + bool outputs_stencil; > + > /** gl_FragDepth layout for ARB_conservative_depth. */ > enum gl_frag_depth_layout depth_layout; > } fs; > diff --git a/src/compiler/spirv/spirv_to_nir.c > b/src/compiler/spirv/spirv_to_nir.c > index e00dcafa12..dcb8b31967 100644 > --- a/src/compiler/spirv/spirv_to_nir.c > +++ b/src/compiler/spirv/spirv_to_nir.c > @@ -3395,6 +3395,10 @@ vtn_handle_execution_mode(struct vtn_builder > *b, struct vtn_value *entry_point, > case SpvExecutionModeContractionOff: > break; /* OpenCL */ > > + case SpvExecutionModeStencilRefReplacingEXT: > + b->shader->info.fs.outputs_stencil = true; > + break; > + > default: > vtn_fail("Unhandled execution mode"); > } > diff --git a/src/compiler/spirv/vtn_variables.c > b/src/compiler/spirv/vtn_variables.c > index 36976798e9..42f915d434 100644 > --- a/src/compiler/spirv/vtn_variables.c > +++ b/src/compiler/spirv/vtn_variables.c > @@ -1373,6 +1373,10 @@ apply_var_decoration(struct vtn_builder *b, > nir_variable *nir_var, > case SpvBuiltInFragCoord: > nir_var->data.pixel_center_integer = b- > >pixel_center_integer; > break; > + case SpvBuiltInFragStencilRefEXT: > + if (!b->shader->info.fs.outputs_stencil) > + nir_var->data.read_only = true; > + break; </div></div>From the SPIR-V spec: FragStencilRefEXT must only decorate output variable whose type is an arbitrary-sized integer type scalar. If it is an output variable, I don't think we should make it read-only in any case. </blockquote><div> </div><div>Thanks for doing the spec archeology. I suspected that was the case. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> More over, from ARB_shader_stencil_export (which is the base GL extension this is trying to replicate): "1) Should gl_FragStencilRefARB be initialized to the current stencil reference value on input to the fragment shader? RESOLVED: No. gl_FragStencilRefARB is write-only. If the current stencil reference value is required in a shader, the application should place it in a uniform." In other words, this is an output-only variable and it is not expected to be read. If we see a SpvBuiltInFragStencilRefEXT decoration then my understanding is that we should also have SpvExecutionModeStencilRefReplacingEXT, so if we didn't have that I think we have incorrect SPIR-V, and in that case, instead of marking the variable as read-only, we should maybe just drop a warning instead. <div class="gmail-HOEnZb"><div class="gmail-h5"></div></div></blockquote><div> </div><div>Thanks for checking. I've filed an issue against the Vulkan spec for this: <a href="https://gitlab.khronos.org/vulkan/vulkan/issues/1173">https://gitlab.khronos.org/vulkan/vulkan/issues/1173</a> </div>For now, let's proceed under the assumption that the Vulkan extension follows the GL extension and does not require gl_FragStencilRefARB to be initialized with the API stencil reference value. </div><div class="gmail_quote">--Jason </div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"> > default: > break; > } > diff --git a/src/intel/vulkan/anv_extensions.py > b/src/intel/vulkan/anv_extensions.py > index 581921e62a..cd90c6ae52 100644 > --- a/src/intel/vulkan/anv_extensions.py > +++ b/src/intel/vulkan/anv_extensions.py > @@ -86,6 +86,8 @@ EXTENSIONS = [ > Extension('VK_KHX_multiview', 1, True), > Extension('VK_EXT_debug_report', 8, True), > Extension('VK_EXT_external_memory_dma_buf', 1, True), > + Extension('VK_EXT_shader_stencil_export', 1, > + 'device->info.gen >= 9'), > ] > > class VkVersion: > diff --git a/src/intel/vulkan/anv_pipeline.c > b/src/intel/vulkan/anv_pipeline.c > index e16a7a1994..ed63fa42cd 100644 > --- a/src/intel/vulkan/anv_pipeline.c > +++ b/src/intel/vulkan/anv_pipeline.c > @@ -143,6 +143,7 @@ anv_shader_compile_to_nir(struct anv_pipeline > *pipeline, > .multiview = true, > .variable_pointers = true, > .storage_16bit = device->instance->physicalDevice.info.gen > >= 8, > + .stencil_export = device->instance->physicalDevice.info.gen > >= 9, > }, > }; > > diff --git a/src/intel/vulkan/genX_pipeline.c > b/src/intel/vulkan/genX_pipeline.c > index 89cbe293b8..683a4607e6 100644 > --- a/src/intel/vulkan/genX_pipeline.c > +++ b/src/intel/vulkan/genX_pipeline.c > @@ -1600,6 +1600,7 @@ emit_3dstate_ps_extra(struct anv_pipeline > *pipeline, > ps.PixelShaderHasUAV = true; > > #if GEN_GEN >= 9 > + ps.PixelShaderComputesStencil = wm_prog_data- > >computed_stencil; > ps.PixelShaderPullsBary = wm_prog_data->pulls_bary; > ps.InputCoverageMaskState = wm_prog_data->uses_sample_mask ? > ICMS_INNER_CONSERVATIVE : > ICMS_NONE; </div></div><div class="gmail-HOEnZb"><div class="gmail-h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>