Bug 30007

If this is still a bug, you're going to have to provide more info than "it doesn't work anymore."

git bisect it. If you, for some reason, cannot do this, at least something like "it worked August 24 and stopped working September 3."

Excuse me for not providing enough information in the first place..
I now made some tests and I am sure that the blur effect works with commit e01a49af61a4d56800b1ad672959ba7a88c1da1e (Aug. 14th)..
I suspect that the regression is due to the introduction of the new compiler, which (I think) was merged around the 17th or 18th of August..
Is this of any help?

(In reply to comment #1)
> If this is still a bug, you're going to have to provide more info than "it
> doesn't work anymore."
> 
> git bisect it. If you, for some reason, cannot do this, at least something like
> "it worked August 24 and stopped working September 3."

A black area? It looks like a shader fail to compile. The driver always prints some info to stderr if anything goes wrong.

Can you somehow get the stderr output?

Also, is there anything in dmesg?

Created attachment 38627 [details]
dmesg log

I attached my dmesg..
I'm trying to figure out how to get the stderr output..

(In reply to comment #3)
> A black area? It looks like a shader fail to compile. The driver always prints
> some info to stderr if anything goes wrong.
> 
> Can you somehow get the stderr output?
> 
> Also, is there anything in dmesg?

I don't seem to be able to find a way to retrieve the stderr from the driver..
I'm on gentoo linux with mesa, libdrm and xf86-video-ati from git and xorg-server-1.9..
Can anybody suggest me how to retrieve that data so that I will be able to post it here?

(In reply to comment #5)
> I attached my dmesg..
> I'm trying to figure out how to get the stderr output..
> 
> (In reply to comment #3)
> > A black area? It looks like a shader fail to compile. The driver always prints
> > some info to stderr if anything goes wrong.
> > 
> > Can you somehow get the stderr output?
> > 
> > Also, is there anything in dmesg?

He means stderr from the program causing the problem. I don't know how you run kwin (I don't use kde), to get catch stderr output you generally want to run it from the terminal with something like `program-causing-problems &> ~/stderr.output`.

Ok, here is something that probably confirms the fact that there is a problem with a shader - it is form kwin's stderr:

r300 FP: Compiler Error:
Fragment program does not support relative addressing  of source operands.
Using a dummy shader instead.

(In reply to comment #7)
> He means stderr from the program causing the problem. I don't know how you run
> kwin (I don't use kde), to get catch stderr output you generally want to run it
> from the terminal with something like `program-causing-problems &>
> ~/stderr.output`.

Thanks. I am pretty sure the GLSL compiler fails to unroll a loop.

Also can you try recent Mesa git? IIRC the loop unrolling for the new compiler was merged on the 3rd September.

Actually, I'm using mesa compiled from git yesterday evening..

(In reply to comment #9)
> Thanks. I am pretty sure the GLSL compiler fails to unroll a loop.
> 
> Also can you try recent Mesa git? IIRC the loop unrolling for the new compiler
> was merged on the 3rd September.

Can your run kwin with the environment variable RADEON_DEBUG=fp and post the stderr output.

Tom, I don't think we handle relative addressing when unrolling loops.

An alternative solution on ML:

http://lists.freedesktop.org/archives/mesa-dev/2010-September/002896.html

I'm attaching it right now..
Marek - I tried to apply that patch by modifying the mesa-9999 ebuild, but patching failes..

(In reply to comment #11)
> Can your run kwin with the environment variable RADEON_DEBUG=fp and post the
> stderr output.

Created attachment 38634 [details]
kwin stderr with RADEON_DEBUG=fp

Yeah you probably need the other patches as well. It was actually the 10th patch in the series. Some of them have been committed, the others have not. I'm gonna wait until all of them are pushed to master.

Ah, ok..
If you could quickly figure out which patches are still to be committed and post them here, I could try if this solves the bug.
If that's too complicated, just let me know when they are merged, so I can give you feedback.

(In reply to comment #15)
> Yeah you probably need the other patches as well. It was actually the 10th
> patch in the series. Some of them have been committed, the others have not. I'm
> gonna wait until all of them are pushed to master.

Created attachment 38652 [details]
Piglit test for the kwin blur shader

(In reply to comment #9)
> Thanks. I am pretty sure the GLSL compiler fails to unroll a loop.

I think the shader that failed to compile is a different shader that we use for scaling textures for the taskbar thumbnails. The blur shader doesn't use loops.

But I have attached a piglit test that I hope can be used to reproduce the bug.

I don't have an R300, but the blur shader works for me with R600C and the new GLSL compiler. The piglit test fails with R600G, but not with the symptoms described in this bug report.

In fact I have the black thumbnails issue too; should I open a new bug for that?
Should I just recompile mesa with your patch and report what happens?


(In reply to comment #18)
> (In reply to comment #9)
> > Thanks. I am pretty sure the GLSL compiler fails to unroll a loop.
> 
> I think the shader that failed to compile is a different shader that we use for
> scaling textures for the taskbar thumbnails. The blur shader doesn't use loops.
> 
> But I have attached a piglit test that I hope can be used to reproduce the bug.
> 
> I don't have an R300, but the blur shader works for me with R600C and the new
> GLSL compiler. The piglit test fails with R600G, but not with the symptoms
> described in this bug report.

(In reply to comment #18)
> (In reply to comment #9)
> > Thanks. I am pretty sure the GLSL compiler fails to unroll a loop.
> 
> I think the shader that failed to compile is a different shader that we use for
> scaling textures for the taskbar thumbnails. The blur shader doesn't use loops.
> 
> But I have attached a piglit test that I hope can be used to reproduce the bug.
> 
> I don't have an R300, but the blur shader works for me with R600C and the new
> GLSL compiler. The piglit test fails with R600G, but not with the symptoms
> described in this bug report.

Why would you use relative addressing (= indirect indexing) in the fragment shader then? If it's not indexing with a loop counter (my assumption), what is it?

(In reply to comment #20)
> Why would you use relative addressing (= indirect indexing) in the fragment
> shader then? If it's not indexing with a loop counter (my assumption), what is
> it?

The shader used for scaling the thumbnails is indeed indexing with a loop counter. That shader looks like this:

uniform sampler2D texUnit;
uniform vec2 offsets[25];
uniform vec4 kernel[25];
uniform int kernelSize;

void main(void)
{
    vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
    for (int i = 1; i < kernelSize; i++) {
        sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
        sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
    }
    gl_FragColor = sum;
}

That's why I think it's this shader that failed to compile. But this is not the shader that's used by the blur effect (which this bug report was about). So I believe we're looking at two separate issues here.

Yes, for sure there are two separate issues: the black thumbnails (for which Fredrick seems to have identified the problem) and the blur issue I reported (for which, as far as I understood, it is not clear by what it is caused).
Just let me know if I can do any test or something to help you track the source of the bug.

(In reply to comment #21)
> (In reply to comment #20)
> > Why would you use relative addressing (= indirect indexing) in the fragment
> > shader then? If it's not indexing with a loop counter (my assumption), what is
> > it?
> 
> The shader used for scaling the thumbnails is indeed indexing with a loop
> counter. That shader looks like this:
> 
> uniform sampler2D texUnit;
> uniform vec2 offsets[25];
> uniform vec4 kernel[25];
> uniform int kernelSize;
> 
> void main(void)
> {
>     vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
>     for (int i = 1; i < kernelSize; i++) {
>         sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
>         sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
>     }
>     gl_FragColor = sum;
> }
> 
> That's why I think it's this shader that failed to compile. But this is not the
> shader that's used by the blur effect (which this bug report was about). So I
> believe we're looking at two separate issues here.

I've created a new bug for kwin blur effect: https://bugs.freedesktop.org/show_bug.cgi?id=30152

(In reply to comment #21)
> 
> The shader used for scaling the thumbnails is indeed indexing with a loop
> counter. That shader looks like this:
> 
> uniform sampler2D texUnit;
> uniform vec2 offsets[25];
> uniform vec4 kernel[25];
> uniform int kernelSize;
> 
> void main(void)
> {
>     vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
>     for (int i = 1; i < kernelSize; i++) {
>         sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
>         sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
>     }
>     gl_FragColor = sum;
> }
> 
> That's why I think it's this shader that failed to compile. But this is not the
> shader that's used by the blur effect (which this bug report was about). So I
> believe we're looking at two separate issues here.


The r500 should be able to execute this, but there are two things that are currently preventing correct executions:
1. Relative addressing in loops is not yet supported.
2. The frontend declares the variable "i" as a regular constant and not an immediate, so the r300 compiler has no idea what its initial value is, or what is incremented by, so  even with relative addressing in loops, this would still fail

Does kernelSize have to be a uniform?  If it is always the same then adding it as a constant would allow the loop to be unrolled which would get around both of the above issue.  Even something like this would be better:

uniform sampler2D texUnit;
uniform vec2 offsets[25];
uniform vec4 kernel[25];
uniform int kernelSize;
 
void main(void)
{
    vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
    for (int i = 1; i < 25; i++) {
        
sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
        sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
    }
    gl_FragColor = sum;
}

 
> uniform sampler2D texUnit;
> uniform vec2 offsets[25];
> uniform vec4 kernel[25];
> uniform int kernelSize;
> 
> void main(void)
> {
>     vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
>     for (int i = 1; i < 25; i++) {
> 
> sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
>         sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
>     }
>     gl_FragColor = sum;
> }

Oops, I submitted before I was done.  This shader would be better, because the loop could be unrolled even if kernelSize is different in each execution:

uniform sampler2D texUnit;
uniform vec2 offsets[25];
uniform vec4 kernel[25];
uniform int kernelSize;

void main(void)
{
    vec4 sum = texture2D(texUnit, gl_TexCoord[0].st) * kernel[0];
    for (int i = 1; i < 25; i++) {
        if ( i < kernelSize) {
            sum += texture2D(texUnit, gl_TexCoord[0].st - offsets[i]) * kernel[i];
            sum += texture2D(texUnit, gl_TexCoord[0].st + offsets[i]) * kernel[i];
        }
    }
    gl_FragColor = sum;
}

Fredrik, thanks for the feedback. I've pushed your patch to the piglit repository.

(In reply to comment #23)
> The r500 should be able to execute this, but there are two things that are
> currently preventing correct executions

However the reporter has r400, so the hardware just cannot do it. I am nearly sure fglrx would fall back to software, which is not particularly useful for us. Specifying kernelSize at compile time should be enough for the compiler to unroll the loop.

(In reply to comment #24)
> Oops, I submitted before I was done.  This shader would be better, because the
> loop could be unrolled even if kernelSize is different in each execution:

Yeah, the shader could work provided the instruction scheduler generates no more than 4 TEX indirections. I'd personally lean towards making kernelSize a compile-time constant. So instead of calling glUniform1f(kernelSize, i), let's call glUseProgram(shader[i]);

Fredrik, please, would it be hard to implement this in kwin?

Another option for kwin would be using the shader only on GL3-capable hardware, because lots of GL2 hardware don't support indirect indexing.

Are you sure that we have to change kwin?

For me, blur worked just fine on rv410 when using the old glsl compiler - it stopped working right after the merge of the new compiler.

Because of that, I wonder if it might just be a software (glsl2) issue. Or was there some sort of software fallback in the old compiler that is not available in the new one?

Concerning the thumbnail issue: I think that thumbnails used to work with kernel 2.6.34 and stopped working when I updated to 2.6.35 (some time before the compiler merge). Was there any feature added to 2.6.35 that could cause this issue?

(In reply to comment #25)
> Fredrik, thanks for the feedback. I've pushed your patch to the piglit
> repository.

Thanks.

> (In reply to comment #24)
> > Oops, I submitted before I was done.  This shader would be better, because the
> > loop could be unrolled even if kernelSize is different in each execution:
> 
> Yeah, the shader could work provided the instruction scheduler generates no
> more than 4 TEX indirections. I'd personally lean towards making kernelSize a
> compile-time constant. So instead of calling glUniform1f(kernelSize, i), let's
> call glUseProgram(shader[i]);
> 
> Fredrik, please, would it be hard to implement this in kwin?

It shouldn't be too hard, and it's actually something I've been planning to do but never got around to before the KDE 4.5 release.

In the meantime I've committed a change that gets rid of the kernelSize uniform  and makes the loop always do a fixed number of iterations instead (with the unused kernel weights set to zero).

The MESA_GLSL=dump output shows that the loop is being unrolled, so with a bit of luck the shader should be working now.

(In reply to comment #26)
> Another option for kwin would be using the shader only on GL3-capable hardware,
> because lots of GL2 hardware don't support indirect indexing.

Working well with the Mesa drivers and the hardware our users are using is something I consider a high priority, so if we can make the shader work without indirect indexing then I think that's the preferred approach.

Thanks to you and Tom for all the suggestions.

(In reply to comment #27)
> Are you sure that we have to change kwin?
> 
> For me, blur worked just fine on rv410 when using the old glsl compiler - it
> stopped working right after the merge of the new compiler.
> 
> Because of that, I wonder if it might just be a software (glsl2) issue. Or was
> there some sort of software fallback in the old compiler that is not available
> in the new one?

Well this discussion is only about the thumbnail issue now. The discussion about the blur effect has been moved to bug 30152.

> Concerning the thumbnail issue: I think that thumbnails used to work with
> kernel 2.6.34 and stopped working when I updated to 2.6.35 (some time before
> the compiler merge). Was there any feature added to 2.6.35 that could cause
> this issue?

The only thing I can think of is if for some reason kwin decided not to use the shader with 2.6.34. If the upgrade make additional GL extensions available it could explain why the problem didn't show up before.

Sorry for the stupid question, but is the thumbnail bug fixed with kde svn + the latest mesa git?
Will we get the fixes with kde-SC-4.5.2 and mesa-1.9?

(In reply to comment #30)
> Sorry for the stupid question, but is the thumbnail bug fixed with kde svn +
> the latest mesa git?
> Will we get the fixes with kde-SC-4.5.2 and mesa-1.9?

Well I can't test it, but the commit I was referring to will be in 4.5.2.

Would you be able to post a patch with this commit for the 4.5.1 source?
If you do so, I could test it and give you feedback..
If this is not feasible, then I'll wait for 4.5.2..

(In reply to comment #31)
> (In reply to comment #30)
> > Sorry for the stupid question, but is the thumbnail bug fixed with kde svn +
> > the latest mesa git?
> > Will we get the fixes with kde-SC-4.5.2 and mesa-1.9?
> 
> Well I can't test it, but the commit I was referring to will be in 4.5.2.

The black thumbnails bug is now fixed for me with kde sc-4.5.1 and the latest git mesa..
The blur effects keeps being broken though..

Just to keep the bug up to date..
I got a new laptop with an integrated intel graphics card (core i5, using i965 classic driver) and the blur effect is broken exactly as it was with r300g (black background instead of blurred image).
Probably the bug is not in the driver but in kwin..

(In reply to comment #33)
> The black thumbnails bug is now fixed for me with kde sc-4.5.1 and the latest
> git mesa..
> The blur effects keeps being broken though..

(In reply to comment #8)
> r300 FP: Compiler Error:
> Fragment program does not support relative addressing  of source operands.
> Using a dummy shader instead.

Relative addressing is supported since commit 	cbfdf262ccf8b573f1fa4c0065a8eb1fb87d93da.

Thumbnails don't work on r5xx because the r300 compiler fails to do any register allocation inside loops. Disabling the "register rename" pass may fix it.

davide, the thumbnails cannot work for you anyway because ATI X700 doesn't support loops. Not sure what's the current status in latest kwin, I am using kwin 4.5.1.

*** Bug 29400 has been marked as a duplicate of this bug. ***

First some findings. The current implemention of thumbnails in kwin is strictly targeted for GL3 hardware and should not work on GL2 hardware. This simple fact can be seen from how the GLSL shader looks like and what features it uses (you might also notice that the shader cannot be even ported to DirectX 9 without a complete rewrite). However, the GLSL compiler in Mesa does something fglrx didn't do: it lowers indirect addressing to conditional moves, but the cost is such shaders end up being too long and therefore very slow. This is something KDE people should fix if they want to ever compete with the performance of other compositors. So far my impression from reading Kwin shaders isn't very good and their shaders really need a broader review before getting accepted unless Kwin is a toy project not suitable for every day use.

Commit 8543902bfbdfc15c39525bd99bee22e2f2126e74 fixes the Kwin thumbnails for R5xx GPUs, but we can't really do anything with the speed.

One way to address this and other well-known performance issues with Kwin is to refraing from using loops in GLSL shaders.