Bug 31830

Created attachment 40468 [details]
output with RADEON_DEBUG=fp and RADEON_DEBUG=fp,noopt

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

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?

(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.

Created attachment 42355 [details] [review]
Possible Fix

I think this patch fixes Lightsmark, can you confirm this?  Does this patch fix Sanctuary as well?

(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.

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.

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?

(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.

(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.

(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.

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.

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.

Created attachment 42626 [details]
Screenshot with RADEON_DEBUG=noopt

Created attachment 42627 [details]
Screenshot with scheduler patch

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?

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.

(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?

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.

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.