Bug 57875

Created attachment 70995 [details]
The "blue triangles"

Does reverting the commit e866bd1adea2c3b4971ad68e69c644752f2ab7b6 help?

Yes, reverting commit e866bd1adea2c3b4971ad68e69c644752f2ab7b6 works.

I'll have a look at it once I find a download for this app.

http://sldev.free.fr/

The stable branch Linux: CoolVLViewer-1.26.4.XX works with r300,
the  CoolVLViewer-1.26.5.XX doesn't  because it derives from Second Life official viewer series 3 and so suffers from the aforementioned bug 39251.

I can reproduce the bug, and after a quick investigation it seems to be a legitimate problem how depth clamping is handled. The application enables it, it enables it only if the extension is available and if I don't set the GPUs CLIP_DISABLE flag but keep the extension the bugs go away(so having the extension doesn't trigger a different codepath that is broken in some other way).

Marek, in 09109c11d9efd78c0f87fc55911e03eda5fd980b you mention GB_TILE_CONFIG.Z_EXTENDED. From looking at the code of this application and the comments therein, I suspect the clamping part of GL_ARB_depth_clamp is required here. Do you have some code left over from your previous DEPTH_CLAMP experiments?

Aw, of course thr R3xx_3D_Registers.pdf document doesn't document bit 24 of R300_GB_TILE_CONFIG, which supposedly contains the Z_EXTENDED flag...

(In reply to comment #7)
> Aw, of course thr R3xx_3D_Registers.pdf document doesn't document bit 24 of
> R300_GB_TILE_CONFIG, which supposedly contains the Z_EXTENDED flag...

That bit only exists on r5xx asics.  R3xx and r4xx don't have that bit.

I don't remember having anything useful besides some quick hacks and I don't have them anymore. I came to the conclusion ARB_depth_clamp is not implementable on r300-r500. I don't think the extended range [-2,2] would help and IIRC, the register is not allowed to be written by userspace anyway, it must be written in the kernel.

The way I see it, there are 4 solutions to this issue:

1) Wine shouldn't use ARB_depth_clamp, but instead it should use an extension that exposes CLIP_DISABLE as defined by D3D9 to the user. The problem is such an extension doesn't exist.

2) We could agree on a wine-specific hack for r300g, which would expose ARB_depth_clamp for Wine only. We already blacklist certain apps for Hyper-Z, this would be no different.

3) We could clamp POS.Z to the range [-POS.W, POS.W] in the vertex shader. The problem with this approach is the clamping should be per-pixel, not per-vertex.

4) Disabling ARB_depth_clamp again.

(In reply to comment #8)
> That bit only exists on r5xx asics.  R3xx and r4xx don't have that bit.
I'd say it's related to floating point depth buffer support. I don't know about the capabilities of the GPUs though and have to read up the interactions between depth_clamp and depth_buffer_float again, but I'd expect the limited depth buffer range to clamp the values.

(In reply to comment #9)
> I don't remember having anything useful besides some quick hacks and I don't
> have them anymore.
I've written a kernel hack, now the broken geometry is mostly behind the correct parts. I guess this is where you stopped last time.

Before I give up on this I'd like to investigate some more things, particularly the interaction with glDepthRange(something similar to 92e7c6a2581b5f612a84587500399bb00318c6f0) and the interactions with and the interaction with the depth buffer format, maybe something like 73856817973caab283427c52152672f524c49a07 is needed. Furthermore I'd like to find out if this application uses a floating point depth buffer.

> 1) Wine shouldn't use ARB_depth_clamp, but instead it should use an
> extension that exposes CLIP_DISABLE as defined by D3D9 to the user. The
> problem is such an extension doesn't exist.
You could do something like MESA_depth_clamp_hack which uses the entrypoints and enums of ARB_depth_clamp but only has an effect with GL_DEPTH_TEST off.

> 2) We could agree on a wine-specific hack for r300g, which would expose
> ARB_depth_clamp for Wine only. We already blacklist certain apps for
> Hyper-Z, this would be no different.
Sounds ugly. I'd like to invest a few more days before I start thinking about that route.

