Re: R300 cleanup questions

[Oliver M. <z3r...@gm...>]

I'd like some input on the VBO stuff in r300. In r300_context.h we have the
following.

/* KW: Disable this code.  Driver should hook into vbo module
 * directly, see i965 driver for example.
 */
/* #define RADEON_VTXFMT_A */
#ifdef RADEON_VTXFMT_A
#define HW_VBOS
#endif

So the VTXFMT (radeon_vtxfmt_a.c) code is disabled anyway. This also disables
hardware VBOs. I guess this has been done since the new VBO branch was merged.

So, the question is, should this "dead" code be removed? I think all drivers are
(or should be) moving to the new VBO code anyway.

I've already made a patch for this, but I'm not committing until I get the okay
from a few people.

Thanks.


On 5/9/07, Oliver McFadden <z3r...@gm...> wrote:
> I also think we might need to add _dri_warning/_dri_error because the _mesa
> versions output "Mesa warning: %s" which implies to the user this is a Mesa
> problem, not a DRI driver problem.
>
> I could add r300Warning and r300Error, but probably all DRI drivers need
> warning
> and error functions... So maybe adding them to the common DRI code?
>
>
> On 5/9/07, Oliver McFadden <z3r...@gm...> wrote:
> > Hi,
> >
> > I added the "not implemented yet" comment back, although there are other
> > places
> > that use 65535 so it could be some kind of hardware limit...
> >
> > The only reason that I went with "camel case" r300FooBar names is because
> > that's
> > what 90% of the driver uses; it's easier to change a few r300_foo_bar to
> > r300FooBar than the other way around. The important thing is it's
> > consistent.
> >
> > Now I just hope I don't get shot for all the commits. ;)
> >
> >
> > On 5/9/07, Brian Paul <bri...@tu...> wrote:
> > > Jerome Glisse wrote:
> > > > On 5/8/07, Christoph Brill <eg...@gm...> wrote:
> > > >> I reviewed the cleanup done by Olliver McFadden and had the following
> > > >> questions:
> > > >>
> > > >> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >>
> > > >> Is it necessary/usefull that the function is static?
> > > >
> > > > I think it's better to have static function, i am thinking of symbol
> > > export and
> > > > other things like that.
> > >
> > > Yes, make functions static whenever possible.
> > >
> > >
> > > >> -/* Immediate implementation has been removed from CVS. */
> > > >> -
> > > >> -/* vertex buffer implementation */
> > > >> -
> > > >> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
> > > >> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long
> addr
> > > >>
> > > >> Why move all the comments to the head of the file. IMO the method
> > should
> > > >> have a doxygen comment that states it is the vertex buffer
> > > >> implementation of fire_EB, right?
> > > >>
> > > >>
> > > >> -                if (num_verts > 65535) {      /* not implemented yet
> > */
> > > >> +                if (num_verts > 65535) {
> > > >>
> > > >> Comments like this should be kept. Otherwise it looks like a hardware
> > > >> limitation while the limitation can be worked around or the
> limitation
> > > >> does not exist.
> > > >>
> > > >>
> > > >> Last but not least is
> > > >> r300_foo_bar
> > > >> preferred or
> > > >> r300FooBar
> > > >> Which is the one mesa uses?
> > > >
> > > > We can use the one we like, i prefer r300_foo_bar over r300FooBar
> which
> > > > i dislike but the choice is up to the first person who do big cleanup
> :)
> > > and
> > > > we do not have to conform to mesa coding style for driver but use the
> > one
> > > > we like the more.
> > >
> > > Yes, core Mesa has a fairly consistant naming scheme but it's the
> > > prerogative of the driver writers to choose their style.  That said,
> > > naming within each driver should be consistant.
> > >
> > > -Brian
> > >
> > >
> -------------------------------------------------------------------------
> > > This SF.net email is sponsored by DB2 Express
> > > Download DB2 Express C - the FREE version of DB2 express and take
> > > control of your XML. No limits. Just data. Click to get it now.
> > > http://sourceforge.net/powerbar/db2/
> > > --
> > > _______________________________________________
> > > Dri-devel mailing list
> > > Dri...@li...
> > > https://lists.sourceforge.net/lists/listinfo/dri-devel
> > >
> >
>