Am Dienstag 17 Februar 2009 13:51:39 schrieb Maciej Cencora:
> On poniedziałek, 16 lutego 2009 22:32:42 you wrote:
> > Am Sonntag 15 Februar 2009 22:22:02 schrieb Maciej Cencora:
> > > 0001-r300-Print-reg-address-when-debugging-is-enabled.patch
> >
> > This patch is a nice idea, though it is somewhat incorrect because there
> > are state atoms that contain multiple cmdpacket()s (for example, look at
> > r300-
> >
> > >hw.fp). So I guess it would be nice if you could take that into account
> >
> > somehow.
>
> Maybe we should restrict it one cmdpacket per one state atom?
There is some state that we always change at the same time, it's just the
corresponding hardware registers happen to be not contiguous.
Both from a design and from an (admittedly micromanaging) efficiency point-of-
view, we should try to keep this kind of state in a single state atom.
> > > 0004-r300-rewrite-and-hopefully-simplify-RS-setup.patch
> >
> > Consider this chunk (and similar ones that come later):
> > >+ if (R300_OUTPUTS_WRITTEN_TEST(OutputsWritten,
> > > VERT_RESULT_COL0, _TNL_ATTRIB_COLOR0)) { + count
> > > = VB->AttribPtr[_TNL_ATTRIB_COLOR0]->size; + if
> > > (count == 4)
> > >+ col_fmt = R300_RS_COL_FMT_RGBA;
> > >+ else if (count == 3)
> > >+ col_fmt = R300_RS_COL_FMT_RGB1;
> > >+ else
> > >+ col_fmt = R300_RS_COL_FMT_0001;
> > >+
> > >+ r300->hw.ri.cmd[R300_RI_INTERP_0 + col_ip] =
> > > R300_RS_COL_PTR(col_ip) | R300_RS_COL_FMT(col_fmt); +
> > > r300->hw.rr.cmd[R300_RR_INST_0 + col_ip] = R300_RS_INST_COL_ID(col_ip)
> > > | R300_RS_INST_COL_CN_WRITE | R300_RS_INST_COL_ADDR(fp_reg); +
> > > InputsRead &= ~FRAG_BIT_COL0;
> > >+ ++col_ip;
> > >+ ++fp_reg;
> > >+ } else {
> >
> > The dependency on count seems wrong for HW TCL for me, but maybe I don't
> > understand the hardware correctly. Isn't the setup here for interpolation
> > of fragment values *after* the vertex program?
> >
> > So consider a case where a program sends only RGB color to the vertex
> > program, but the vertex program generates the full RGBA set of values, I
> > would expect this to produce incorrect results. I realize that this
> > particular code was there before, so either I'm wrong or this is actually
> > an older bug. But it seems completely plausible that all our test cases
> > so far haven't caught it.
>
> You're right, we should be checking what vertex program is writing out, and
> not what is put in vertex buffer. Current setup is correct only for
> software TCL path, but I think I'd better leave the fix for another round
> of patches.
I agree that this should be left to a different patch and shouldn't hold up
your changes. Also, before we change anything here I would prefer to have an
appropriate test case to make sure we really understood this correctly.
> > Note also the comment about always routing all 4 components of the
> > texcoords in hw_tcl mode.
>
> I don't understand why we have to route 4 compontents. Maybe you could
> explain it to me?
Well, if my theory is right, somebody figured out that in hwtcl we always need
to route all 4 components because we don't know into which components the
vertex program actually writes interesting values.
(Note that a perspective divide is done for textures as well if you use TXP,
which is the default for fixed function; so for a 2D texture, the s, t, and q
components are used.)
> > Also, it seems like our handling of mismatching vp output / fp input
> > might be incorrect. Currently, those registers are simply skipped, which
> > I guess is fine, but I believe the spec says that fragment attributes
> > that have not been written are undefined, while other fragment attributes
> > hold the correct values. Unfortunately, this won't be what happens with
> > the current code: If e.g. color wasn't written by the vertex program,
> > then all texcoords will get incorrect (shifted by one) values.
>
> Hmm, so we probably should just increase fp_reg in such a case. Am I right?
Yes and no. Experience and some comments in the code tell me that this part of
the hardware may be a little touchy to such things. For example, it may still
be necessary to do some RI_INTERP setup with dummy values.
I'd suggest leaving this to a separate patch (with appropriate test cases) as
well, to make sure we understood the problem correctly and to ensure better
bisectability.
> > Of course, all those issues seem to be old bugs. I think your patch is a
> > very good step in the right direction.
> >
> > Oh, one more thing I just noticed:
> > >+ if (InputsRead & FRAG_BIT_FOGC) {
> > >+ if (R300_OUTPUTS_WRITTEN_TEST(OutputsWritten,
> > > VERT_RESULT_FOGC, _TNL_ATTRIB_FOG)) { +
> > > r300->hw.ri.cmd[R300_RI_INTERP_0 + tex_ip] |= R300_RS_SEL_S(0) |
> > > R300_RS_SEL_T(1) | R300_RS_SEL_R(2) | R300_RS_SEL_Q( 3) |
> > > R300_RS_TEX_PTR(rs_tex_count);
> > >+ r300->hw.rr.cmd[R300_RR_INST_0 + tex_ip] |=
> > > R300_RS_INST_TEX_ID(tex_ip) | R300_RS_INST_TEX_CN_WRITE |
> > > R300_RS_INST_TEX_ADDR(fp_reg); + InputsRead &=
> > > ~FRAG_BIT_FOGC;
> > >+ rs_tex_count += 4;
> > >+ ++tex_ip;
> > >+ ++fp_reg;
> > > } else {
> > >- WARN_ONCE("fragprog wants col1, vp doesn't
> > > provide it\n"); + WARN_ONCE("fragprog wants fogc,
> > > vp doesn't provide it\n"); }
> > > }
> >
> > Shouldn't we check for overflow of tex_ip here?
>
> Seems like I forgot to check if we have free texcoord for hw tcl path. For
> swtcl it is currently checked during PSC setup - _mesa_exit if we run out
> of free texcoords so tex_ip will never overflow.
Yeah, something like this should definitely happen. Killing the program is
very ugly, but it's still better than uncontrolled overwriting of memory.
Ideally, we should recognise the condition early enough to fall back to
software rendering.
> > > 0005-r300-route-fog-coord-and-W-pos-correctly.patch
> >
> > There's a part about routing W-pos for swtcl, but it seems it is missing
> > from the hwtcl path. Is that correct?
>
> It is there: see r300SelectVertexShader in r300_vertprog.c, but I've just
> found problem with this (hw tcl path only). If fragment program requires
> both fragment depth and fogcoord, they are routed in reversed order -
> colors, texcoords, wpos, fogcoord and RS reads it colors, texcoords,
> fogcoord, wpos. I'll fix it.
Ah I see, thanks.
By the way, have you considered stuffing wpos and fogcoord in the same
register if both are needed? I'm just curious - if this is impossible for some
reason or requires significantly more changes to the code, just forget about
it. It's an optimization anyway, and it's more important to get things to work
correctly in the first place.
> > And what does the following have to do with fog:
> > > @@ -132,7 +132,6 @@ static void r300SetVertexFormat( GLcontext *ctx )
> > >
> > > if (RENDERINPUTS_TEST( index_bitset, _TNL_ATTRIB_POINTSIZE )) {
> > > EMIT_ATTR( _TNL_ATTRIB_POINTSIZE, EMIT_1F );
> > > - vap_fmt_0 |=
> > > R300_VAP_OUTPUT_VTX_FMT_0__PT_SIZE_PRESENT; offset += 1;
> > > }
>
> Commit message should: mention swtcl PSC setup cleanup.
>
> > I can't really comment on the swtcl changes.
> >
> > > 0006-r300-enable-EXT_fog_coord-extension.patch
> >
> > Seems pretty reasonable.
> >
> > However, it breaks Z write in fragment programs for me. I have to re-add
> > the write to fg_depth_src, see:
> > http://cgit.freedesktop.org/~nh/mesa/log/?h=from-osiris
>
> One more change was needed to fix the regression on my rs690 (setup
> fg_depth_src to FG_DEPTH_SRC_SCAN in r300ResetHwState).
>
> I'll update patches as soon as I get the answers for questions above.
Thanks again!
cu,
Nicolai
------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
Mesa3d-dev mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