> 3) We could clamp POS.Z to the range [-POS.W, POS.W] in the vertex shader.
> The problem with this approach is the clamping should be per-pixel, not
> per-vertex.
I've been thinking about doing this in Wine before NV_depth_clamp became ARB_depth_clamp. Claming the Z value in the fragment shader would be easier, but I think won't work as the geometry would be clipped before it reaches the fragment stage.

I found the r500 register docs, and yeah, the Z_EXTENDED bit sounds less useful now...

(In reply to comment #9)
> 1) Wine shouldn't use ARB_depth_clamp, but instead it should use an
> extension that exposes CLIP_DISABLE as defined by D3D9 to the user. The
> problem is such an extension doesn't exist.
> 
If ARB_depth_clamp can't be made to work on r300g, that would be my preferred solution. There may even be some value in that for a hypothetical D3D or wined3d state tracker, although I still don't see that happening in the short term, for various reasons.

I think a new extension is the way to go. We could probably call it GL_MESA_depth_clip_disable or GL_MESA_depth_clamp_d3d.

One could try to combine the CLIP_DISABLE register with clamping gl_FragDepth inside the fragment shader. I'd expect the cost of this to be way too high, even if it's just enabled when both GL_DEPTH_TEST and GL_DEPTH_CLAMP are enabled. Maybe some considerations similar to GL_ARB_conservative_depth can be applied, but I guess that'd require hardware support.

