| 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 %).
Does reverting the commit e866bd1adea2c3b4971ad68e69c644752f2a= b7b6 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 se= ems to be a legitimate problem how depth clamping is handled. The application enables i= t, 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 requ= ired 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 docum= ent 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 hac= ks 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 h= elp 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 extensi= on that exposes CLIP_DISABLE as defined by D3D9 to the user. The problem is su= ch 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-ver= tex. 4) Disabling ARB_depth_clamp again.
(In reply to comment #8= ) > That bit only exists on r5xx asics. R3xx and r4= xx don't have that bit. I'd say it's related to floating point depth buffer support. I don't know a= bout 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, particul= arly 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 inste= ad 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 r3= 00g, 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 abo= ut that route. > 3) We could clamp POS.Z to the range [-POS.W, PO= S.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 t= he fragment stage.
I found the r500 register docs, and yeah, the Z_EXTENDED bit s= ounds 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 ca= ll it GL_MESA_depth_clip_disable or GL_MESA_depth_clamp_d3d. One could try to combine the CLIP_DISABLE register with clamping gl_FragDep= th 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 a= nd r4xx don't have that bit. > I'd say it's related to floating point depth buffer support. I don't k= now > 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 bes= ides some quick hacks and I don't > > have them anymore. > I've written a kernel hack, now the broken geometry is mostly behind t= he > correct parts. I guess this is where you stopped last time. >=20 > 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 an= d the 92e7c6a258 fixes a piglit test which passes with r300g, so it's most probab= ly unrelated. glDepthRange is packed in the Z component of the vectors in pipe_viewport_state, which is applied after clipping and converts coordinat= es from the clip space to the window space.
Created attac=
hment 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 "S=
tatus".
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.
| What | Removed | Added |
|---|---|---|
| Attachment #71244 is obsolete | 1 |
Created att=
achment 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 e866bd1adea2c3b4971ad68e69c64475= 2f2ab7b6 (above mentioned workaround) with git "HEAD at 7c35521 mesa: add missing texel fetch code for sRGB DXT forma= ts" the compilation ends with: ... CC r300_screen.o r300_screen.c: In function =E2=80=98r300_get_param=E2=80=99: r300_screen.c:130:14: error: =E2=80=98PIPE_CAP_TIMER_QUERY=E2=80=99 undecla= red (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 =E2=80=98PIPE_CAP_QUERY_TIME= _ELAPSED=E2=80=99 not handled in switch [-Wswitch] r300_screen.c:88:5: warning: enumeration value =E2=80=98PIPE_CAP_TEXTURE_BUFFER_OBJECTS=E2=80=99 not handled in switch [-W= switch] 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 val= ues 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.
| What | Removed | Added |
|---|---|---|
| Status | NEW | RESOLVED |
| Resolution | --- | FIXED |
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.