https://bugs.freedesktop.org/show_bug.cgi?id=57875
Priority: medium Bug ID: 57875 Assignee: dri-devel@lists.freedesktop.org Summary: Second Life viewer bad rendering with git-ec83535 Severity: normal Classification: Unclassified OS: All Reporter: andabata12@yahoo.it Hardware: Other Status: NEW Version: unspecified Component: Drivers/Gallium/r300 Product: Mesa
With git "HEAD at ec83535 automake/gallium: attempt to fix -lrt", Cool VL Viewer a TPV for Second Life 3D world ( the official Linden Lab viewer is from ages blocked cause of https://bugs.freedesktop.org/show_bug.cgi?id=39251 ) can set only basic shaders otherwise with atmospheric, water and reflections shaders enabled, I have extensive blue triangles during the execution. On the contrary with graphics drivers from git-54ff536 ( HyperZ enabled by default) all was right with smooth rendering, lowered cpu usage and augmented framerate ( 8-10 %).
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #1 from Piero Finizio andabata12@yahoo.it --- Created attachment 70995 --> https://bugs.freedesktop.org/attachment.cgi?id=70995&action=edit The "blue triangles"
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #2 from Marek Olšák maraeo@gmail.com --- Does reverting the commit e866bd1adea2c3b4971ad68e69c644752f2ab7b6 help?
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #3 from Piero Finizio andabata12@yahoo.it --- Yes, reverting commit e866bd1adea2c3b4971ad68e69c644752f2ab7b6 works.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #4 from Stefan Dösinger stefandoesinger@gmx.at --- I'll have a look at it once I find a download for this app.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #5 from Piero Finizio andabata12@yahoo.it --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #6 from Stefan Dösinger stefandoesinger@gmx.at --- 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?
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #7 from Stefan Dösinger stefandoesinger@gmx.at --- 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...
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #8 from Alex Deucher agd5f@yahoo.com --- (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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #9 from Marek Olšák maraeo@gmail.com --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #10 from Stefan Dösinger stefandoesinger@gmx.at --- (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.
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.
Sounds ugly. I'd like to invest a few more days before I start thinking about that route.
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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #11 from Stefan Dösinger stefandoesinger@gmx.at --- I found the r500 register docs, and yeah, the Z_EXTENDED bit sounds less useful now...
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #12 from Henri Verbeet hverbeet@gmail.com --- (In reply to comment #9)
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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #13 from Stefan Dösinger stefandoesinger@gmx.at --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #14 from Marek Olšák maraeo@gmail.com --- (In reply to comment #10)
There's no floating-point depth buffer support in r500.
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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #15 from Stefan Dösinger stefandoesinger@gmx.at --- Created attachment 71244 --> https://bugs.freedesktop.org/attachment.cgi?id=71244&action=edit 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #16 from Henri Verbeet hverbeet@gmail.com --- (In reply to comment #15)
- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
Stefan Dösinger stefandoesinger@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #71244|0 |1 is obsolete| |
--- Comment #17 from Stefan Dösinger stefandoesinger@gmx.at --- Created attachment 71407 --> https://bugs.freedesktop.org/attachment.cgi?id=71407&action=edit MESA_depth_clip extension spec, try 2.
Thanks for the comments, here is an updated version.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #18 from Piero Finizio andabata12@yahoo.it --- 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
...
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #19 from Stefan Dösinger stefandoesinger@gmx.at --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #20 from Marek Olšák maraeo@gmail.com --- FWIW, the extension specification looks good to me.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
Piero Finizio andabata12@yahoo.it changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |FIXED
--- Comment #21 from Piero Finizio andabata12@yahoo.it --- With last git-6eb0d3d and git-9bdf5be the bug disappeared, so I mark it as Resolved.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #22 from Stefan Dösinger stefandoesinger@gmx.at --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #23 from Piero Finizio andabata12@yahoo.it --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #24 from Piero Finizio andabata12@yahoo.it --- 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
https://bugs.freedesktop.org/show_bug.cgi?id=57875
Piero Finizio andabata12@yahoo.it changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |---
--- Comment #25 from Piero Finizio andabata12@yahoo.it --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #26 from Marek Olšák maraeo@gmail.com --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #27 from Stefan Dösinger stefandoesinger@gmx.at --- 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 :-)
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #28 from Marek Olšák maraeo@gmail.com --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #29 from Stefan Dösinger stefandoesinger@gmx.at --- 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).
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #30 from Christoph Bumiller e0425955@student.tuwien.ac.at --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #31 from Henri Verbeet hverbeet@gmail.com --- 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?
https://bugs.freedesktop.org/show_bug.cgi?id=57875
--- Comment #32 from Marek Olšák maraeo@gmail.com --- 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.
https://bugs.freedesktop.org/show_bug.cgi?id=57875
Marek Olšák maraeo@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |RESOLVED Resolution|--- |FIXED
--- Comment #33 from Marek Olšák maraeo@gmail.com --- I reverted the problematic commit: http://cgit.freedesktop.org/mesa/mesa/commit/?id=4dfe1a0df56d084b6a29fe423af...
Closing.
dri-devel@lists.freedesktop.org
-
bugzilla-daemon＠freedesktop.org