Bug 10024

Summary: R300 fragment.position viewport transformation is incorrect
Product: Mesa Reporter: Oliver McFadden <z3ro.geek>
Component: Drivers/DRI/r300Assignee: Default DRI bug account <dri-devel>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: high CC: darius.scerb, rune
Version: git   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Screenshot of incorrect fragment.position
Rendering with R300
Rendering with the blob
test app: fragment.position clipping issue
Another rendering with R300
Indirect buffers dump with iba-2
tri-mov indirect buffers
diff between the dumps of tri-mov and tri-position
wpos in FP hack
Looking from the left to the right
Looking from the right to the left
Rendering with R300 and the latest patch
R300 fragment.position.xy
Blob fragment.position.xy
R300 fragment.position * 0.005
Blob fragment.position * 0.005
R300 fragment.position with correct range modification via MUL
R300 with the floor as a mirror
wpos for texure patch
Blob rendering the vertical mirror with correct XY masking.
R300 rendering the vertical mirror with correct XY masking.
R300 rendering the vertical mirror with INCORRECT XYZ masking.
Screenshot 1
Screenshot 2
Screenshot 3
Screenshot 4
Screenshot 5
Screenshot 6
Blob rendering parallel to the mirror (good)
R300 rendering parallel to the mirror
The fragment program used with the last two screenshots
Blob results for Piglit test
R300 results for Piglit test
Diff of the previous two attachments

Description Oliver McFadden 2007-02-18 19:09:02 UTC
I believe that the viewport transformation that R300 will insert into the vertex
program when you use fragment.position in the fragment program is incorrect or
at least takes some shortcuts that results in bad results in some cases.

I will attach a screenshot of this problem.
Comment 1 Oliver McFadden 2007-02-18 19:11:13 UTC
Created attachment 8773 [details]
Screenshot of incorrect fragment.position

A screenshot showing the incorrect fragment.position in my engine based on the
Quake 3 and XreaL code.
Comment 2 Oliver McFadden 2007-02-18 19:13:41 UTC
I forgot to mention that this program works correctly with the blob, so it's not
a bug in my program, but defiantly a bug in the R300 driver.

This is probably more accurately described as an error of omission, or R300
taking some shortcut on the viewport transformation, perhaps.

Maybe it would be possible to dump what the blob does for fragment.position...
I've tried this, but so far I haven't got any useful results.
Comment 3 Oliver McFadden 2007-02-18 19:18:45 UTC
I also forgot to mention that in the screenshot you should see a mirror, which
should function just like a mirror would in the real world. You should see the
wall reflected in the mirror, and not the weird incorrect effect that actually
happens.

Again, the exact same program does work with the blob, so it's not a bug in my
code.
Comment 4 Rune Petersen 2007-02-20 10:23:39 UTC
I am ill atm, but I'll make a comment:

I see no way to make a mirror using fragment.position. Quake 3 does mirrors/portals without using fragment.position, and R300 still renders it wrong.

Please explain how it is done.
Comment 5 Oliver McFadden 2007-02-20 11:08:03 UTC
You render the scene from the point of view of the mirror, into a texture, then render that texture onto a surface. At least this is how I understand it, I didn't write this part of the renderer, but I did write the ARB fragment and vertex program backend and the ARB fragment and vertex programs for it.  It is defiantly a case of R300 doing something wrong, or not doing something it should be doing, because this code will work with the blob. I can generate some comparison screenshots with a simple "MOV result.color, fragment.position;" and you can see the difference. I'll do this later today.  I hope you feel better soon. 
Comment 6 Oliver McFadden 2007-02-20 11:18:19 UTC
Created attachment 8790 [details]
Rendering with R300
Comment 7 Oliver McFadden 2007-02-20 11:18:41 UTC
Created attachment 8791 [details]
Rendering with the blob
Comment 8 Oliver McFadden 2007-02-20 11:20:12 UTC
I should point out that both the R300 and blob render use the exact same vertex
and fragment programs.

The fragment program is just "MOV result.color, fragment.position;" and the
vertex program is the standard 4 component DP4 transform.
Comment 9 Rune Petersen 2007-02-21 11:16:25 UTC
Created attachment 8804 [details]
test app: fragment.position clipping issue

This might be the cause of your problem. or one of them.
Comment 10 Oliver McFadden 2007-02-21 12:09:56 UTC
Created attachment 8805 [details]
Another rendering with R300

This is another screenshot of rendering with R300, but from another angle. You
can see the color direction is wrong; it's going from white on the left into
yellow (x + y) on the right. This should be reversed; it should be going from
yellow on the left to white on the right. Perhaps a swizzle got mixed up
somewhere?

