[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 18:19 schrieb Jose Fonseca:
> On 28/09/17 17:16, Roland Scheidegger wrote:
>> 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...
> 
> I wouldn't call this "cheat", but merely do something sane, something
> that's not merely undefined, something that works for all APIs.
Well, basically it amounts to using a undefined shader output, and such
things tend to give undefined results.

> 
> After all, when did gallium stop to be an abstraction to become
> "whatever GL api does"?
Yes, we could do it differently.
Actually, there's a comment in p_state.h which even states this is
indeed the case.
"   /**
    * Enable bits for clipping half-spaces.
    * This applies to both user clip planes and shader clip distances.
    * Note that if the bound shader exports any clip distances, these
    * replace all user clip planes, and clip half-spaces enabled here
    * but not written by the shader count as disabled.
    */
   unsigned clip_plane_enable:PIPE_MAX_CLIP_PLANES;
"
However, I was wrong saying this could work. I realized it cannot - this
is because there's no explicit switch between old user clip planes and
clip distances. If the shader writes (at least one) clip distance, then
those clip distances will be used as additional clip planes (when the
clip enable bits are set). However, if clip dist output does not exist,
then that means old user clip planes are enabled instead and the
corresponding clip math performed (using the user clip planes and either
ordinary position output or clipVertex output).
Therefore, always setting all clip enable bits to 1 and not writing clip
dist at all would perform user clip plane clipping... Would be fixable
with another rasterizer bit I suppose but I'm not sure it's worth it?

Roland