Re: [Dri-devel] mesa-3-5 DRI branch

[Keith W. <ke...@va...>]

Mark Crichton wrote:
> 
> * Keith Whitwell (ke...@va...) [010531 15:37]:
> > Mark Crichton wrote:
> > > I've found some issues in lib/GL/mesa/src/drv/mga/mgatris.c w.r.t. some
> > > of the inline assembly (newer gcc's will complain that esi has already
> > > been clobbered...fix is to not specifically list esi as clobbered, I
> > > think),
> >
> > Can you verify this?
> >
> 
> Yup.
> 
> gcc -v:
> Reading specs from /usr/lib/gcc-lib/i386-linux/2.95.4/specs
> gcc version 2.95.4 20010319 (Debian prerelease)
> 
> And you get a lot of:
> mgatris.c:99: Invalid `asm' statement:
> mgatris.c:99: fixed or forbidden register 4 (si) was spilled for class
> SIREG.
> 
> Which makes sense, since we've declared esi in the input ("=D" (vb)).
> Newer gcc's will complain about this.  It did bite the linux kernel
> people a while back.  Removing the ': "esi"' allows compiliation to
> complete.
> 
> However, I don't have a "visual standard" to test against.  Pulsar from
> xscreensaver appears to work, but my other 2 test apps (Parsec, GLTron,
> Chromium B.S.U.) show some problems.
> 
> > > and some dodgy code (lines 671-673 in same file if you don't
> > > have an X86).
> >
> > Why do you think this is bad?  It's just the same as the triangle code
> > elsewhere in that file.
> 
> Very dodgy, shouldn't-even-compile-for-!X86:
> 
> #define EMIT_VERT( j, vb, vertex_size, v )              \
> do {                                            \
>    for ( j = 0 ; j < vertex_size ; j++ )                \
>       vb[j] = v->ui[j];                         \
>    vb += vertex_size;                           \
> } while (0)
> 
> But, at lines 671-673:
>       EMIT_VERT( j, vb, vertex_size, start );
>       EMIT_VERT( j, vb, vertex_size, VERT(elts[i-1]) );
>       EMIT_VERT( j, vb, vertex_size, VERT(elts[i]) );
> 
> start is of type GLuint, and the macro (for !X86) will happily try to
> do a start->ui[j], which doesn't exist. (All the other instances seem
> to have v as type mgaVertPtr, not GLuint)
> 
> Which does raise another question, which macro is "correct", the raw
> copy (rep; movsl), or the C code?

The assembly is supposed to be a specialization of the 'C', with the same
semantics.  So, the type of start should be changed...

> >
> > I've been banging the driver back into shape on the 3.5 branch over the last
> > few days, but once it's working I don't have any real intention to do more
> > work on it, though I will be keeping it uptodate with infrastructure changes I
> > make elsewhere.
> >
> 
> So, the operative question is, what stuff does the MGA hardware do that
> isn't supported by the driver?

I haven't got the 'mga_render.c' code working with indexed vertices in the mga
yet (see the 3.4 code and perhaps radeon_render.c for examples).

That would be a good start.  In fact that whole file is disabled at the
moment.

Keith