Also there is some other bug because of the magenta patch there. I'm not sure
why that happens. Also, weather this "kind of works" or is totally wrong (see
the other R300 rendering screenshot) seems to depend a lot on how you're looking
at the surface.
Comment 11 Oliver McFadden 2007-02-21 12:12:10 UTC
What should I test with the program you attached? What's the correct/incorrect
result?
Comment 12 Rune Petersen 2007-02-21 12:31:21 UTC
oops, use the up/down arrows...

move forward until the "walls" are clipped..
the colors are dependent on fragment.position.z, when the "walls" are clipped, fragment.position.z becomes wrong.
Comment 13 Oliver McFadden 2007-02-21 12:45:16 UTC
I see. That probably shouldn't happen.  I think something is seriously wrong with the instructions in insert_wpos. The strange thing is that when dumping the indirect buffers from the blob, it doesn't seem to do any RCP etc instructions. But this might not be reliable.  Also, did you see the second R300 screenshot I attached? I explained what's wrong in it's comments. 
Comment 14 Oliver McFadden 2007-02-21 12:47:08 UTC
Also, the fragment.position.z bug probably doesn't affect me, since I'm using it
as an argument to the TEX instruction thus only fragment.position.xy is used.

I guess it might affect the ones where I do "MOV result.color,
fragment.position;", though.
Comment 15 Rune Petersen 2007-02-21 13:39:37 UTC
xyz is most likely broken in my test app, I just use z an an indicator.

I myself have been unable to capture the indirect buffers, could you attach the output?
Comment 16 Oliver McFadden 2007-02-21 16:07:38 UTC
Created attachment 8809 [details]
Indirect buffers dump with iba-2

This is a dump of the indirect buffers when running tri-position on x86 with the
blob. It may or may not be accurate. I hacked iba-2 a bit based on a discussion
between me and aet on #dri-devel.  Mostly just calculating a few things
dynamically and making it run correctly for me.

The *.txt files are a result of running pretty_print_command_stream.tcl on the
binary dumps. The interesting files are "ib_0002_0370.txt" and
"ib_0004_0372.txt" and "nrta" as they are the only ones to contain fragment
program instructions; grep for "EASY_PFS_INSTR". They are virtually identical;
the later file contains an extra packet, but it shouldn't be important for what
we're looking at.
Comment 17 Rune Petersen 2007-02-22 04:28:31 UTC
the fragment.position is not calculated in the vertex program:

MUL     OUT: 0.xyzw (TEMP)
        ARG0 3.xyzw (PARM)      ARG1 0.wwww (ATTR)      ARG2 0.wwww
MUL     OUT: 1.xyzw (RESULT)
        ARG0 1.xyzw (ATTR)      ARG1 1.1111 (ATTR)      ARG2 1.1111
MAD     OUT: 0.xyzw (TEMP)
        ARG0 2.xyzw (PARM)      ARG1 0.zzzz (ATTR)      ARG2 0.xyzw (TEMP)
MAD     OUT: 0.xyzw (TEMP)
        ARG0 1.xyzw (PARM)      ARG1 0.yyyy (ATTR)      ARG2 0.xyzw (TEMP)
MAD     OUT: 0.xyzw (TEMP)
        ARG0 0.xyzw (PARM)      ARG1 0.xxxx (ATTR)      ARG2 0.xyzw (TEMP)
MUL     OUT: 0.xyzw (RESULT)
        ARG0 0.xyzw (TEMP)      ARG1 0.1111 (TEMP)      ARG2 0.1111
MUL     OUT: 2.xyzw (RESULT)
        ARG0 0.xyzw (TEMP)      ARG1 0.1111 (TEMP)      ARG2 0.1111

