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.
> 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.
Note also the comment about always routing all 4 components of the texcoords
in hw_tcl mode.
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.
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?
> 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?
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;
> }
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
The above branch is looking good in Piglit on my RV535 (hardware TCL). Note
that Piglit doesn't really stress the different fog paths, but some basic
tests are there. In fact, glean/fragProg1 now passes. Good work! :)
cu,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss nieals, wie sie sein sollte.
------------------------------------------------------------------------------
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
