[Mesa-dev] [RFC 0/2] VK_EXT_shader_stencil_export

[Gustavo Lima Chaves]

* Kenneth Graunke <kenneth at whitecape.org> [2018-02-14 01:16:01 -0800]:

> On Sunday, February 11, 2018 6:26:39 PM PST Gustavo Lima Chaves wrote:
> > I've been seeking to add this extension support on my free time and
> > have now come to a point where some input could really help.
> 
> Hi Gustavo!

Hi Kenneth!

> 
> Welcome to the Mesa community :)  Thanks for taking a shot at this!

:D

> 
> > At vtn_get_builtin_location(), is that existing one used for the
> > extension what we want? Should we also check the shader stage we're in
> > there too, once it's meant for fragment shaders?
> 
> I think your code to extend vtn_get_builtin_location looks fine.
> We'll see SpvDecorationBuiltIn, call that function, and your new
> code will handle SpvBuiltInFragStencilRefEXT, assigning it the
> location FRAG_RESULT_STENCIL, which is what we want.
> 
> IIRC, SPIR-V implementations aren't required to handle errors (unlike
> GLSL) - they leave that to the validation layers.  So, while you
> probably don't need to check the stage, it might be nice to
> 
>    assert(b->shader->info.stage == MESA_SHADER_FRAGMENT);
> 
> just to be on the safe side.

Nice. Thanks for remembering me of the rationale on error checking, I
guess I'll leave it to validation layers, then.

> 
> > The new ExecutionModeStencilRefReplacingEXT execution mode seems to
> > have no support in the code base yet. I'm still trying to figure out
> > what it should do in the meantime (the original
> > ARB_shader_stencil_export had no mentions of modes, I'm afraid).
> 
> Right.  You'll need to add support for it, as part of implementing this
> SPIR-V extension.  What exactly it should do is a good question :)
> 
> With GL_ARB_shader_stencil_export, 
> 
>     #extension GL_ARB_shader_stencil_export: enable
> 
> to a shader causes the compiler to declare a built-in output variable
> called gl_FragStencilRefARB.  If the shader statically writes to
> gl_FragStencilRefARB, then the stencil value is taken from the shader.
> Otherwise, it comes from the OpenGL API specified value.
> 
> It looks like Vulkan works a little differently.  The VK spec says:
> 
>     FragStencilRefEXT
> 
>         Decorating a variable with the FragStencilRefEXT built-in
>         decoration will make that variable contain the stencil reference
>         value for all samples covered by the fragment. This value will
>         be used as the stencil reference value used in stencil testing.
> 
>         To write to FragStencilRefEXT, a shader must declare the
>         StencilRefReplacingEXT execution mode. If a shader declares the
>         StencilRefReplacingEXT execution mode and there is an execution
>         path through the shader that does not set FragStencilRefEXT,
>         then the fragment’s stencil reference value is undefined for
>         executions of the shader that take that path.
> 
>         [...]
> 
> I believe that specifying the StencilRefReplacingEXT execution mode
> indicates "this shader will write stencil - use the shader's value
> rather than the API-specified one."  This lets the compiler know ahead
> of time, without having to search through the code to see if it's
> statically written.
> 
> It sounds like shaders are perhaps allowed to declare the
> FragStencilRefEXT built-in variable without the execution mode - they're
> just not allowed to write to it.  That seems strange and useless...
> 
> That got me thinking - what happens if you read it?  Issue 1 of the
> ARB_shader_stencil_export spec makes it clear that you can't:
> 
>     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.
> 
> (If I recall correctly, outputs in GLSL are definitely readable, so I
> think what they mean is that the initial value is undefined.  A shader
> could write a value, then read it back later...it just doesn't magically
> start with a meaningful value.)
> 
> But the Vulkan wording, "will make that variable contain the stencil
> reference value", sounds a bit scary - like it might guarantee that.
> I doubt they intended that, though.
> 
> My guess is that you'll want to extend vtn_handle_execution_mode() to
> make SpvExecutionModeStencilRefReplacingEXT set a "outputs stencil" flag
> in vtn_builder.  Then, make apply_var_decoration() do
> 
>       case SpvBuiltinFragStencilRefEXT:
>          if (!b->outputs_stencil)
>             nir_var->data.read_only = true;
>          break;
> 
> to prevent writes to the variable when the execution mode isn't set.

I find your interpretation sound enough (var read only if not in that
mode), let's go for it, then.

> 
> I'm not the most familiar with SPIR-V, so I copied Jason - he may be
> able to give a more authoritative answer.
> 
> > When it comes to testing, I started simple with
> > https://github.com/glima/VK-GL-CTS/tree/stencil_export . The idea
> > there is just to assert that a custom value is really written to a
> > FragStencilRefEXT-decorated output variable. I'm still not positive
> > that I should go the createTestsForAllStages() path there--maybe
> > something like addTessCtrlTest() for fragment tests would be more
> > reasonable. I also know there are issues in the tests, but I hope
> > anyone more familiar with vtn_get_builtin_location() will be able to
> > help sort it out.
> 
> Thanks for adding tests!  Since you can only write FragStencilRefEXT
> from the fragment stage, I'm not sure what you would test in the other
> stages.  In GLSL, you could write a negative test making sure it isn't
> possible to write gl_FragStencilRefARB from other stages...but with
> SPIR-V's lax error checking...there's probably nothing to do there.

Sure, I'll leave a FS test only, then. Yeah, I'll start with a
positive test just to be sure. That github repo has been updated to
conform with said changes and, still, there's something that is
puzzling me.

Just having an OpExecutionMode entry (fragments["debug"]) for
StencilRefReplacingEXT (that goes right after the infrastructure's
OriginUpperLeft one, thus the "debug" fragments placement) will
jeopardize the test—the final image ends up with all zeros. It has
been a pain to find the difference in the pipeline (the path is too
big!) between executions with and without that fragment on the shader
code. Is it a bug? Is my SPIR-V code buggy in any regard, still? I
guess we want to test like that, with:

 - execution mode StencilRefReplacingEXT
 - capability StencilExportEXT
 - and extension SPV_EXT_shader_stencil_export

all enabled, right?

> 
> > BTW, when said tests come to a better/final shape, do people really
> > take pull requests for VK-GL-CTS from github? All commits in there
> > seem to have Google's gerrit annotations, so I'm confused about that.
> > 
> > Thanks a lot.
> 
> Khronos uses an internal Gerrit for accepting patches from member
> companies who've already signed contributor agreements.  Being an Intel
> employee, you'll want to use that.  I believe they only use Github for
> contributions from volunteers who aren't affiliated with a company.
> 
> I'll send you a separate email with instructions on submitting CTS
> patches.

Got that, thank you so much!

> 
> --Ken



> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


-- 
Gustavo Lima Chaves
Intel - Open Source Technology Center