could you do a capture of something simple like tri-mov, in the hopes I can track down what they are doing.
Comment 18 Oliver McFadden 2007-02-22 11:52:15 UTC
Have a look at the ib_*.txt files, those should be the fragment program, but I still didn't see anything like RCP or anything there...  I can dump tri-mov if that's useful, too. I'll do that soon. 
Comment 19 Rune Petersen 2007-02-22 12:26:42 UTC
I did capture fp when I did the code (didn't use it)
If you look at: R300_PFS_INSTR2_1 the instruction is ?? should be RCP

I need the dump of tri-mov in order to isolate what blob does for fragment program.
Comment 20 Oliver McFadden 2007-02-22 14:17:49 UTC
Created attachment 8819 [details]
tri-mov indirect buffers
Comment 21 Oliver McFadden 2007-02-22 14:21:34 UTC
Also, the modified iba-2 is at http://z3ro.name/reveng/iba-2 if you want to
download it. I've modified it per the discussion between me and aet on
#dri-devel, and the defines at the top of the file are documented. You might
need to change those defines for your computer.

I tested this on x86 with AGP only. I doubt it works on x86_64 or PCI-E.

I'm still willing to make any dumps you need, just adding this comment in case
you or someone else wants the modified code.
Comment 22 Oliver McFadden 2007-02-22 14:26:33 UTC
Created attachment 8820 [details]
diff between the dumps of tri-mov and tri-position
Comment 23 Rune Petersen 2007-02-23 12:03:57 UTC
It looks pretty straight forward.

in stead of doing the calculations in the vertex shader, pass the value straight to the fragment shader and calculate there.

It will take some time, there isn't a framework for changes to the fragment program as there is for the vertex program.
Comment 24 Oliver McFadden 2007-02-23 15:05:01 UTC
So the instructions that are added to the vertex program are correct, but because they are in the vertex program they generate some errors in the result?  I guess the same kind of instruction insertion would work in the fragment program. There is already a mechanism to pass the number of the texcoord containing the data that's needed from the vertex program into the fragment program, so it shouldn't be too hard... I think. 
Comment 25 Oliver McFadden 2007-02-23 15:07:14 UTC
Oh, I see... The fragment program just sets the texcoord number for the vertex
program to use. It doesn't really have the ability to insert instructions yet.
Obviously that would have to be done first.
Comment 26 Rune Petersen 2007-02-24 09:38:49 UTC
Created attachment 8837 [details] [review]
wpos in FP hack

I have managed to hack together a patch that works correctly for the test app I made. how does it look on your end?

This is just a PoC.
Comment 27 Oliver McFadden 2007-02-24 16:35:37 UTC
With that patch it seems that the fragment.position is just completely yellow when written to result.color, which isn't correct. It should be yellow fading into white.  I guess this is a good start though, getting the position calculation into the fragprog. 
Comment 28 Rune Petersen 2007-02-24 16:48:10 UTC
please attach a screenshot, I need to look at the RGB levels.
if you could make it the same angle as in the blob rendering it would be easier.
Comment 29 Oliver McFadden 2007-02-24 17:15:18 UTC
I think that first time when the fragment.position was completely yellow was
just some freak R300 error, which does happen sometimes... I've seen a number of
such errors on various things.

Anyway, I've cleaned my Mesa tree and recompiled, and it does seem to get the
white to yellow gradient now, however there are some problems.

When looking from left to right you see the correct result (yellow to white),
however when looking right to left you see the wrong result (yellow to white,
too); it should be white to yellow.

I'll attach some screenshots, that should make it clear.
Comment 30 Oliver McFadden 2007-02-24 17:16:00 UTC
Created attachment 8841 [details]
Looking from the left to the right
Comment 31 Oliver McFadden 2007-02-24 17:17:11 UTC
Created attachment 8842 [details]
Looking from the right to the left
Comment 32 Oliver McFadden 2007-02-24 17:20:57 UTC
Created attachment 8843 [details]
Rendering with R300 and the latest patch

Here's a screenshot of the rendering with R300 and the latest patch. You can see
it's not correct, but I think it's better than before.
Comment 33 Oliver McFadden 2007-02-24 17:23:10 UTC
I found that tri-position does seem to generate completely yellow (no yellow to
white gradient) though.

If you like, I can make a screenshot of tri-position, too?
Comment 34 Rune Petersen 2007-02-25 03:09:34 UTC
the gradients look right to me, fragment.position is camera. So Z should be the distance from the camera. and in your case should always result in yellow close to the camera, and white far from the camera.

Of course I may be wrong again.
Does the blob really render it differently?
Comment 35 Oliver McFadden 2007-02-25 06:19:20 UTC
You're correct about the left vs right color, I confirmed this with the blob, too.  I'm not sure why this isn't rendering correctly with R300 then. It seems that it should be working... 
Comment 36 Rune Petersen 2007-02-25 06:37:31 UTC
we have only confirmed that Z in not completely wrong.

In order to confirm the X & Y, you need to get them in the 0.0-1.0 range.
Then send me a screenshot with blob & R300.
Comment 37 Rune Petersen 2007-02-26 11:50:05 UTC
I would still like you to confirm the X & Y values.

Do something like:
MUL result.color.xy, fragment.color.xy, {0.005}.x;

If you could send me a screenshot from r300 & blog, so I can compare....
Comment 38 Oliver McFadden 2007-02-26 19:35:17 UTC
I assume you mean "MUL result.color.xy, fragment.position.xy, {0.005}.x;" and not fragment.color.  I'm testing with "MUL result.color.xy, fragment.position.xy, {0.005}.x;" now, and I'll post the attach the screenshots after I'm done. 
Comment 39 Oliver McFadden 2007-02-26 19:42:57 UTC
Created attachment 8865 [details]
R300 fragment.position.xy

I had to use "MUL result.color.xy, fragment.position, {0.005}.x;" for the
program to successfully load (removed the .xy mask on fragment.position) but
this shouldn't make a difference since result.color is masked for .xy anyway.
Comment 40 Oliver McFadden 2007-02-26 19:53:10 UTC
Created attachment 8866 [details]
Blob fragment.position.xy

Ditto
Comment 41 Oliver McFadden 2007-02-26 19:59:00 UTC
I forgot to mention that the R300 screenshot is with your patch to calculate the
WPOS in the fragment program.
Comment 42 Rune Petersen 2007-02-27 06:43:09 UTC
The transition is too narrow try:
"MUL result.color.x, fragment.position, {0.0005}.x;"
"MUL result.color.y, fragment.position, {0.001}.x;"

Though I must say they look almost identical.
Could it be that fragment.position is now correct, but there is a texturing bug?

Is there a way for you to apply the texture in a similar way, but doesn't use fragment.position. It shouldn't look correct, I would just like to confirm if the texture is applied correctly.
Comment 43 Oliver McFadden 2007-02-27 07:23:51 UTC
I'm also starting to think that fragment.position is now correct, but there is some other bug. I can get somewhat correct rendering with R300 now. It is not completely correct, there is some bug because it seems the mirror direction is reversed, or perhaps it's some other problem.  I'll make a screenshot of the latest R300 rendering, and you can commit your latest patch. I think there might still be some work that needs to be done, but your patch certainly helps towards this goal 
Comment 44 Oliver McFadden 2007-02-27 07:36:41 UTC
Created attachment 8882 [details]
R300 fragment.position * 0.005

This is a screenshot of rendering with R300 with the following fragment program.
Bugzilla will probably destroy up the formatting here, but it should still be
readable.

MUL tmp, fragment.position, {0.005}.x;
TEX result.color, tmp, texture[0], 2D;

Ignore the fact that this isn't scaled properly, that's not the point; the point
is what you can see in the texture that is rendered.

If you compare this to the blob screenshot which I will attach next, you can see
the rendering is incorrect. I'm not exactly sure why yet. The blob screenshot is
the correct one. Both screenshots are generated by exactly the same code. The
only difference is the 3D driver.
Comment 45 Oliver McFadden 2007-02-27 07:37:16 UTC
Created attachment 8883 [details]
Blob fragment.position * 0.005
Comment 46 Oliver McFadden 2007-02-27 07:39:27 UTC
Also, both screenshots have been taken from exactly the same position so that
they can be compared easily.

You can see that the R300 screenshot is very different when compared to the blob
screenshot. I'm not sure what this means yet, but I suspect it's related to the
incorrect rendering with R300.
Comment 47 Rune Petersen 2007-02-27 08:35:09 UTC
a hunch:
in fragprog.c replace:
	fpi[i].SrcReg[1].File = PROGRAM_STATE_VAR;
	fpi[i].SrcReg[1].Index = window_index;
	fpi[i].SrcReg[1].Swizzle = MAKE_SWIZZLE4(SWIZZLE_X, SWIZZLE_Y, SWIZZLE_Z, SWIZZLE_ZERO);

	fpi[i].SrcReg[2].File = PROGRAM_STATE_VAR;
	fpi[i].SrcReg[2].Index = window_index;
	fpi[i].SrcReg[2].Swizzle = MAKE_SWIZZLE4(SWIZZLE_X, SWIZZLE_Y, SWIZZLE_Z, SWIZZLE_ZERO);

with:
	fpi[i].SrcReg[1].File = PROGRAM_STATE_VAR;
	fpi[i].SrcReg[1].Index = window_index;
	fpi[i].SrcReg[1].Swizzle = MAKE_SWIZZLE4(SWIZZLE_Z, SWIZZLE_Z, SWIZZLE_Z, SWIZZLE_ZERO);

	fpi[i].SrcReg[2].File = PROGRAM_STATE_VAR;
	fpi[i].SrcReg[2].Index = window_index;
	fpi[i].SrcReg[2].Swizzle = MAKE_SWIZZLE4(SWIZZLE_Z, SWIZZLE_Z, SWIZZLE_Z, SWIZZLE_ZERO);

A bit messy, I make a patch when I get home.
Comment 48 Oliver McFadden 2007-02-27 09:04:29 UTC
I think that change made it worse; now it's mostly a grey blob, probably from
the floor texture.
Comment 49 Rune Petersen 2007-02-27 09:46:27 UTC
in the same code:
try to replace the first set of SWIZZLE_Z with SWIZZLE_ONE
and replace the second set of SWIZZLE_Z with SWIZZLE_ZERO

This should make it a straight MOV command.
Comment 50 Oliver McFadden 2007-02-27 09:58:40 UTC
Didn't seem to help. Do you want a screenshot?  I also tried the "MUL result.color, fragment.position, {0.005}.x;", but it produces black. I didn't look at the RGB values, just looks black. 
Comment 51 Rune Petersen 2007-02-27 11:45:35 UTC
My theory is that the TEX instruction needs its parameters in a different range.

Lets try something different:
remove the last 2 changes, so you only have the "wpos in FP hack".
then do something like this:
MUL tmp.x, fragment.position, {0.0001}.x;
MUL tmp.y, fragment.position, {0.005}.x;
TEX result.color, tmp, texture[0], 2D;

Change the constants so it is scaled properly, don't think about offset.
Then report back the constants used, and the canvas dimensions.
Comment 52 Oliver McFadden 2007-02-27 21:02:22 UTC
Created attachment 8894 [details]
R300 fragment.position with correct range modification via MUL

This is a screenshot of R300 with fragment.position modified into the correct
range by a local parameter. There is no power-of-two adjust, though it shouldn't
be needed here.

fbufWidthScale = 1.0 / glConfig.vidWidth;
fbufHeightScale = 1.0 / glConfig.vidHeight;

You can see it kind of works. You can see it's almost as if the mirrored view is
behind the mirror object; the back of the mirror is the metal looking texture
seen in the mirror.

Exactly the same code works on the blob, so this is entirely a R300 problem, not
an error on my part. Not trying to point blame or anything like that, but it
means we can eliminate one thing. :)

Also, we must be doing something different than the blob, because if you look at
the screenshots of the unmodified fragment.position you can see the black gaps
are different sizes, etc. I think perhaps R300 might need to do some extra math
on fragment.position?

I think this might be close to working soon, though.
Comment 53 Oliver McFadden 2007-02-27 22:17:58 UTC
Created attachment 8896 [details]
R300 with the floor as a mirror

I just tried turning the floor into a mirror surface, and this seems to work
correctly on R300, even though the other mirror surface doesn't work on R300.

Maybe it's an orientation thing; something might go wrong with fragment.position
when the mirror is vertical relative to the view, but it works fine when the
mirror is horizontal relative to the view.

Although with the floor as a mirror, I could move around and change the view
angles without any noticeable rendering errors...

This is very strange.
Comment 54 Rune Petersen 2007-02-28 08:03:07 UTC
Created attachment 8905 [details] [review]
wpos for texure patch

your scales tells me that the range for X & Y is [0 1] meaning my original hunch was correct, could you try this patch, without scaling X & Y.

How to handle these two ranges properly in the driver I'll have to figure out.
Comment 55 Oliver McFadden 2007-02-28 18:50:22 UTC
I just tried your patch, and if I disable the scaling that I do manually in the fragment program (MUL tmp.xy, tmp, program.local[0];) it will work just as if I still had the scaling.  So basically the driver seems to do the scaling then, but I think this is wrong, with the blob I must manually scale the fragment.position with a MUL. Also, it seems a non-power-of-two adjust is needed for both drivers, so I'm applying that via a MUL now, too. I don't think the driver should do the scaling automatically; yes, it works for my purposes, but I'm not sure it's compliant with the specification.  If I only apply the first patch to move the fragment.position calculation into the fragment program, then I can do the scaling/non-power-of-two adjust myself and get almost prefect results. As I showed before, I can get the mirror to work correctly when it's on the floor, however the vertical mirror doesn't seem to work correctly.  On the vertical mirror it's almost as if it's looking from behind the mirror, which shouldn't happen. When I apply correct scaling/non-power-of-two adjust, it looks like the front of the mirror is the same as the back of the mirror. Both sides appear to have the back-of-mirror texture applied to them. The back of the mirror does have this texture applied to it, but the front of the mirror only gets this texture as a result of the mirroring. If this makes sense.  It's really weird. I can't explain how the texture or the fragment.position would cause it to appear to be looking from behind the mirror. So perhaps this is another non-fragment.position related bug?  Also, just letting you know that I will be away from Friday to Sunday, and possibly Monday (local timezone), but I'll try to reply as soon as possible. 
Comment 56 Oliver McFadden 2007-02-28 18:51:22 UTC
Bugzilla seemed to destroy the paragraphs on that last reply, sorry. :(
Comment 57 Oliver McFadden 2007-02-28 18:56:35 UTC
This is the same message as before, but with double newlines so that (hopefully)
bugzilla won't try to destroy the formatting. If it does, I guess someone can
delete this duplicate message.


I just tried your patch, and if I disable the scaling that I do manually in the
fragment program (MUL tmp.xy, tmp, program.local[0];) it will work just as if I
still had the scaling.


So basically the driver seems to do the scaling then, but I think this is wrong,
with the blob I must manually scale the fragment.position with a MUL. Also, it
seems a non-power-of-two adjust is needed for both drivers, so I'm applying that
via a MUL now, too. I don't think the driver should do the scaling
automatically; yes, it works for my purposes, but I'm not sure it's compliant
with the specification.


If I only apply the first patch to move the fragment.position calculation into
the fragment program, then I can do the scaling/non-power-of-two adjust myself
and get almost prefect results. As I showed before, I can get the mirror to work
correctly when it's on the floor, however the vertical mirror doesn't seem to
work correctly.


On the vertical mirror it's almost as if it's looking from behind the mirror,
which shouldn't happen. When I apply correct scaling/non-power-of-two adjust, it
looks like the front of the mirror is the same as the back of the mirror. Both
sides appear to have the back-of-mirror texture applied to them. The back of the
mirror does have this texture applied to it, but the front of the mirror only
gets this texture as a result of the mirroring. If this makes sense.


It's really weird. I can't explain how the texture or the fragment.position
would cause it to appear to be looking from behind the mirror. So perhaps this
is another non-fragment.position related bug?


Also, just letting you know that I will be away from Friday to Sunday, and
possibly Monday (local timezone), but I'll try to reply as soon as possible.
Comment 58 Oliver McFadden 2007-03-04 00:03:23 UTC
I think that you can commit this now. I think that the problem with the vertical mirror might be some other bug, like a texturing bug.  I believe that fragment.position is now correct; I'll have to look into why the vertical mirror doesn't work further and figure that out. :)  Btw, if it's not too much trouble, could you have a look at the KIL instruction on R300 bug? Bug #9996.  Thank you. 
Comment 59 Oliver McFadden 2007-03-06 02:47:16 UTC
I think I figured out why the vertical mirror isn't working. For some reason it's fragment.position (after appropriate scaling and non-power-of-two adjust) seems to be mostly blue/cyan...  From what I've seen previously with the blob, it should be yellow/white. I'll make some screenshots with the same angle for comparison soon. 
Comment 60 Oliver McFadden 2007-03-06 03:19:05 UTC
Created attachment 8995 [details]
Blob rendering the vertical mirror with correct XY masking.

