2010/9/14 Chia-I Wu <olva...@gmail.com>: > 2010/9/14 Kristian Høgsberg <k...@bitplanet.net>: >> 2010/9/14 Chia-I Wu <olva...@gmail.com>: >>> 2010/9/14 Kristian Høgsberg <k...@bitplanet.net>: >>>> 2010/9/13 keith whitwell <keith.whitw...@gmail.com>: >>>>> Hey Kristian, >>>>> >>>>> The first question is whether this is necessary - from vague memory I >>>>> have an idea that current attributes need not be updated by vertex >>>>> buffer rendering - ie. it's optional/implementation-dependent. >>>>> >>>>> I assume you're concerned with the case where you have something like >>>>> >>>>> // ctx->Current.Color is xyz >>>>> >>>>> glDrawArrays(); >>>>> >>>>> // has ctx->Current.Color been updated?? >>>>> >>>>> But assuming I'm wrong about that & we really do want to make >>>>> DrawArrays set the current values, the patch looks good... >>>> >>>> No, what I'm seeing is that the code in question sets three generic >>>> vertex attributes and then calls glDrawArrays(). The value of the >>>> last attribute is not propagates into the shader. >>>> >>>> The problem is that the vertex array code keeps the values in >>>> exec->vtx.vertex, but the implementation of glDrawArrays looks in >>>> ctx->Current (that's what I assume, I didn't track that down). When >>>> the code hits a case where the size of an attribute is smaller that >>>> what we're trying to set, it recomputes the layout of the >>>> exec->vtx.vertex values and as a side effect copies the >>>> exec->vtx.vertex values to ctx->Current. Since we start out with >>>> attrsz == 0 for all attributes, each new attribute will trigger this >>>> recomputation and thus effectively flushes all previous values to >>>> ctx->Current. Which is why all but the last attribute make it to the >>>> shader. >>>> >>>> Note that the ATTR macro is defined differently, depending on >>>> FEATURE_beginend - the !FEATURE_beginend case sets the >>>> FLUSH_UPDATE_CURRENT flag too. I don't know why we wouldn't also set >>>> it in the FEATURE_beginend case, not using begin/end in that case is >>>> still an option. >>> The way glColor4f is dispatched depends on whether it is GL or ES: >>> >>> GL (with FEATURE_beginend): glColor4f -> neutral_Color4f -> vbo_Color4f >>> ES (w/o FEATURE_beginend): glColor4f -> _es_Color4f -> vbo_Color4f >>> >>> In the former case, FLUSH_UPDATE_CURRENT should have been set by >>> vbo_exec_BeginVertices which is called by neutral_Color4f. In the latter >>> case, >>> the flag must be set in vbo_Color4f. Could it be a bug some where in >>> vtxfmt? >>> >>> One issue I noticed just last week is that the current scheme does not take >>> into account "ES with FEATURE_beginend". This happens with --enable-gles2 >>> build. Since FEATURE_beginend is enabled in such build, vbo_Color4f does >>> not >>> set FLUSH_UPDATE_CURRENT. Yet, an ES context does not use neutral_Color4f. >> >> And that's exactly the problem I have. Setting FLUSH_UPDATE_CURRENT >> in the ATTR macro makes sure that the FLUSH_CURRENT in >> vbo_exec_DrawArrays (and other array draw funcs) ends up calling >> vbo_exec_copy_to_current(), which then pushes the values into >> ctx->Current before the draw call. I don't see a problem with this >> approach; in a begin/end, this flag is already set, so there's no >> overhead, outside begin/end (ES2) it's required for correct behaviour. > If this patch is to be applied, it makes sense to also stop setting > FLUSH_UPDATE_CURRENT in vbo_exec_BeginVertices, and remove the > !FEATURE_beginend case. > > But then, does it make sense to remove exec vtxfmt in main/vtxfmt.c and the > related code (a big chunk!), and define ATTR to > > #define ATTR(...) do { > struct vbo_exec_context *exec = &vbo_context(ctx)->exec; > > if (!exec->ctx->Driver.NeedFlush) > vbo_exec_BeginVertices(ctx); > > /* ... */ > } > > From what I can tell, and I am be wrong, exec vtxfmt and PRE_LOOPBACK are to > avoid any unnecessary operation in vtxfmt functions, even as cheap as setting > FLUSH_UPDATE_CURRENT repeatedly. If that cannot be avoided, there seems to be > no need to have exec vtxfmt.
Yeah, it seems like we can just do that. We can use the unlikely() macro to reduce the overhead of the test. Kristian _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev