Bug 6412 - mach64 vertex buffer cleanup
mach64 vertex buffer cleanup
Status: RESOLVED WONTFIX
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/Mach64
git
x86 (IA32) Linux (All)
: high normal
Assigned To: Default DRI bug account
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-28 08:59 UTC by George -
Modified: 2009-08-24 12:23 UTC (History)
0 users

See Also:


Attachments
patch for mach64 DRI (15.52 KB, patch)
2006-03-28 09:00 UTC, George -
Details | Splinter Review
t_dd_vb.c for mach64 (9.32 KB, patch)
2006-03-28 09:01 UTC, George -
Details | Splinter Review
t_dd_vbtmp.h for mach64 (19.15 KB, patch)
2006-03-28 09:01 UTC, George -
Details | Splinter Review
big patch for mach64 DRI (34.25 KB, patch)
2006-03-28 12:07 UTC, George -
Details | Splinter Review
big patch for mach64 DRI - try 2 (43.96 KB, patch)
2006-03-29 04:05 UTC, George -
Details | Splinter Review
patch to tnl_dd_vb.c (2.04 KB, patch)
2006-03-29 04:15 UTC, George -
Details | Splinter Review
big patch for mach64 DRI - try 3 (43.21 KB, patch)
2006-03-30 10:18 UTC, George -
Details | Splinter Review
big patch for mach64 DRI - try 4 (42.19 KB, patch)
2006-04-02 01:00 UTC, George -
Details | Splinter Review
big patch for mach64 DRI - try 5 (42.70 KB, patch)
2006-04-03 01:11 UTC, George -
Details | Splinter Review
use t_vertex.c - try 1 (20.59 KB, patch)
2006-04-05 05:54 UTC, George -
Details | Splinter Review
use t_vertex.c - try 2 (49.31 KB, patch)
2006-04-06 08:49 UTC, George -
Details | Splinter Review
use t_vertex.c - try 3 (50.02 KB, patch)
2006-04-07 07:44 UTC, George -
Details | Splinter Review
use t_vertex.c - try 4 (51.88 KB, patch)
2006-04-13 06:19 UTC, George -
Details | Splinter Review
use t_vertex.c - try 5 (51.32 KB, patch)
2006-05-24 04:22 UTC, George -
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description George - 2006-03-28 08:59:21 UTC
mach64 is unique is having two implementations of VB, one that uses the
"template" vertex format and one that uses the "native" vertex format.

The "template" implementation uses the mesa provided t_dd_vb.c. However, 
it still provides its own version of t_dd_vbtmp.h (mach64_vbtmp.h) and 
adds complex draw functions in mach64_tris.c for conversion to native format.

The "native" implementation has simple draw functions. However, it performs
conversion to native format during emit (mach64_native_vbtmp.h) and back to
template format in translate (mach64_native_vb.c), these functions differ
greatly from the tnl_dd templates.

The attached patches have yet another implementation of the mach64 VB.
They define mach64Vertex as a struct that matches the native vertex format,
this allows to use both the mesa templates with very little changes and 
the simple draw functions.

The quirks are:
* we still have to provide slightly modified versions of templates t_dd_vb.c
  and t_dd_vbtmp.h, however the changes to t_dd_vb.c are minimal and can be
  dropped with another iteration of cleanups if the patches are accepted.

* they drop MACH64_PREMULT_TEXCOORDS, it did not seem to make a difference
  in performance.

* they push conversion to little-endian down to the draw functions, these
  changes are untested as I have x86.
Comment 1 George - 2006-03-28 09:00:24 UTC
Created attachment 5096 [details] [review]
patch for mach64 DRI
Comment 2 George - 2006-03-28 09:01:01 UTC
Created attachment 5097 [details] [review]
t_dd_vb.c for mach64
Comment 3 George - 2006-03-28 09:01:33 UTC
Created attachment 5098 [details] [review]
t_dd_vbtmp.h for mach64
Comment 4 Felix Kühling 2006-03-28 10:06:21 UTC
I haven't looked at your patches yet ... You may want to take a look at the
t_vertex.c based code in e.g. the Savage driver. Template-based vertex setup
code in some drivers has been replaced with the t_vertex-based code. If it is
flexible enough to generate mach64 native vertices that may be beneficial in
terms of binary and source code size, though not necessarily performance.
Comment 5 George - 2006-03-28 12:07:48 UTC
Created attachment 5100 [details] [review]
big patch for mach64 DRI

This patch does not add files, instead it merges the changes to existing files.


Specifically, it incorporates the changes for NATIVE_VTXFMT in mach64_vbtmp.h
which would only be used for the template format. mach64_vbtmp.h was derived
from t_dd_vbtmp.h v3.5 while the changes for native format are derived 
from t_dd_vbtmp.h v5.0.1 (current). Thus the changes for mach64_vbtmp.h are
those between the different versions of t_dd_vbtmp.h and the additions for the
native format (patch 5098).

