Bug 31830 - [bisected] broken shadows in Unigine Sanctuary and Lightsmark
[bisected] broken shadows in Unigine Sanctuary and Lightsmark
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/r300
git
Other All
: medium normal
Assigned To: Default DRI bug account
http://www.unigine.com/download/#sanc...
: regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-21 23:56 UTC by Pavel Ondračka
Modified: 2011-02-05 23:54 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
screenshot (181.88 KB, image/jpeg)
2010-11-21 23:56 UTC, Pavel Ondračka
Details
output with RADEON_DEBUG=fp and RADEON_DEBUG=fp,noopt (207.26 KB, application/x-bzip2)
2010-11-21 23:57 UTC, Pavel Ondračka
Details
Possible Fix (1.58 KB, patch)
2011-01-24 01:13 UTC, Tom Stellard
Details | Splinter Review
Disable instruction rewriting in the scheduler (1.56 KB, patch)
2011-01-26 01:11 UTC, Tom Stellard
Details | Splinter Review
screenshot of the problem (318.90 KB, image/jpeg)
2011-01-26 02:17 UTC, Pavel Ondračka
Details
Screenshot with RADEON_DEBUG=noopt (109.30 KB, image/jpeg)
2011-01-28 00:21 UTC, Tom Stellard
Details
Screenshot with scheduler patch (115.65 KB, image/jpeg)
2011-01-28 00:22 UTC, Tom Stellard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Ondračka 2010-11-21 23:56:43 UTC
Created attachment 40467 [details]
screenshot

With latest mesa git shadows in Unigine are broken again. This is with shaders set to low or medium (high works fine).

commit 8833f53e659e079e7ab74bb9197f9b44b1eeefe0
Author: Tom Stellard <tstellar@gmail.com>
Date:   Wed Nov 10 21:34:18 2010 -0800

    r300/compiler: Enable rename_reg pass for r500 cards
    
    In addition, the rename_reg pass has been rewritten to use
    rc_get_readers().


Workaround is RADEON_DEBUG=noopt.
Comment 1 Pavel Ondračka 2010-11-21 23:57:50 UTC
Created attachment 40468 [details]
output with RADEON_DEBUG=fp and RADEON_DEBUG=fp,noopt
Comment 2 Pavel Ondračka 2011-01-21 04:19:37 UTC
Penumbra shadows in Lightsmark 2008 2.0 are broken with the same commit. It works fine with RADEON_DEBUG=noopt. 
BTW during bisecting I run into some interesting performance issues:

Before 8833f53e659e079e7ab74bb9197f9b44b1eeefe0: 5,66 +- 0,01 fps, shadows are OK
mesa 8833f53e659e079e7ab74bb9197f9b44b1eeefe0: 3,69 +- 0,07 fps, broken shadows

with current mesa master (af4e2f46653cbc7ceaf1291ba22087ec5758d07f):
stock settings: 10,23 +- 0,08 fps, broken shadows
with RADEON_DEBUG=noopt: 11,73 +- 0,16 fps, shadows are OK
with register rename pass disabled: 14,35 +- 0,04 fps shadows are OK

I disabled register rename by changing r3xx_fragprog.c line 127,
{"register rename", 1, !is_r500 || opt, rc_rename_regs,	NULL},
to {"register rename", 1, !is_r500, rc_rename_regs,NULL},
I hope this is correct.

Mesa: af4e2f46653cbc7ceaf1291ba22087ec5758d07f
Kernel: 2.6.37
GPU: RV530
Comment 3 Tom Stellard 2011-01-23 00:33:24 UTC
What penumbra quality number are you using in Lightsmark? Are you seeing any error messages like this:

r300: ERROR: FS input generic 18 unassigned, not enough hardware slots.
r300: ERROR: FS input generic 19 unassigned, not enough hardware slots.
r300: ERROR: FS input generic 20 unassigned, not enough hardware slots.
r300: ERROR: FS input WPOS unassigned, not enough hardware slots.

