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

[Roland Scheidegger]

Am 28.09.2017 um 17:53 schrieb 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?
I don't think that will work - the (gl and presumably gallium) semantics
are that enabled clip planes must be written or the results are
undefined. I suppose we could cheat there too (in draw) and take the
insersection of enabled and written clip dists, but otherwise we'd have
to enable only these clip planes a shader actually writes...

Roland