On poniedziałek, 16 lutego 2009 22:32:42 you wrote:
> Am Sonntag 15 Februar 2009 22:22:02 schrieb Maciej Cencora:
> > I've rewritten and split the patches.
>
> Thank you.
>
> Git complains about trailing whitespace when I apply this. It's a good idea
> to configure your editor to remove trailing whitespace automatically.
>
> > 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?
> > 0002-r300-silence-valgrind.patch
>
> Cool. Thanks for testing with Valgrind!
>
> > 0003-r300-add-few-macros-for-RS-setup.patch
>
> Cool.
>
> > 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.
> 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?
> 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?
> 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.
> > 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.
> 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.
Maciej
------------------------------------------------------------------------------
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
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