If you are, were they also present before commit 8833f53e659e079e7ab74bb9197f9b44b1eeefe0?
Comment 4 Pavel Ondračka 2011-01-23 00:53:39 UTC
(In reply to comment #3)
> What penumbra quality number are you using in Lightsmark? Are you seeing any
> error messages like this:
> 
> r300: ERROR: FS input generic 18 unassigned, not enough hardware slots.
> r300: ERROR: FS input generic 19 unassigned, not enough hardware slots.
> r300: ERROR: FS input generic 20 unassigned, not enough hardware slots.
> r300: ERROR: FS input WPOS unassigned, not enough hardware slots.
> 
> If you are, were they also present before commit
> 8833f53e659e079e7ab74bb9197f9b44b1eeefe0?

Yes, I'm seeing identical messages, and they also were present before 8833f53e659e079e7ab74bb9197f9b44b1eeefe0.

Penumbra quality: 8/8 on Gallium 0.4 on ATI RV530.
Comment 5 Tom Stellard 2011-01-24 01:13:49 UTC
Created attachment 42355 [details] [review]
Possible Fix

I think this patch fixes Lightsmark, can you confirm this?  Does this patch fix Sanctuary as well?
Comment 6 Pavel Ondračka 2011-01-25 05:24:00 UTC
(In reply to comment #5)
> Created an attachment (id=42355) [details]
> Possible Fix
> 
> I think this patch fixes Lightsmark, can you confirm this?  Does this patch fix
> Sanctuary as well?

I'm sorry to report this, but your patch doesn't fix neither Lightsmark nor Sanctuary here.
Comment 7 Tom Stellard 2011-01-26 01:11:35 UTC
Created attachment 42514 [details] [review]
Disable instruction rewriting in the scheduler

I think this bug is actually in the scheduler, and it is just being exposed by the reg rename pass.  I have been able to track down a few instructions that the scheduler is converting from vector to scalar that change how the shadows are rendered.  However, these conversions look valid too me, so I'm really not sure what the problem is.  I think this patch fixes Lightsmark, but I'm not sure what the shadows are supposed to look like.  Can you try this patch on Lightsmark and Sanctuary.  Thanks.
Comment 8 Pavel Ondračka 2011-01-26 02:17:15 UTC
Created attachment 42516 [details]
screenshot of the problem

(In reply to comment #7)
> Created an attachment (id=42514) [details]
> Disable instruction rewriting in the scheduler
> 
> I think this bug is actually in the scheduler, and it is just being exposed by
> the reg rename pass.  I have been able to track down a few instructions that
> the scheduler is converting from vector to scalar that change how the shadows
> are rendered.  However, these conversions look valid too me, so I'm really not
> sure what the problem is.  I think this patch fixes Lightsmark, but I'm not
> sure what the shadows are supposed to look like.  Can you try this patch on
> Lightsmark and Sanctuary.  Thanks.

Hi, this patch doesn't help neither. I tested it alone and also with the first patch. Concerning how the penumbra shadows should look, you don't get the right behavior with RADEON_DEBUG=noopt? I've attached screenshot comparing bad and good shadows. Don't know how to help you more, I still have the debug patches you've sent me when we were debugging sanctuary before the "enable rename_reg" patch was committed, maybe I could use them again to identify the failing shader?
Comment 9 Tom Stellard 2011-01-26 09:05:17 UTC
(In reply to comment #8)
> Created an attachment (id=42516) [details]
> screenshot of the problem
> 
> Hi, this patch doesn't help neither. I tested it alone and also with the first
> patch. Concerning how the penumbra shadows should look, you don't get the right
> behavior with RADEON_DEBUG=noopt? I've attached screenshot comparing bad and
> good shadows. Don't know how to help you more, I still have the debug patches
> you've sent me when we were debugging sanctuary before the "enable rename_reg"
> patch was committed, maybe I could use them again to identify the failing
> shader?

That's OK, I already know which shader is failing.
Comment 10 Tom Stellard 2011-01-27 00:48:55 UTC
(In reply to comment #3)
> What penumbra quality number are you using in Lightsmark? Are you seeing any
> error messages like this:
> 
> r300: ERROR: FS input generic 18 unassigned, not enough hardware slots.
> r300: ERROR: FS input generic 19 unassigned, not enough hardware slots.
> r300: ERROR: FS input generic 20 unassigned, not enough hardware slots.
> r300: ERROR: FS input WPOS unassigned, not enough hardware slots.
> 

In Lightsmark, I've been able to identify the failing shader and a few instruction transformations that change the way the shadows are rendered.  I've spent a lot of time looking at this, and all the transformations in question appear to be correct.  It's possible that above errors are the reason why the shadows are misrendering.  The last error about WPOS also appears in sanctuary.  

My hypothesis is that with these kinds of errors, the behavior of a shader is undefined.  This means that prior to the commit in question, the scenes were by luck being rendered correctly, or close enough so that it wasn't noticeable.  Then, the modifications made to the shader by the reg_rename pass changed the state of the GPU in such a way that the misrendering became more obvious.

Maybe Marek can comment on whether or not this is a plausible hypothesis.
Comment 11 Pavel Ondračka 2011-01-27 01:18:41 UTC
(In reply to comment #10)
> (In reply to comment #3)
> > What penumbra quality number are you using in Lightsmark? Are you seeing any
> > error messages like this:
> > 
> > r300: ERROR: FS input generic 18 unassigned, not enough hardware slots.
> > r300: ERROR: FS input generic 19 unassigned, not enough hardware slots.
> > r300: ERROR: FS input generic 20 unassigned, not enough hardware slots.
> > r300: ERROR: FS input WPOS unassigned, not enough hardware slots.
> > 
> 
> In Lightsmark, I've been able to identify the failing shader and a few
> instruction transformations that change the way the shadows are rendered.  I've
> spent a lot of time looking at this, and all the transformations in question
> appear to be correct.  It's possible that above errors are the reason why the
> shadows are misrendering.  The last error about WPOS also appears in sanctuary. 
> 
> My hypothesis is that with these kinds of errors, the behavior of a shader is
> undefined.  This means that prior to the commit in question, the scenes were by
> luck being rendered correctly, or close enough so that it wasn't noticeable. 
> Then, the modifications made to the shader by the reg_rename pass changed the
> state of the GPU in such a way that the misrendering became more obvious.
> 
> Maybe Marek can comment on whether or not this is a plausible hypothesis.

Does this also explain the slowdown mentioned in comment 2? Because it is in all scenes, not only the "penumbra shadows" one and it is really weird that RADEON_DEBUG=noopt actually makes things faster.
Comment 12 Marek Olšák 2011-01-27 02:20:19 UTC
Well, if I disable the register rename pass, the Penumbra shadows are rendered more or less correctly, so it must be a bug somewhere in the pair passes (or maybe even rename_regs).

My hypothesis is that a few unitialized shadow-map coordinates shouldn't influence the results computed from other shadow maps and those undefined shadow map samples should not contribute much to the final color, considering that the majority of samples are still computed correctly.

Also, disabling rename_regs speeds up Lightsmark by about 50%, so we should really reconsider whether having such a pass is worth the cost.
Comment 13 Tom Stellard 2011-01-28 00:20:29 UTC
I'm pretty sure this is a bug in the scheduler.  I'm attaching screenshots comparing running with this patch: https://bugs.freedesktop.org/attachment.cgi?id=42514 to running with RADEON_DEBUG=noopt.  The screenshots look very similar, but I think the shadow with RADEON_DEBUG=noopt is darker.

As for performance difference between RADEON_DEBUG=noopt and with optimizations.  I think we are seeing this because Lightsmark compiles new shaders while running the benchmark and the longer compile time for optimized shaders is lowering the FPS.
Comment 14 Tom Stellard 2011-01-28 00:21:18 UTC
Created attachment 42626 [details]
Screenshot with RADEON_DEBUG=noopt
Comment 15 Tom Stellard 2011-01-28 00:22:12 UTC
Created attachment 42627 [details]
Screenshot with scheduler patch
Comment 16 Pavel Ondračka 2011-01-28 00:53:06 UTC
I'm puzzled, because here with mesa master + disable scheduler rewrite patch, I get the wrong behavior (right part of https://bugs.freedesktop.org/attachment.cgi?id=42516 ). However your screenshot in Comment 15 looks almost completely good. To be sure I did once again git clean -fdx, git reset --hard origin/master, patch again and recompile, but nothing changed. Some hardware difference or my setup is broken? Maybe someone with the same card as mine (RV530) can test your patch to confirm if it indeed fixes the shadows? Any chance you have also some other patches applied?
Comment 17 Tom Stellard 2011-01-28 01:36:08 UTC
The right side of this screenshot that you posted: https://bugs.freedesktop.org/attachment.cgi?id=42516
Looks a lot like what I see when I disable the reg rename pass.

I've double checked the results with the scheduler patch and they are the same as the screenshot I posted.
Comment 18 Pavel Ondračka 2011-01-28 03:42:50 UTC
(In reply to comment #17)
> The right side of this screenshot that you posted:
> https://bugs.freedesktop.org/attachment.cgi?id=42516
> Looks a lot like what I see when I disable the reg rename pass.
> 
> I've double checked the results with the scheduler patch and they are the same
> as the screenshot I posted.

This is getting weird, because Marek in comment 12 mentioned that the penumbra shadows are rendered correctly with register rename pass disabled. I guess he also have RV530?
Comment 19 Marek Olšák 2011-01-28 10:56:20 UTC
I think I know the reason why it's slow with the rename_regs pass.

The statistics of the most complex shader are:

~~~~~~~~ FRAGMENT PROGRAM ~~~~~~~
~ 108 Instructions
~  71 Vector Instructions (RGB)
~  35 Scalar Instructions (Alpha)
~   0 Flow Control Instructions
~  36 Texture Instructions
~   2 Presub Operations
~  36 Temporary Registers
~~~~~~~~~~~~~~ END ~~~~~~~~~~~~~~

If you disable rename_regs, you will get:

~~~~~~~~ FRAGMENT PROGRAM ~~~~~~~
~ 143 Instructions
~  71 Vector Instructions (RGB)
~  35 Scalar Instructions (Alpha)
~   0 Flow Control Instructions
~  36 Texture Instructions
~   2 Presub Operations
~   4 Temporary Registers
~~~~~~~~~~~~~~ END ~~~~~~~~~~~~~~

Wow, 4 temps! It definitely appears to be a register allocator issue. Even though the rename_regs pass has helped with instruction scheduling a lot, it made regalloc perform much worse.
Comment 20 Tom Stellard 2011-02-05 23:54:34 UTC
This is fixed in master by commit 68b701f5ded5f6b73a6b07cf37d862fab4836607.  I'm still not sure what exactly was causing the misrenderings, but it is clear that the register rename pass and the scheduler were hurting the performance of Lightsmark, so, I'm disabling the register rename pass until we have a better scheduler.