This is the blob rendering the vertical mirror with correct masking, as in the
following program. The results are the correct ones because this will work
correctly when I use it with the TEX instruction.

!!ARBfp1.0

TEMP tmp;

MOV tmp.xy, fragment.position;
MUL tmp.xy, tmp, program.local[0];
MUL tmp.xy, tmp, program.local[1];
MOV result.color.xy, tmp;

END
Comment 61 Oliver McFadden 2007-03-06 03:21:57 UTC
Created attachment 8996 [details]
R300 rendering the vertical mirror with correct XY masking.

Same program as the previous comment, however this gives incorrect rendering on
R300. The previous screenshot (from the blob) shows the correct rendering
should be blue/cyan (ignore my initial comment saying otherwise) but this
screenshot is a green/yellow/red.
Comment 62 Oliver McFadden 2007-03-06 03:27:56 UTC
Created attachment 8997 [details]
R300 rendering the vertical mirror with INCORRECT XYZ masking.

This is R300 rendering the vertical mirror with incorrect XYZ masking on the
final result.color MOV, as in this program. Notice the result.color MOV.

!!ARBfp1.0

TEMP tmp;

MOV tmp.xy, fragment.position;
MUL tmp.xy, tmp, program.local[0];
MUL tmp.xy, tmp, program.local[1];
MOV result.color.xyz, tmp;

