Bug 57842 - r200: Culling is broken when rendering to an FBO
Summary: r200: Culling is broken when rendering to an FBO
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/r200 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact:
Depends on:
Reported: 2012-12-03 15:12 UTC by Stefan Dösinger
Modified: 2013-01-03 18:31 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

Test case (3.13 KB, text/plain)
2012-12-03 15:12 UTC, Stefan Dösinger
potential fixes for viewport trouble (10.66 KB, patch)
2012-12-04 20:57 UTC, Roland Scheidegger
Details | Splinter Review
Fix for attachment 71014 (554 bytes, patch)
2012-12-05 08:50 UTC, Stefan Dösinger
Details | Splinter Review
Myr200FrontFace (1.03 KB, text/plain)
2012-12-23 23:41 UTC, smoki
STK minimap icon (886.35 KB, image/png)
2012-12-24 02:18 UTC, smoki
attachment 72047 as patch for r200/radeon (2.43 KB, patch)
2012-12-24 03:02 UTC, Rafał Mużyło
Details | Splinter Review

Description Stefan Dösinger 2012-12-03 15:12:57 UTC
Created attachment 70972 [details]
Test case

When rendering to an FBO with culling enabled, the r200 driver discards all geometry. As soon as GL_CULL_FACE is enabled, nothing is drawn. The glCullFace, glFrontFace and primitive orientation don't seem to make a difference. When rendering to a glx front or back buffer, culling behaves correctly.

The attached test case shows the issue. It creates an FBO, clears it with a blue color, and then draws a front- and back facing triangle in red and green respectively, and culls front-facing primitives. Afterwards the FBO texture is drawn to the window. The expected result is that the green triangle shows up. The actual result is a solid blue window.

I've had a quick look at the driver source code. r200FrontFace swaps front- and back facing geometry because the driver renders everything upside down on FBOs. This code doesn't seem to have any effect on this bug. What seems to trigger it is not inverting the viewport in r200UpdateWindow(), but I don't understand why.

The test case can be compiled with gcc fbo.c -o fbo -lGL -lglut , and started by simply running the binary without any parameters.
Comment 1 Roland Scheidegger 2012-12-04 20:20:18 UTC
Hmm this is odd I can't see anything wrong with the culling setup wrt fbos.
The viewport, window handling though seems somewhat bogus.
First, r200UpdateViewportOffset() has no callers (it is used in radeon->vtbl.update_viewport_offset but that one seems to have no callers neither). This function would not take the y flip into account but since it isn't used... I'd just get rid of it but it's the only thing handling stippling that probably needs to move somewhere else.
Also, both r200UpdateViewportOffset() and r200UpdateWindow() use the driDrawable height for the yoffset in the viewport. I don't think that will work for fbos, should just use the ctx->DrawBuffer height instead?
I can't see though why that would just cause failures with culling.
Comment 2 Roland Scheidegger 2012-12-04 20:57:11 UTC
Created attachment 71014 [details] [review]
potential fixes for viewport trouble

I had something like this in mind. Untested though and all that viewport updating stuff seems really weird. I hope I didn't create circular calling chains...
Might not fix this bug at all...
Comment 3 Stefan Dösinger 2012-12-05 08:50:04 UTC
Created attachment 71022 [details] [review]
Fix for attachment 71014 [details] [review]

The patch needed this small fix to compile. It seems that my gcc doesn't like logical operations on floats, which kinda makes sense.

Otherwise I see no difference, but I haven't really tested polygon stipple or the scissor rect on fbos yet. Clipping is still broken.
Comment 4 Stefan Dösinger 2012-12-05 08:51:39 UTC
(In reply to comment #3)
> Clipping is still broken.
Sorry, this should read "culling is still broken".
Comment 5 smoki 2012-12-23 23:41:40 UTC
Created attachment 72047 [details]

 Seems like i managed to fix it, but with dirty hack... see function in attachment. tcl somehow is not aware of wind inverting, and i just inverted commands, but it works that way :).

 This fbo test case works, fbotexture and tri-cull from mesa demos - with both swtcl and tcl. What i tested, yeah Aquaria game is no more black with fbo usage and also Supertuxkart now show icons in minimap, etc.

 Anyhow, maybe someone know how to make that better to be acceptible for mesa and pushed in git - i don't know better :).
