[Mesa-dev] [PATCH] vbo: Set FLUSH_UPDATE_CURRENT when setting vertex attibutes

Kristian Høgsberg krh at bitplanet.net
Tue Sep 14 09:21:41 PDT 2010

2010/9/14 Chia-I Wu <olvaffe at gmail.com>:
> 2010/9/14 Kristian Høgsberg <krh at bitplanet.net>:
>> 2010/9/14 Chia-I Wu <olvaffe at gmail.com>:
>>> 2010/9/14 Kristian Høgsberg <krh at bitplanet.net>:
>>>> 2010/9/13 keith whitwell <keith.whitwell at 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.


More information about the mesa-dev mailing list