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

[Ilia Mirkin]

On Tue, Oct 3, 2017 at 4:25 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 03.10.2017 09:54, Olivier Lauffenburger wrote:
>>>
>>> On 29/09/17 12:17, Nicolai Hähnle wrote:
>>>>
>>>> On 28.09.2017 20:02, Roland Scheidegger wrote:
>>>>>
>>>>> 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/04055
>>>>>>>>>>> 9.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?
>>>>
>>>>
>>>> If you do care sufficiently, IMO it'd be cleaner to have separate bit
>>>> sets for user clip planes and clip distances. It's more bits, but I
>>>> think the disentangling of state would be worth it.
>>>
>>>
>>> That sounds a good idea to me too IMHO, as long it's not too much hassle.
>>>
>>> Jose
>>
>>
>> How is it that gl_ClipVertex does work? The clip planes enable bits are
>> used to compute and fill the clip distance attributes after the vertex
>> shader has been executed.
>> As seen from afar, there is no real difference between this behavior and
>> the fact to take the clip plane enable bits into account when doing
>> clipping
>> by user planes.
>
>
> This logic may be why the clip bits were aliased in Gallium in the first
> place, and perhaps some hardware works like this, adding the clip distance
> generation code to the vertex shader when legacy gl_ClipVertex is used.
>
> However, at least in Radeon hardware those states are completely orthogonal:
> there are six user clip planes which have their clip enable bits, and
> separately there are eight cull/clip distance outputs, each of which can
> separately be enabled for cull/clip. So the hardware has 6 + 8 + 8 clip/cull
> enable bits.

All NVIDIA hardware works like this -- starting with NV30. NV30 has 6
clip distances, NV50 (and later) have 8. Each distance can be
enabled/disabled, and on NV50+ you can also control whether it clips
or culls.

Adreno A3xx (and A2xx?) also supports up to 6 user clip planes, but
also with no clip vertex support. A4xx+ is all software clipping.

BTW - GL 3.0 requires 8 user clip planes.

Cheers,

  -ilia