[Mesa-dev] [PATCH] gallium: do not automatically enable clip planes when a vertex shader writes to gl_ClipDistance

[Jose Fonseca]

On 28/09/17 16:29, Roland Scheidegger wrote:
> Am 28.09.2017 um 16:12 schrieb Jose Fonseca:
>> On 27/09/17 15:07, Roland Scheidegger wrote:
>>> Am 27.09.2017 um 09:13 schrieb Olivier Lauffenburger:
>>>> Software rasterizer and LLVM contain code to enable clipping as soon as
>>>> a vertex shader writes to gl_ClipDistance, even if the corresponding
>>>> clip planes are disabled.
>>>> GLSL specification states that "Values written into gl_ClipDistance for
>>>> planes that are not enabled have no effect."
>>>> The actual behavior is thus non-conformant.
>>>>
>>>> This patch removes the code that automagically enables user clipping
>>>> planes even if they are disabled.
>>>>
>>>> Signed-off-by: Olivier Lauffenburger <o.lauffenburger at topsolid.com>
>>>
>>> FWIW that code is there because you can't disable clip distances with
>>> d3d10 - if you write them in the shader, they're enabled (d3d9 didn't
>>> have clip distances, just old user clip planes, which of course have
>>> enable bits). They are very similar to cull distances there (which you
>>> can't disable with gl neither).
>>> I suppose we cheated there a bit... I might even have realized it wasn't
>>> quite GL conformant when we did this, but it didn't cause piglit
>>> regressions then (I guess it's very rare a shader actually declares clip
>>> distance outputs but does not enable them).
>>> This was introduced back in June 2013:
>>> https://lists.freedesktop.org/archives/mesa-dev/2013-June/040559.html
>>>
>>> So with this removed, I suppose we need to add a workaround in our code
>>> (which is indeed rather unfortunate). But I don't see another
>>> (reasonable) way to make it gl conformant.
>>> If however there's still no piglit test exercising this, there should be
>>> one.
>>
>> I'm still not following.  Are we talking about
>> pipe_rasterizer_state::clip_plane_enable controlling
>> TGSI_SEMANTIC_CLIPDIST?
> Yes.
> 
>>
>> I thought these have nothing to do with one another.
>> pipe_rasterizer_state::clip_plane_enable should control
>> legacy/fixed-fuction user clip planes.
> Nope. Every single driver (except those using draw) assumes this also
> enables clip dists - this includes svga which translates those away in
> the shader which aren't enabled.
> 
> 
>>
>> If the OpenGL state tracker needs to translate GLSL shaders that write
>> gl_ClipDistance but where the clip plane is disabled, the solution is
>> simple: just create a shader variant that omits the
>> TGSI_SEMANTIC_CLIPDIST in question, or writes an constant to it.
> Well, it is easier to have an extra enable and having to add additional
> rasterizer dependencies on state trackers which don't have separate
> enable, rather than having to hack shaders with state trackers which
> don't have them. Of course, svga does exactly that but it's a bit annoying.
> 
>>
>> Like it was mentioned, it should be extremely rare for apps to emit
>> gl_ClipDistance yet disable the clip planes, hence the shader variants
>> should be extremely rare.
>>
>> It's not just D3D10/11 that doesn't have flags to disable clip
>> distances: I can't find them in Vulkan spec neither.  And while I know
>> few/none want to build Vulkan drivers atop Gallium interface I think
>> it's still useful to decide what deserves to be in Gallium interface or
>> not.
> Of course, newer apis won't have that - clearly the separate enables just
> carried over from legacy GL.
> 
>> So in short, llvmpipe is fine, please let's fix the state tracker instead.
> Well, we're really the only ones caring about non-gl gallium state
> tracker, so I'm not sure it makes sense to impose the d3d10 semantics
> there. We just cheated in draw.
> And actually, thinking about this, it's really not even possible easily
> and cleanly doing this in the state tracker: you can pass those clip
> dists to the next shader stage. If that's all pre-fragment stage
> (vertex, geometry, tesselation), this is still doable just annoying
> (must pass them unaltered between stages not the last stage before
> rasterization), but it's impossible to pass them to fragment stage
> (unless you'd use generic varying) if you rewrite the shader to not
> include them (don't ask me if it works correctly in svga, it might have
> code for this too).
> 
> So I believe there's really no other choice other than following GL
> semantics there in gallium.

Ok..

So when implementing D3D10 we just need to ensure clip enable is always 
set to all ones?

Jose