END

I'm not exactly sure what's going on, but the blob was defiantly using the XY
mask when I made it's screenshot. So it must be doing something differently.
I'll wait for your reply, Rune, as I'm not really sure what's going on.
Comment 63 Oliver McFadden 2007-03-06 03:31:32 UTC
I also did some more checking with just an X mask (result.color.x) and R300 generates a red color, while the blob generates a cyan color. So I think something is missing... 
Comment 64 Oliver McFadden 2007-03-15 06:38:19 UTC
I just made some more screenshots with R300, and it seems there are some cases where something goes wrong depending on the view angle. I made 6 screenshots that I'll attach they are at various different angles showing errors in the fragment.position calculation.  Note that I am doing a MUL for scale/npot adjust, but I'm doing the same thing with the blob too, and it works there, so it is a bug with the fragment.position calculation. 
Comment 65 Oliver McFadden 2007-03-15 06:38:57 UTC
Created attachment 9157 [details]
Screenshot 1
Comment 66 Oliver McFadden 2007-03-15 06:39:13 UTC
Created attachment 9158 [details]
Screenshot 2
Comment 67 Oliver McFadden 2007-03-15 06:39:28 UTC
Created attachment 9159 [details]
Screenshot 3
Comment 68 Oliver McFadden 2007-03-15 06:39:42 UTC
Created attachment 9160 [details]
Screenshot 4
Comment 69 Oliver McFadden 2007-03-15 06:39:58 UTC
Created attachment 9161 [details]
Screenshot 5
Comment 70 Oliver McFadden 2007-03-15 06:40:13 UTC
Created attachment 9162 [details]
Screenshot 6
Comment 71 Oliver McFadden 2007-03-15 06:44:33 UTC
Rune, if you're around and need any more information or dumps etc please let me
know. :)
Comment 72 Oliver McFadden 2007-03-15 06:59:03 UTC
Oops, I might have made a mistake with which patches I had applied to that Mesa
tree. I think that the fragment.position patch wasn't applied. I'm going to go
confirm that now, so possibly disregard the last 6 screenshots.