Comment 6 Stefan Dösinger 2012-12-24 00:19:33 UTC
I'll give the patch a try after the holidays. My I'm currently 100 miles away from my r200-equipped laptop and won't have access to it until Wednesday or Thursday.
Comment 7 Rafał Mużyło 2012-12-24 01:04:03 UTC
Perhaps the message in this commit (http://cgit.freedesktop.org/mesa/mesa/commit/src/mesa/drivers/dri/r200/r200_state.c?id=60e60bb3026a269fefe1cfd3312fdf3a7e4c595f) is of note:

Tested with r300, radeon and r200 compile tested only.

Though looking at the changes of attachment 72047 [details], perhaps the other cards should be rechecked too - these changes look more like the changes made in that commit to r300, so it might not be as much of a hack as the author thought.
Comment 8 Rafał Mużyło 2012-12-24 01:16:38 UTC
...minor correction: that r300 was for the non-galium driver (that was removed awhile ago), but it seems that the logic in that commit was incorrect, so the "hack" from that attachement seems to be a logic fix and most likely *is* valid for radeon/radeon_state.c.
Comment 9 smoki 2012-12-24 01:36:29 UTC
 That Dänzer's commit is good, it inverts winding properly and it works good but for swtcl only. My tries to make it work but for both swtcl and tcl, and for Stefan's test case also.

 And it works for me both swtcl and tcl for those tests, but i am not very good at programming ;).

 "Fixes fgl_glxgears and progs/demos/fbotexture after pressing 'c'."

 Yeah that both works too "pressing 'c'" now works. Pbuffered fgl_glxgears works, but with -fbo switch - that never showing a texture on r200 :).

 So i've tested: fgl_glxgears, fbotexture, supertuxkart, fbo test from this bug...
and also tri-cull to be sure no-fbo case work. And that all works for swtcl and tcl on rv280.
Comment 10 smoki 2012-12-24 02:18:37 UTC
Created attachment 72050 [details]
STK minimap icon

 He, he, i'am not programmer at all ;) but i managed to fix fogcoords, lighting and culling on rv280 with tcl this year.
 Happy holidays :).
Comment 11 Rafał Mużyło 2012-12-24 03:02:43 UTC
Created attachment 72051 [details] [review]
attachment 72047 [details] as patch for r200/radeon

@comment 9: your own attachment says it was not.
Anyway, attachment 72047 [details] translated into a patch.

'cull_face = (mode == GL_CW) ? R200_FFACE_CULL_CW : R200_FFACE_CULL_CCW'
is bit of a mouthful, but atm. I've got not idea how to turn that into proper obfuscation style of mesa sources.
Comment 12 smoki 2012-12-24 03:38:13 UTC
 Yeah i tried your patch and it works, thanks Raphal. Someone could pushed it in git if it's in good looking now ;).
Comment 13 Roland Scheidegger 2012-12-30 22:32:45 UTC
(In reply to comment #11)
> Created attachment 72051 [details] [review] [review]
> attachment 72047 [details] as patch for r200/radeon
> @comment 9: your own attachment says it was not.
> Anyway, attachment 72047 [details] translated into a patch.
> 'cull_face = (mode == GL_CW) ? R200_FFACE_CULL_CW : R200_FFACE_CULL_CCW'
> is bit of a mouthful, but atm. I've got not idea how to turn that into
> proper obfuscation style of mesa sources.

patch looks correct to me. TCL culling is part of the tcl engine and hence ignored if there's no TCL. But viewport transform (which is where the fbo inversion happens) is done later, hence TCL culling is not subject to that inversion.
FWIW it would also be possible to simply disable TCL culling, setup culling is enough. The docs are actually saying it TCL culling is a performance optimization with questionable quality (as the coords aren't snapped to rasterizer subpixels). I guess it would also be possible to disable setup culling instead (of course only with tcl) but I'm not sure if that would make any performance compared to having enabled culling at both places.
Comment 14 Stefan Dösinger 2013-01-02 22:42:07 UTC
The patch fixes the culling issues in the dx8 d3d samples as well. Smoki, can you  mail the patch to the mesa-dev list? I could do it as well, but usually it's better if the author does this.
Comment 15 smoki 2013-01-03 05:14:54 UTC
 Feel free to send it Stefan. Shame on me :( i don't know how to make proper mesa patch nor how to send it :(.
Comment 16 Stefan Dösinger 2013-01-03 14:17:41 UTC
Commit it to Git and use git send-email to send it to mesa-dev@lists.freedesktop.org . Google finds many tutorials on how to do this.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.