[Mesa-dev] [PATCH] mesa: Make gl_vertex_array contain pointers to first order VAO members.

[Mathias Fröhlich]

Hi Emil,

On Tuesday, 6 March 2018 12:13:07 CET Emil Velikov wrote:
> Just putting forward some suggestions. Please take them with a pinch of 
salt.

You are welcome!

> Wondering if one cannot split this up somehow. Perhaps introduce
> gl_vertex_array::VertexAttrib first and the gl_vertex_buffer_binding
> struct as 2/2?
> It's a big thing to suggest, yet it would make it easier to catch all
> the subtleties.

Hmm, yes I was playing with several steps and variants as I also dont like 
this huge changes. Initially I had just mechanic changes, like done with sed. 
Easier to review, but made about any 80 column wrapping void. And that series 
just touched all the files multiple times. Which did not look better to me. 
May be this was only my taste for the series, but finally I ended with this 
blob as the first public attempt. And luckily I already got some review!
The most brittle part for me is the classic nvidia backend as I have nothing 
to thest that. All tnl based drivers should be well coverd with testing on 
classic swrast, so I don't expect surprises with these changes there. Gallium 
type drivers I can test with radeonsi. And the intel drivers get testing with 
my notebook. May be I am also pushing without special treatment of classic 
nvidia but any help there not to recieve bugzilla entries at my side is 
apprecheated.

> > @@ -462,21 +459,8 @@ void
> >  _mesa_update_vao_derived_arrays(struct gl_context *ctx,
> >                                  struct gl_vertex_array_object *vao)
> >  {
> > -   GLbitfield arrays = vao->NewArrays;
> > -
> >     /* Make sure we do not run into problems with shared objects */
> >     assert(!vao->SharedAndImmutable || vao->NewArrays == 0);
> > -
> > -   while (arrays) {
> > -      const int attrib = u_bit_scan(&arrays);
> > -      struct gl_vertex_array *array = &vao->_VertexArray[attrib];
> > -      const struct gl_array_attributes *attribs =
> > -         &vao->VertexAttrib[attrib];
> > -      const struct gl_vertex_buffer_binding *buffer_binding =
> > -         &vao->BufferBinding[attribs->BufferBindingIndex];
> > -
> > -      _mesa_update_vertex_array(ctx, array, attribs, buffer_binding);
> > -   }
> ... and we can drop _mesa_update_vao_derived_arrays() with a follow-up 
patch?

Not that, but to repopulate that function with a not so distant change :-)

I will establish some unique binding information in the VAO and that must be 
done at exactly that entry points. I can remove that but it will reappear 
again in a hand full of weeks at most ...

> > +/**
> > + * Vertex array information which is derived from gl_array_attributes
> > + * and gl_vertex_buffer_binding information.  Used by the VBO module and
> > + * device drivers.
> > + */
> > +struct gl_vertex_array
> > +{
> > +   /** Vertex attribute array */
> > +   const struct gl_array_attributes *VertexAttrib;
> > +   /** Vertex buffer binding */
> > +   const struct gl_vertex_buffer_binding *BufferBinding;
> 
> Worth expanding the documentation to cover the const-ness or lifetime?
> AKA why were we coping all of the data before, and we don't need to now.
> 
> Mildly related:
> AEarray and AEattrib look surprisingly similar to gl_vertex_array now.
> I'm guessing that you're heading there next?

Probably not. The ae things are much closer to vbo_loopback*.
The anticipated future for struct gl_vertex_array is to basically get rid of 
it entirely in modern drivers and the uppermost gl layers. Probably it will 
still survive in the classic tnl driver path, so it probably gets pushed there 
and renamed to tnl_vertex_array...

> > +/**
> > + * Copy one vertex array to another.
> > + */
> > +static inline void
> > +_mesa_copy_vertex_array(struct gl_vertex_array *dst,
> > +                        const struct gl_vertex_array *src)
> > +{
> > +   dst->VertexAttrib = src->VertexAttrib;
> > +   dst->BufferBinding = src->BufferBinding;
> > +}
> > +
> Say "Shallow copy" in the inline comment?

Will to ...

best

Mathias