Sorry about that...
Comment 73 Oliver McFadden 2007-03-15 07:07:01 UTC
The last 6 screenshots were incorrect, sorry about that. I just made two new
screenshots on R300 (checked the fragment.position patch was applied this time)
and the blob.

The blob screenshot is mostly blue while the R300 screenshot is mostly green;
this is probably the source of my problem. It is still probably something wrong
in the fragment.position calculation.

I'll attach the two screenshots and the fragment program.
Comment 74 Oliver McFadden 2007-03-15 07:08:05 UTC
Created attachment 9163 [details]
Blob rendering parallel to the mirror (good)
Comment 75 Oliver McFadden 2007-03-15 07:08:34 UTC
Created attachment 9164 [details]
R300 rendering parallel to the mirror
Comment 76 Oliver McFadden 2007-03-15 07:10:02 UTC
Created attachment 9165 [details]
The fragment program used with the last two screenshots
Comment 77 Oliver McFadden 2007-03-15 09:38:46 UTC
I just committed the fragment.position patch and a few minor corrections. There is still a bug with the swizzle, though... But it is a significant improvement. 
Comment 78 Rune Petersen 2007-03-15 11:50:25 UTC
Do you get the same result if you do:
!!ARBfp1.0

TEMP tmp;

MOV tmp.xy, fragment.position;
MUL tmp.xy, tmp, program.local[0];
MUL tmp.xy, tmp, program.local[1];
MOV result.color.z, {0.0}.x
#TEX result.color, tmp, texture[0], 2D;
MOV result.color.xy, tmp;

END

z = blue, and result.color.z is not written to. to me is makes sense that components not written to are: x=0, y=0, z=0, w=1

(i'll be participating casually for the time beeing)
Comment 79 Oliver McFadden 2007-03-15 12:02:30 UTC
Doing "MOV result.color.z, {0}.x;" doesn't change anything; still the same incorrect mostly green color.  However, doing "MOV result.color.z, {1}.x;" does seem to produce the correct mostly blue color, at least from a visual inspection. I didn't mathematically compare the RGB values or anything like that.  So maybe the final swizzle is wrong? Maybe the blob fills this in as 1? 
Comment 80 Rune Petersen 2007-03-15 12:38:12 UTC
In other words I was right =)

The question is how what value should result.color.z have if not written to.
I know the blob does a MOV to result.color but it is my belief that z should be 0 if not writen to, making the blob wrong. 
Comment 81 Rune Petersen 2007-03-15 12:44:59 UTC
looks like neither case is wrong.

From the spec:
  (7) If a fragment program does not write a color value, what should
    be the final color of the fragment?

      RESOLVED: The final fragment color is undefined.  Note that it may
      be perfectly reasonable to have a program that computes depth 
      values but not colors.  Fragment colors are often irrelevant if
      color writes are disabled (via ColorMask).
Comment 82 Oliver McFadden 2007-03-15 13:22:40 UTC
So with the blob, when I say "MOV result.color.xy, fragment.position;" it really translates that into "MOV result.color.xyz, fragment.position;"; because R300 doesn't do this I get get the mostly green color instead of mostly blue?  It's odd then that doing a TEX with result.position doesn't work on R300, but works on the blob. Since TEX only uses .xy I would expect it to work on R300; it doesn't even look at .z. =/ 
Comment 83 Rune Petersen 2007-03-15 13:37:48 UTC
no the blob does something like this:
"MOV result.color, fragment.color;"
"MOV result.color.xy, fragment.position;"
Comment 84 Oliver McFadden 2007-03-15 13:42:09 UTC
Hmm... Well I'm not sure why my TEX instruction isn't working then, all indications are that it should work. 
Comment 85 Rune Petersen 2007-03-15 14:29:08 UTC
just to be clear it is only curtain that is messed up right?

