Bug 2241

Created attachment 1643 [details] [review]
drm_radeon_cube_20050108.diff.txt

Patch for radeon kernelmodule.

Created attachment 1644 [details] [review]
mesa_radeon_cubemap_20050108.diff.txt

experimental support for GL_ARB_texture_cube_map for radeon mesa driver.

+ cubemap demo works
+ http://frustum.org/3d/index.php?demo=8 works

- mipmap is broken/omitted
- texgen is a tcl fallback
- TexCoord3 is a vfmt fallback
- contains lots dead code

I have applied the drm part to cvs (together with a texture micro tiling so they
can have the same drm minor number), together with the corresponding sanity code
pieces. I'm afraid, the rest is a bit too much for me to review/commit,
especially since I don't have too much time testing on r100. It would definitely
be nice to have though.

Created attachment 1981 [details] [review]
mesa_radeon_cubemap_20050227.diff

patch against current mesa cvs.
unfortunately textures are broken, might be some interaction with the
texture-tiling patches. I might check that (much) later...

@roland:
thanks for committing the drm part.

Created attachment 1986 [details] [review]
mesa_radeon_cubemap_20050228.diff

There was indeed some interaction with tiling, exactly the same as was with
r200. The fixed BLIT_WIDTH y coord hack won't work. New fix, very quick testing
shows the inner sphere is correct, but the outer box is completely wrong,
unless sw tcl is used.
btw I'm not sure why cube maps would be limited to 512x512, though if there
were bugs with larger maps before, this was likely because of the same y coord
hack (which could cause the y coord to exceed the limit of the blitter, the
r200 driver had a workaround for that, though this is now gone with tiling
since it's no longer needed).

ut2003_demo shows at least two problems with this patch:
- missing textures (citadel and antalus ground texture)
- z fighting in citadel "danger balls", maybe because of tcl/texgen fallback

Created attachment 2830 [details] [review]
mesa_radeon_cubemap_20050605_3.diff.txt

patch against current Mesa CVS HEAD

Created attachment 2881 [details]
kernel.config.optiplex

cubemap demo works, even if using new packets and maos_arrays.
projtex (still) works.
ut2003_demo antalus looks ok, at least better than before ;)
ut2003_demo citadel looks ok in sw-tnl mode only.

I don't know if the change is a real fix or only partially working:
ut2003_demo seems to send 3 texcoords when 2d texturing and so a 0 was written
to the q-coord.
Could this (submitting 3 texcoords) be the trigger for a tcl-fallback in the
radeon driver?

Is ctx->Texture.Unit[x]._ReallyEnabled up to date when radeonSetVertexFormat()
is called ?

Created attachment 2935 [details] [review]
change-default-dri-driver-dir-2.patch

with this patch it is also possible to use the 3rd TMU when configured with
driconf. (default: use only 2 TMUs)

ut2003_demo citadel looks now ok with hw-tcl.
Unfortunately sometimes false textures are being used.

Created attachment 3164 [details] [review]
mesa_radeon_cubemap_3tmu_20050903.diff.txt