(In reply to comment #10)
> > That bit only exists on r5xx asics.  R3xx and r4xx don't have that bit.
> I'd say it's related to floating point depth buffer support. I don't know
> about the capabilities of the GPUs though and have to read up the
> interactions between depth_clamp and depth_buffer_float again, but I'd
> expect the limited depth buffer range to clamp the values.

There's no floating-point depth buffer support in r500.


> > I don't remember having anything useful besides some quick hacks and I don't
> > have them anymore.
> I've written a kernel hack, now the broken geometry is mostly behind the
> correct parts. I guess this is where you stopped last time.
> 
> Before I give up on this I'd like to investigate some more things,
> particularly the interaction with glDepthRange(something similar to
> 92e7c6a2581b5f612a84587500399bb00318c6f0) and the interactions with and the

92e7c6a258 fixes a piglit test which passes with r300g, so it's most probably unrelated.

glDepthRange is packed in the Z component of the vectors in pipe_viewport_state, which is applied after clipping and converts coordinates from the clip space to the window space.

Created attachment 71244 [details]
proposed MESA_clip_disable extension

I've written an extension proposal for a new mesa extension that exposes the CLIP_DISABLE bit. The proposal is attached, please review and comment. There are some TODOs I'm not quite sure how to deal with, see the list in "Status".

We can move the extension discussion elsewhere if this bug report isn't the proper place for it.

(In reply to comment #15)
> I've written an extension proposal for a new mesa extension that exposes the
> CLIP_DISABLE bit. The proposal is attached, please review and comment. There
> are some TODOs I'm not quite sure how to deal with, see the list in "Status".
> 
  - It probably makes sense to mention "depth" in the extension name somewhere, we really only care about the near and far clipping planes. E.g. MESA_depth_clip.
  - Perhaps it makes more sense to write the extension in terms of ARB_depth_clamp. I.e., provide a subset of ARB_depth_clamp, that only disables depth clipping, and generates undefined results in the depth test, except for perhaps GL_ALWAYS / GL_NEVER. (Also, note that you can't really make fragment depth values undefined before / in the fragment shader, because you have interactions with e.g. texture filtering as well.) It would avoid the perhaps somewhat questionable "ARB_depth_clamp must not be supported", and you can just defer to ARB_depth_clamp for most of the issues etc. The interaction between the two extensions just becomes that if ARB_depth_clamp is also supported you get defined depth test results.

Created attachment 71407 [details]
MESA_depth_clip extension spec, try 2.

Thanks for the comments, here is an updated version.

If i try to revert the commit e866bd1adea2c3b4971ad68e69c644752f2ab7b6  (above mentioned workaround) with git
"HEAD at 7c35521 mesa: add missing texel fetch code for sRGB DXT formats"
the compilation ends with:
 ...
CC     r300_screen.o
r300_screen.c: In function ‘r300_get_param’:
r300_screen.c:130:14: error: ‘PIPE_CAP_TIMER_QUERY’ undeclared (first use in this function)
r300_screen.c:130:14: note: each undeclared identifier is reported only once for each function it appears in
r300_screen.c:88:5: warning: enumeration value ‘PIPE_CAP_QUERY_TIME_ELAPSED’ not handled in switch [-Wswitch]
r300_screen.c:88:5: warning: enumeration value ‘PIPE_CAP_TEXTURE_BUFFER_OBJECTS’ not handled in switch [-Wswitch]
gmake[4]: *** [r300_screen.o] Error 1
gmake[4]: Leaving directory `/root/mesa/src/gallium/drivers/r300'
gmake[3]: *** [all-recursive] Error 1

 ...

Are there any comments on the 2nd extension spec? If it looks good to you I'll do some tests with the r200 and r500 GPUs I have to see how unclipped Z values behave in fragment processing to make sure the extension doesn't guarantee anything the HW cannot provide.

FWIW, the extension specification looks good to me.

With last git-6eb0d3d and git-9bdf5be the bug disappeared, so I mark it as Resolved.

Are you sure? I see nothing in the r300g git history that I'd expect to have fixed this bug. Unless of course the hw does what ARB_depth_clamp requires when you set CLIP_DISABLE and the rendering problems had nothing to do with depth_clamp in the first place.

If the bug is indeed fixed a bisect for the commit fixing it might be interesting.

Bisecting is  difficult because almost every day I compile from git but i change mesa/drm and xorg/driver/xf86-video-ati in ensemble...
Plus  I compiled on Fedora 18 with 3.7.x-xx kernel that carries power management improvements and other work for the Radeon driver.

To get rid of bug I reverted the commits:

r300_screen.c

- case PIPE_CAP_DEPTH_CLIP_DISABLE: /* XXX implemented, but breaks Regnum Online */
return 1;
...
case PIPE_CAP_INDEP_BLEND_FUNC:
+ case PIPE_CAP_DEPTH_CLIP_DISABLE:

------------------

r300_state.c

- R300_PS_UCP_MODE_CLIP_AS_TRIFAN |
- (state->depth_clip ? 0 : R300_CLIP_DISABLE);

+ R300_PS_UCP_MODE_CLIP_AS_TRIFAN;

----------------- 

now i dont need anymore this way and the version of Second Life viewer is the same: CoolVLViewer-1.26.4.48-Linux

Another path of investigation would be the anti-aliasing performance optimizations By Marek Olsek.
Anyway I will try to help within the limits of possibility.

P.S.

System Software / Hardware Information

Hardware:
Processor: Intel Core Duo T2350 @ 1.86GHz (2 Cores),Graphics: AMD Mobility Radeon X1700 M66 RV535 256MB 

Software:
OS: Fedora 18 (Spherical Cow), Kernel: 3.7.2-201.fc18.i686 (i686), Desktop: KDE 4.9.5, Display Server: X Server 1.13.1, Display Driver: radeon 7.0.99, OpenGL: 2.1 Mesa 9.1-devel (git-9bdf5be) Gallium 0.4, Compiler: GCC 4.7.2 20121109 + LLVM 3.1, File-System: ext4

 Stefan Dösinger you are right: the bug didn't disappear; for some reasons the change of environment for mesa directory doesn't work on FC18 simply with

"/etc/ld.so.conf.d/a-local-xorg.conf (new file) or add to begin of /etc/ld.so.conf if directory doesn't exists:
/usr/local/lib

and to /etc/environment  to add new environment variable that tells libgl where to look for dri drivers (example r300_dri.so):
LIBGL_DRIVERS_PATH=/usr/local/lib/dri/"

so I had  to link /usr/local/lib/dri/r300_dri.so to /usr/lib/dri/r300_dri.so

Sorry for the  inconvenience.

Stefan> We can also go the easy way and only advertise ARB_depth_clamp if the user is Wine. It would work in the same way we disable HyperZ for compositors. I'm assuming Wine can be detected as easily as reading program_invocation_short_name.

This is a bad idea because Wine can also run OpenGL applications, which might use depth_clamp in a way that doesn't work on r300g.

Feel free to revert the patch for now. Implementing MESA_depth_clip is fairly high up on my todo list, but even higher priorities kept interfering.

If you have the time and resources to implement my extension proposal yourself that'd make me extra-happy of course :-)

Stefan, is the extension for implementing the POSITIONT shader semantic? Would a gl_Position output modifier also work for you?

For example:

#extension GL_MESA_xxx : require

pretransformed gl_Position;

void main()
{
...
etc.

Hmm, this seems like an idea worth thinking about. The D3D behavior the proposed extension addresses is part of the D3DDECLUSAGE_POSITIONT / D3DFVF_XYZRHW vertex input semantics.

For now I'm opposed to making this a vertex shader control though. The point of POSITIONT / XYZRHW is to skip vertex processing altogether, so handling POSITIONT semantics in a shader seems a bit off. Furthermore, the clipping behavior of POSITIONT depends on the depth test, so this needs a separate control anyway (or replicating the depth test interaction, which seems somewhat ugly to me because its done by a different stage in the rendering pipeline).

How does it depend on the depth test, and why does it seem "off" to make something that is originally a shader property in D3D a shader property in OpenGL as well ?
Not using shaders isn't allowed (at least not in gallium), so you have to use a pass-through shader anyway, so this limits state inter-dependency.

The qualifier "layout(windowspace)/layout(pretransformed) out vec4 gl_Position" is supposed to disable clipping and the viewport transformation (so glViewport settings will be ignored).

The only open question is how depth values should be treated, which can be either clipped or clamped, and there is actually a D3DCAPS9 bit to tell the user what will happen.
On r600, you cannot disable xy clipping without disabling z clipping, so it will have to advertise clamping. On NV cards you have more fine-grained control, and I'd actually prefer depth clipping there unless GL_DEPTH_CLAMP is enabled as well.

You're mixing things up a bit, the functionality this bug is about is mostly controlled by D3DRS_ZENABLE in d3d9. Considering only pre-transformed (D3DFVF_XYZRHW) vertices, the behaviour is like this:
  - When D3DRS_ZENABLE is D3DZB_TRUE, Z values are clipped when D3DPMISCCAPS_CLIPTLVERTS is set, clamped when it isn't. Depth testing works pretty much as expected. In practice that means the default GL behaviour is fine as long as we set D3DPMISCCAPS_CLIPTLVERTS.
  - When D3DRS_ZENABLE is D3DZB_FALSE, depth value processing is technically just completely disabled. We get equivalent behaviour by disabling the depth test, and clamping depth values.

This does mean that always clamping Z for pre-transformed vertices through an output modifier could be a valid implementation, as long as we don't set D3DPMISCCAPS_CLIPTLVERTS in wined3d. However, it also means that you can't then require the depth test to be always disabled when that output modifier is used. If I'm still understanding the bug correctly, the whole point was that enabling both depth clamping and the depth test at the same time is problematic on r300.

You could of course specify an appropriate interaction with GL_DEPTH_TEST in the extension spec instead of just "always clamp Z / disable clipping and viewport transform". That would probably work for us, but would also probably make for a more complicated extension. It would also make it impossible to ever implement this on things like r200 that can't properly do GLSL, although that may not be enough of a concern to avoid doing it this way. What would be the main advantage from r300g's point of view of making this an output modifier?

There would be no advantage for r300g.

The problem with r300g is that it cannot do depth clamping without disabling the clipping entirely. There is only one big switch called CLIP_DISABLE, which "Disables clip code generation and clipping process for TCL" and is enough to pass the ARB_depth_clamp piglit tests. However it has caused issues like thousands of random/misplaced triangles on the screen. I think the lack of XY clipping is the problem and the switch was probably only meant to be used with software vertex processing.

I reverted the problematic commit: http://cgit.freedesktop.org/mesa/mesa/commit/?id=4dfe1a0df56d084b6a29fe423afe0535abec29e9

Closing.