The "colortest": If you set result.color.z to 1.0, are colors the same/similar for both blob & R300?

If yes, could you experiment setting tmp.z & tmp.w to different values and see if the result of TEX changes.

or maybe try writing to result.color before calling TEX:
      "MOV result.color, {1.0}.x;" 
Comment 86 Oliver McFadden 2007-03-15 15:08:15 UTC
Yes, that's right. Setting result.color.z to 1 makes our driver output the same blue color (at least from visual comparison) as the blob.  I'll try some of the things you mentioned now. 
Comment 87 Oliver McFadden 2007-03-15 15:15:42 UTC
No change when using fragment.position for the TEX lookup and adding one of "MOV
tmp.z, 1.0;", "MOV tmp.z, 0.0;", "MOV tmp.w, 1.0;", "MOV tmp.w, 0.0;" or "MOV
result.color, {1.0}.x" before the "TEX result.color, tmp, texture[0], 2D;"

Maybe it's something in emit_tex? Just a stab in the dark; maybe you have a
better idea.
Comment 88 Oliver McFadden 2007-03-15 15:17:24 UTC
Just to be clear, the testing program would look something like:

!!ARBfp1.0
TEMP tmp;
MOV tmp.xy, fragment.position;
MUL tmp.xy, tmp, program.local[0];
MUL tmp.xy, tmp, program.local[1];
# XXX: Test instruction here.
TEX result.color, tmp, texture[0], 2D;
END
Comment 89 Oliver McFadden 2007-03-23 09:28:25 UTC
I've been taking to Nicolai Haehnle (nh) about this and he has made an OpenGL
test suite for verifying the DRI drivers. The example page is the results of it
running on R300.

http://people.freedesktop.org/~nh/piglit/

http://people.freedesktop.org/~nh/piglit/sample/index.html

There is a test on that page for fragment.position there. You can see it fails.
I think this might be the reason I still have some problems with
fragment.position.
Comment 90 Rune Petersen 2007-03-24 09:33:39 UTC
The test appear to show 2 different issues:

In basic & tex2d unscaled: 
     all deltas with the exception of 0.001471 can be constructed as n * 0.000980??
     E.g. 0.003922 = 4 * 0.000980?? and 0.007843 = 8 * 0.000980??
     We need to track down where this deviation comes from.

In tex2d scaled & texrect:
     (will most likely also suffer from the )
     the fragment shader is broken when it comes to using TEX instructions (Oliver you just work around the problem the tests most likely doesn't)
     When I feel better I see about fixing it.
Comment 91 Oliver McFadden 2007-03-24 10:31:38 UTC
I'll grab the values from running fp-fragment-position from Piglit so we can compare them between R300 and the blob.  Thanks for looking into this, and I do hope you feel better soon; not just so you can fix this. ;) 
Comment 92 Oliver McFadden 2007-03-24 10:49:15 UTC
Created attachment 9274 [details]
Blob results for Piglit test
Comment 93 Oliver McFadden 2007-03-24 10:49:34 UTC
Created attachment 9275 [details]
R300 results for Piglit test
Comment 94 Oliver McFadden 2007-03-24 10:50:54 UTC
Created attachment 9276 [details] [review]
Diff of the previous two attachments
Comment 95 Oliver McFadden 2007-03-24 10:56:00 UTC
The diff of the blob vs R300 has me a little concerned. I would have expected
the delta to be exactly the same on both of them. Perhaps a little difference...

tex2d scaled #1 (0.2,1.2): 0.976471,0.607843,0.027451,0.313726 (blob)
tex2d scaled #1 (0.2,1.2): 0.600000,0.309804,0.403922,0.352941 (r300)

But not that much...
Comment 96 Rune Petersen 2007-04-04 12:28:45 UTC
the r300 now passes the fp-fragment-position test from Piglit
Nicolai fixed it the "easy" way and I didn't even see it.

Anyway we can always work on optimizations later.
Comment 97 Nicolai Hähnle 2008-06-15 01:42:19 UTC
It appears to me that this bug has been fixed for over a year, so I'm closing it. If there are still problems with fragment.position interaction with other stuff, it's most likely a separate bug in fragment program compilation and merits a new bug report.
Comment 98 Adam Jackson 2009-08-24 12:25:58 UTC
Mass version move, cvs -> git

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.