(In reply to comment #10)
> mesa_radeon_cubemap_3tmu_20050903.diff.txt
Just a sidenote, ut2k3 emits 3 coords because it uses projective texturing (for
instance antalus map tree shadows), it will simply use the texture matrix to
swap r and q coords (saves some vertex size that way). Thus you can't simply
only submit 2 coords in that case, the code is quite broken I think wrt emitting
tex coords in that area. I'm not quite sure how the coord selection works, on
r200 there is an explicit selection if you want to use the 3rd or 4th texture
coordinate for the texture AGU, which isn't present on r100 it seems. I'd
suspect it just always uses coord number 3, and unlike the r200, it looks like
you can't even submit all 4 coords, which makes this probably hard to work for
all cases (actually, impossible for hw tcl). Maybe if an app uses 3 coords for a
2d texture, just assume it will later use the texture matrix to calculate a q
coord. The texture matrix would probably need some adjustments to take this into
account (e.g. 3rd and 4th line need to be swapped if my math is correct, so that
the final q coord output is the 3rd coord. Actually, I think the current code is
broken when 4 coords are supplied too, since when you submit the 4th coord as
the 3rd, you'd need to swap columns 3 and 4 of texmat I think, followed by
swapping lines 3 and 4 to get the 4th coord out as the 3rd - not doing any
swapping at all will only work for "simple" matrices.
There are more problems with texmat, as far as I can tell the argument order is
reversed when doing the _math_matrix_mul_matrix in update_texturematrix (this
was fixed in r200, should be the same in radeon).
I think the swapping needed for texture matrix is probably the reason that q
coord texgen is not supported currently.

Created attachment 3515 [details] [review]
mesa_radeon_cubemap_3tmu_20051008.diff.txt

here is another update, I changed the emit code in radeon_swtcl.d a bit to emit
the q-coord always if more than 2 texture-coords are available.
Since Roland added texgen for q-coord, cubemap demo also works with hw-tcl.
Unfortunately ut2003_demo looks better with tcl_mode=0, especially the
reflections on water.

(In reply to comment #12)
> Created an attachment (id=3515) [edit]
> mesa_radeon_cubemap_3tmu_20051008.diff.txt
> 
> here is another update, I changed the emit code in radeon_swtcl.d a bit to emit
> the q-coord always if more than 2 texture-coords are available.
I've commited the 3tmu part of it (with some changes).
As for cube maps, I don't think you can make mipmaps work. ATI's windows driver
does not expose mip/aniso filtering for 3d and cube maps according to dx caps
viewer. Don't know if this is a chip bug or rather a limitation (in the first
case it could potentially work with rv100/rv200, I've got a r100).
I think though it would be easy to prevent the mipmaps from being uploaded and
wasting texture memory (e.g. skip the driCalculateTextureFirstLastLevel
calculation in radeonSetTexImages).

> Since Roland added texgen for q-coord, cubemap demo also works with hw-tcl.
> Unfortunately ut2003_demo looks better with tcl_mode=0, especially the
> reflections on water.
Is something else apart from the water wrong? I've never had much luck with the
water reflections (texgen, if it worked before that would have been because of a
tcl fallback), the r200 couldn't handle it neither (both dri driver and fglrx).
It should be noted the same reflections with ut2k4 work just fine (with r200,
dri driver) so ut2k3 is probably doing something strange there.
Some errors in ut2k3 are also because the game needs 4 texture units sometimes
(for instance the quad damage, should look the same though with sw tcl).

I've looked at the cube map part and some things don't quite work well. In
particular, 3 tmus and cube maps don't mix too well. Apparently, we're hitting
some hardware limitations there. For instance, if you run ut2k3 with 3 tmus, it
will look very wrong, and after that you're stuck with completely broken mipmaps
until you reboot! I've hunted this down, it's because the
TXFORMAT_CUBIC_MAP_ENABLE bit is still set, and if this unit is not enabled, it
will never get unset - this breaks mipmapping on either unit 0 or 1 (or both,
dunno), despite that the unit isn't actually enabled. That could obviously be
fixed, but I'd bet it doesn't work if the unit is enabled neither...
I'm wondering if the 3tmu patch was actually correct, can the radeon do
trilinear filtering simultaneously on all units? And if not, on which units can
it do filtering involving mipmaps simultaneously? And what about trilinear on
some units + cube mapping on others?

Created attachment 3556 [details] [review]
new cube map patch

newer version of the patch, I've dropped all the non-working vtxfmt changes for
now. maos_arrays and maos_verts path could need some adjustments I think
(potentially lots of unnecessary matrix uploads when the texture target is a
cube map). And apparently something needs to be done about the interaction with
other tex units / mipmapping...

commited to cvs. I wasn't sure if the cubic_map_enable bit on unit 2 MUST be
unset if the unit is disabled on other than original r100 chips (looks like a
chip bug to me), but the workaround is probably not really degrading
performance. No vtxfmt code was commited, too many "this doesn't work
correctly...". I think as a first step it would be useful to get "standard"
vtxfmt working without the fancy codegen. That's true for r200 too, btw - while
the code there is indeed simpler (no texture coord swapping hacks needed) and
already handles (multi)texcoord3x, it doesn't handle texturing with projected
coords neither.
I'm not sure optimizing the one-by-one vertex emit path is worth it though, apps
interested in performance will use something else (drawArrays for instance) anyway.

Thanks for finishing this!

Roland, ok to close this one?

(In reply to comment #18)
> Roland, ok to close this one?
Yes, that's ok. vtxfmt has other fallbacks too and I'm not going to touch it
just for that - noone is interested in immediate mode performance anyway.

Mass version move, cvs -> git