[Mesa-dev] [PATCH 03/11] mesa: Implement _mesa_all_varyings_in_vbos.

[Mathias Fröhlich]

On Thursday, June 23, 2016 16:53:59 you wrote:
> On Friday 17 June 2016, Mathias.Froehlich at gmx.net wrote:
> > From: Mathias Fröhlich <mathias.froehlich at web.de>
> > 
> > Implement the equivalent of vbo_all_varyings_in_vbos for
> > vertex array objects.
> > 
> > Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> > ---
> >  src/mesa/main/arrayobj.c | 35 +++++++++++++++++++++++++++++++++++
> >  src/mesa/main/arrayobj.h |  4 ++++
> >  2 files changed, 39 insertions(+)
> > 
> > diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
> > index 9c3451e..041ee63 100644
> > --- a/src/mesa/main/arrayobj.c
> > +++ b/src/mesa/main/arrayobj.c
> > @@ -359,6 +359,41 @@ _mesa_update_vao_client_arrays(struct gl_context *ctx,
> >  }
> >  
> >  
> > +bool
> > +_mesa_all_varyings_in_vbos(const struct gl_vertex_array_object *vao)
> > +{
> > +   /* Walk those enabled arrays that have the default vbo attached */
> > +   GLbitfield64 mask = vao->_Enabled & ~vao->VertexAttribBufferMask;
> > +
> > +   while (mask) {
> > +      /** We do not use u_bit_scan64 as we can here walk
> > +       *  multiple attrib arrays at once
> > +       */
> > +      const int i = ffsll(mask) - 1;
> > +      const struct gl_vertex_attrib_array *attrib_array =
> > +         &vao->VertexAttrib[i];
> > +      const struct gl_vertex_buffer_binding *buffer_binding =
> > +         &vao->VertexBinding[attrib_array->VertexBinding];
> > +
> > +      /* Only enabled arrays shall appear in the _Enabled bitmask */
> > +      assert(attrib_array->Enabled);
> > +      /* We have already masked out vao->VertexAttribBufferMask  */
> > +      assert(!_mesa_is_bufferobj(buffer_binding->BufferObj));
> > +
> > +      /* Bail out once we find the first non vbo with a non zero stride */
> > +      if (buffer_binding->Stride != 0)
> > +         return false;
> 
> I'm not sure if this is correct.  The default value for Stride is 16,
> not 0.  The only way Stride can be zero in a binding point that doesn't
> have a buffer object bound is if the user has explicitly called
> glBindVertexBuffer() with both the buffer and stride parameters set
> to zero.
> 
> StrideB in gl_client_array on the other hand is always zero when the array
> is one of the currval arrays managed by the VBO context.  It is never zero
> when the array is a user array that has been specified with gl*Pointer().
> 
> I think the point of vbo_all_varyings_in_vbos() is to return false if any
> enabled array doesn't have a VBO bound, and is not one of the currval
> arrays.

Hmm, at least the current implementation of vbo_all_varyings_in_vbos() does not
make a difference between enabled arrays with a binding stride == 0 and
a faked array also with stride == 0 taking the userspace pointer from the
current values.

So what it does is: Return false iff there is any at least one array that
is not in a vbo and that has gl_client_array::StrideB == 0.
AFAICT this is used to determine if there needs to be some upload to
gpu accessible memory for anything that is not constant over the draw.

Indeed, IF the point is - like you meant above - to return false if any
enabled array doesn't have a VBO bound, and is not one of the currval
arrays. Then the implementation could be easier. Additionally I would
suggest an other name for this:

/* Returns true if all enabled arrays reside in vbos */
static inline bool
_mesa_all_enabled_in_vbos(const struct gl_vertex_array_object *vao)
{
   return 0 == (vao->_Enabled & ~vao->VertexAttribBufferMask);
}


But back to vbo_all_varyings_in_vbos() and _mesa_all_varyings_in_vbos().
The first observation: In the case of VAO driven client arrays,
gl_client_array::StrideB is fed from gl_vertex_buffer_binding::Stride.
That is it should be equivalent to look at the later instead of the former.

Then you can try to transform 

GLboolean vbo_all_varyings_in_vbos( const struct gl_client_array *arrays[] )
{
   GLuint i;
   
   for (i = 0; i < VERT_ATTRIB_MAX; i++)
      if (arrays[i]->StrideB &&
          arrays[i]->BufferObj->Name == 0)
         return GL_FALSE;

   return GL_TRUE;
}

into the _mesa_all_varyings_in_vbos() by splitting the for loop from
vbo_all_varyings_in_vbos() into 3 loops:

GLboolean vbo_all_varyings_in_vbos( const struct gl_client_array *arrays[] )
{
  for (current value arrays)
    ...

  for (enabled arrays with real vbo)
    ...

  for (enabled arrays without real vbo)
    ...

  return true;
}

The first loop of the above has always StrideB == 0. So the original
loop bodys if statement is always false and continues the loop.
The second loop of the above has always ...->Name != 0, So again the
original loop bodys if statement is always false and continues the loop.
The third loop of the above is the remaining question and is what the
patch implements. The check for ...->Name != 0 can be omitted in the
third loop as we already know to work with non vbo arrays.

Note that this thought does not need to care about any default values
being set or which OpenGL api function sets the stride value.

So far to what I thought, but what else did I miss?

Thanks for looking at this BTW

Mathias

> 
> > +
> > +      /* Note that we cannot use the xor variant since the _BoundArray mask
> > +       * may contain array attributes that are bound but not enabled.
> > +       */
> > +      mask &= ~buffer_binding->_BoundArrays;
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +
> >  /**********************************************************************/
> >  /* API Functions                                                      */
> >  /**********************************************************************/
> > diff --git a/src/mesa/main/arrayobj.h b/src/mesa/main/arrayobj.h
> > index 6a4247f..d30c85c 100644
> > --- a/src/mesa/main/arrayobj.h
> > +++ b/src/mesa/main/arrayobj.h
> > @@ -81,6 +81,10 @@ extern void
> >  _mesa_update_vao_client_arrays(struct gl_context *ctx,
> >                                 struct gl_vertex_array_object *vao);
> >  
> > +/* Returns true if all varying arrays reside in vbos */
> > +extern bool
> > +_mesa_all_varyings_in_vbos(const struct gl_vertex_array_object *vao);
> > +
> >  /*
> >   * API functions
> >   */
> > 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160624/876ffc2e/attachment-0001.html>