As a side effect, it fixes the mach64 driver when compiled with the template
format (currently it shows a black screen).
Comment 6 George - 2006-03-28 12:12:41 UTC
(In reply to comment #4)
> I haven't looked at your patches yet ... You may want to take a look at the
> t_vertex.c based code in e.g. the Savage driver. Template-based vertex setup
> code in some drivers has been replaced with the t_vertex-based code. If it is
> flexible enough to generate mach64 native vertices that may be beneficial in
> terms of binary and source code size, though not necessarily performance.

I'll have a look at it,
thanks for the pointer.
Comment 7 George - 2006-03-29 04:05:06 UTC
Created attachment 5106 [details] [review]
big patch for mach64 DRI - try 2

this version of the patch:
* finishes the changes for LE32 (which are untested because of I have x86)
* drops the xy variables in the draw functions and uses symbolic names v.x, v.y


Other than bug fixes, I do not plan to do other changes in the near future.
Comment 8 George - 2006-03-29 04:15:00 UTC
Created attachment 5109 [details] [review]
patch to tnl_dd_vb.c

This is just some casts to GLfloat in print_vertex.
Comment 9 George - 2006-03-30 10:18:58 UTC
Created attachment 5126 [details] [review]
big patch for mach64 DRI - try 3

merge some codepaths for "native" and "template".

I looked at t_vertex.c, it seems to use extensively byte offsets from the
template vertex format which does not fit very well with the mach64 native
vertex format. If we were to use it, we would also have to use the "complex"
draw functions in mach64_tris.c
Comment 10 George - 2006-04-02 01:00:35 UTC
Created attachment 5155 [details] [review]
big patch for mach64 DRI - try 4

fix C version of DO_COPY_VERTEX to work on LE, by inspection it should also
work on BE.

don't drop the PREMUL_TEXCOORDS code, just disable by default since it seems to
marginally hurt performance.
Comment 11 George - 2006-04-03 01:11:40 UTC
Created attachment 5164 [details] [review]
big patch for mach64 DRI - try 5

Set pv.q1 correctly, fixes regression for tests/projtex.
Enable PREMULT_TEXCOORDS by default, rumored to be for multitexturing.
Comment 12 George - 2006-04-05 05:54:21 UTC
Created attachment 5198 [details] [review]
use t_vertex.c - try 1

Start over.

Leave native vertex format implementation in peace. Port the template vertex
format implementation over to tnl from tnl_dd.
Comment 13 George - 2006-04-06 08:49:32 UTC
Created attachment 5206 [details] [review]
use t_vertex.c - try 2

this is pretty much as far as I can go without additions to t_vertex.c
Comment 14 George - 2006-04-07 07:44:00 UTC
Created attachment 5209 [details] [review]
use t_vertex.c - try 3

quasi-native vertex format.

we use the following hack in attribute emission:

   /* The TNL module requires that the first attribute is _TNL_ATTRIB_POS, 
    * at the same time, the mach64 native vertex format has the XYZW coords 
    * at non-consecutive positions spread inside the vertex.
    *
    * We reserve space for the XYZW coords with EMIT_PAD's and when the vertex 

    * is copied to the vertex buffer for submission to the card, we use the 
    * _TNL_ATTRIB_POS values to fill in the pads reserved for XYZW and discard 

    * the first 4 bytes of the vertex.
    */
Comment 15 George - 2006-04-13 06:19:26 UTC
Created attachment 5282 [details] [review]
use t_vertex.c - try 4

port to new RENDERINPUTS macros
Comment 16 George - 2006-05-24 04:22:26 UTC
Created attachment 5723 [details] [review]
use t_vertex.c - try 5

Summary:

For the native vertex format (default and faster), this patch only touches
mach64FastRenderClippedPoly() to fix a problem I'm currently seeing with the
native format.

For the template vertex format (optional and slower), it replaces the current
non-working code with code based on t_vertex.c. It kills more than 1KLoC. It is
untested on BE. So far, I have only seen the following regression with regard
to the native format: in tests/texgenmix, the first texture with 3 coords is
not drawn the first time it is mapped.

Anyboby cares to test/review ?
Comment 17 George - 2006-10-07 01:37:52 UTC
This bug has served well in understanding mach64 vertex setup but I don't think
it is clean enough to apply.

Possible outcomes of this bug report:

* mach64 has to be compiled with -fno-strict-aliasing to run ok. The culprit
  for this is mach64FastRenderClippedPoly() in mach64_tris.c:1580. Either
  drop the optimized version of that function or see patch for an alternative.

* The non-native vertex buffer does not currently work and I don't know how
  well it serves as a source of documentation. It could propably be dropped.

Closing.
Comment 18 ajax at nwnk dot net 2009-08-24 12:23:46 UTC
Mass version move, cvs -> git