Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
Hi all, On Wednesday, 11 April 2018 16:23:52 CEST Brian Paul wrote: > Hmm, in my experience, interleaved arrays are fairly common. > > I still haven't had much time to look at Mathias's latest patches. > > And I haven't looked this code in the state tracker recently, but I seem > to recall there was some difference between interleaved arrays (in one > VBO) vs. separate arrays in separate VBOs that needed special handling. > > As for determining whether arrays are interleaved, if that's something > we still need to do, I think it could be lifted into core Mesa. We > could add a new gl_vertex_array_object::_IsInterleaved field which is > only updated when the VAO state is modified. We don't need such a flag. We can easily integrate that into the current patch series. The bitmask loops help to loop in an outer loop by unique binding point - which would mean in the user space case by interleaved set or array attributes - and for each of that binding points (= one new buffer object) in an inner loop over the attributes belonging to this binding point. I think that I have a solution for both of your concerns by just using this nested bitmask loops. It is just that the interleaved scan for *user*space*arrays* (not the vbo interleaved case - that is already handled well or already taken from the already application provided data in the VAO) is currently not done in generic code, but can be moved there with a computational effort that is not more than carrying over the information from gl_vertex_buffer_binding to pipe_vertex_buffer and from gl_array_attributes to pipe_vertex_element. Look at the comment in patch #1. There is a pseudo code example how this is meant to work. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
Hi Marek, On Tuesday, 10 April 2018 20:09:06 CEST Marek Olšák wrote: > Generally, if you have to loop over all arrays to find common vertex > buffers, it's better not to do it. The default separate path is going to > perform best, because it's straightforward and interleaved arrays are super > rare. Yes, I know. All the cleanup I did recently is to get rid of all these kind of loops and if needed make them as cheap as possible. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
On Wed, Apr 11, 2018 at 10:23 AM, Brian Paul wrote: > Hmm, in my experience, interleaved arrays are fairly common. > In what kinds of apps are they common? Certainly not in Steam games. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
Hmm, in my experience, interleaved arrays are fairly common.
I still haven't had much time to look at Mathias's latest patches.
And I haven't looked this code in the state tracker recently, but I seem
to recall there was some difference between interleaved arrays (in one
VBO) vs. separate arrays in separate VBOs that needed special handling.
As for determining whether arrays are interleaved, if that's something
we still need to do, I think it could be lifted into core Mesa. We
could add a new gl_vertex_array_object::_IsInterleaved field which is
only updated when the VAO state is modified.
-Brian
On 04/10/2018 12:09 PM, Marek Olšák wrote:
Generally, if you have to loop over all arrays to find common vertex
buffers, it's better not to do it. The default separate path is going to
perform best, because it's straightforward and interleaved arrays are
super rare.
Marek
On Mon, Apr 9, 2018 at 7:15 PM, Mathias Fröhlich
mailto:mathias.froehl...@gmx.net>> wrote:
Hi Marek,
On Saturday, 7 April 2018 01:53:58 CEST Marek Olšák wrote:
> So interleaved attribs are unsupported, right?
>
> is_interleaved_arrays was probably slowing things down, so I'm OK with
that.
I am currently away from all the source code and be back at about
the 22.4.
But out of my head: The main purpose of the is_interleaved_arrays
that I could
spot is to minimize the vbo's that are send down the pipeline. In
the non vbo
case the is_interleaved_arrays check did nothing I could finally spot?
The buffer itself is marked as user buffer and we need a new vbuffer
because
of the pointer value anyway? Correct?
So, the VAO now contains all the redundancy information. And thanks
to this
bitmask sieves we can easily collect the arrays belonging to a specific
precollapsed binding point.
So, the is_interleaved is fully there in the vbo case. Even better
as before.
It sees even 4 attributes distributed across two pairwise
interleaved vbo
arrays.
So even if you are fine, if you tell me that the user buffer code
can make use
of the same sharing finally, I can take a look at that and establish
the same
sort of sharing here.
best
Mathias
>
> Marek
>
> On Sun, Apr 1, 2018 at 2:13 PM, mailto:mathias.froehl...@gmx.net>> wrote:
> > From: Mathias Fröhlich mailto:mathias.froehl...@web.de>>
> >
> > Finally make use of the binding information in the VAO when
> > setting up arrays for draw.
> >
> > Signed-off-by: Mathias Fröhlich mailto:mathias.froehl...@web.de>>
> > ---
> >
> > src/mesa/state_tracker/st_atom_array.c | 448
> >
> > +
> >
> > 1 file changed, 124 insertions(+), 324 deletions(-)
> >
> > diff --git a/src/mesa/state_tracker/st_atom_array.c
> > b/src/mesa/state_tracker/st_atom_array.c
> > index 2fd67e8d84..46934a718a 100644
> > --- a/src/mesa/state_tracker/st_atom_array.c
> > +++ b/src/mesa/state_tracker/st_atom_array.c
> > @@ -48,6 +48,7 @@
> >
> > #include "main/bufferobj.h"
> > #include "main/glformats.h"
> > #include "main/varray.h"
> >
> > +#include "main/arrayobj.h"
> >
> > /* vertex_formats[gltype - GL_BYTE][integer*2 +
normalized][size - 1] */
> > static const uint16_t vertex_formats[][4][4] = {
> >
> > @@ -306,79 +307,6 @@ st_pipe_vertex_format(const struct
> > gl_array_attributes *attrib)
> >
> > return vertex_formats[type - GL_BYTE][index][size-1];
> >
> > }
> >
> > -static const struct gl_vertex_array *
> > -get_client_array(const struct gl_vertex_array *arrays,
> > - unsigned mesaAttr)
> > -{
> > - /* st_program uses 0x to denote a double
placeholder attribute
> > */
> > - if (mesaAttr == ST_DOUBLE_ATTRIB_PLACEHOLDER)
> > - return NULL;
> > - return &arrays[mesaAttr];
> > -}
> > -
> > -/**
> > - * Examine the active arrays to determine if we have interleaved
> > - * vertex arrays all living in one VBO, or all living in user
space.
> > - */
> > -static GLboolean
> > -is_interleaved_arrays(const struct st_vertex_program *vp,
> > - const struct gl_vertex_array *arrays,
> > - unsigned num_inputs)
> > -{
> > - GLuint attr;
> > - const struct gl_buffer_object *firstBufObj = NULL;
> > - GLint firstStride = -1;
> > - const GLubyte *firstPtr = NULL;
> > - GLboolean userSpaceBuffer = GL_FALSE;
> > -
> > - for (attr = 0; attr < num_inputs; attr++) {
> > - const struct gl_vertex_array *array;
> > - const struct gl_vertex_buffer_binding *binding;
> > - const struct gl_array_attributes *attrib;
> > -
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
Generally, if you have to loop over all arrays to find common vertex
buffers, it's better not to do it. The default separate path is going to
perform best, because it's straightforward and interleaved arrays are super
rare.
Marek
On Mon, Apr 9, 2018 at 7:15 PM, Mathias Fröhlich
wrote:
> Hi Marek,
>
> On Saturday, 7 April 2018 01:53:58 CEST Marek Olšák wrote:
> > So interleaved attribs are unsupported, right?
> >
> > is_interleaved_arrays was probably slowing things down, so I'm OK with
> that.
>
> I am currently away from all the source code and be back at about the 22.4.
>
> But out of my head: The main purpose of the is_interleaved_arrays that I
> could
> spot is to minimize the vbo's that are send down the pipeline. In the non
> vbo
> case the is_interleaved_arrays check did nothing I could finally spot?
> The buffer itself is marked as user buffer and we need a new vbuffer
> because
> of the pointer value anyway? Correct?
>
> So, the VAO now contains all the redundancy information. And thanks to this
> bitmask sieves we can easily collect the arrays belonging to a specific
> precollapsed binding point.
> So, the is_interleaved is fully there in the vbo case. Even better as
> before.
> It sees even 4 attributes distributed across two pairwise interleaved vbo
> arrays.
>
> So even if you are fine, if you tell me that the user buffer code can make
> use
> of the same sharing finally, I can take a look at that and establish the
> same
> sort of sharing here.
>
> best
>
> Mathias
>
>
> >
> > Marek
> >
> > On Sun, Apr 1, 2018 at 2:13 PM, wrote:
> > > From: Mathias Fröhlich
> > >
> > > Finally make use of the binding information in the VAO when
> > > setting up arrays for draw.
> > >
> > > Signed-off-by: Mathias Fröhlich
> > > ---
> > >
> > > src/mesa/state_tracker/st_atom_array.c | 448
> > >
> > > +
> > >
> > > 1 file changed, 124 insertions(+), 324 deletions(-)
> > >
> > > diff --git a/src/mesa/state_tracker/st_atom_array.c
> > > b/src/mesa/state_tracker/st_atom_array.c
> > > index 2fd67e8d84..46934a718a 100644
> > > --- a/src/mesa/state_tracker/st_atom_array.c
> > > +++ b/src/mesa/state_tracker/st_atom_array.c
> > > @@ -48,6 +48,7 @@
> > >
> > > #include "main/bufferobj.h"
> > > #include "main/glformats.h"
> > > #include "main/varray.h"
> > >
> > > +#include "main/arrayobj.h"
> > >
> > > /* vertex_formats[gltype - GL_BYTE][integer*2 + normalized][size - 1]
> */
> > > static const uint16_t vertex_formats[][4][4] = {
> > >
> > > @@ -306,79 +307,6 @@ st_pipe_vertex_format(const struct
> > > gl_array_attributes *attrib)
> > >
> > > return vertex_formats[type - GL_BYTE][index][size-1];
> > >
> > > }
> > >
> > > -static const struct gl_vertex_array *
> > > -get_client_array(const struct gl_vertex_array *arrays,
> > > - unsigned mesaAttr)
> > > -{
> > > - /* st_program uses 0x to denote a double placeholder
> attribute
> > > */
> > > - if (mesaAttr == ST_DOUBLE_ATTRIB_PLACEHOLDER)
> > > - return NULL;
> > > - return &arrays[mesaAttr];
> > > -}
> > > -
> > > -/**
> > > - * Examine the active arrays to determine if we have interleaved
> > > - * vertex arrays all living in one VBO, or all living in user space.
> > > - */
> > > -static GLboolean
> > > -is_interleaved_arrays(const struct st_vertex_program *vp,
> > > - const struct gl_vertex_array *arrays,
> > > - unsigned num_inputs)
> > > -{
> > > - GLuint attr;
> > > - const struct gl_buffer_object *firstBufObj = NULL;
> > > - GLint firstStride = -1;
> > > - const GLubyte *firstPtr = NULL;
> > > - GLboolean userSpaceBuffer = GL_FALSE;
> > > -
> > > - for (attr = 0; attr < num_inputs; attr++) {
> > > - const struct gl_vertex_array *array;
> > > - const struct gl_vertex_buffer_binding *binding;
> > > - const struct gl_array_attributes *attrib;
> > > - const GLubyte *ptr;
> > > - const struct gl_buffer_object *bufObj;
> > > - GLsizei stride;
> > > -
> > > - array = get_client_array(arrays, vp->index_to_input[attr]);
> > > - if (!array)
> > > -continue;
> > > -
> > > - binding = array->BufferBinding;
> > > - attrib = array->VertexAttrib;
> > > - stride = binding->Stride; /* in bytes */
> > > - ptr = _mesa_vertex_attrib_address(attrib, binding);
> > > -
> > > - /* To keep things simple, don't allow interleaved zero-stride
> > > attribs. */
> > > - if (stride == 0)
> > > - return false;
> > > -
> > > - bufObj = binding->BufferObj;
> > > - if (attr == 0) {
> > > - /* save info about the first array */
> > > - firstStride = stride;
> > > - firstPtr = ptr;
> > > - firstBufObj = bufObj;
> > > - userSpaceBuffer = !_mesa_is_bufferobj(bufObj);
> > > - }
> > > - else {
> > > - /* check if other arrays interleave with the first, in same
> > > buffer */
> > > - if (stride != firstStrid
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
Hi Marek,
On Saturday, 7 April 2018 01:53:58 CEST Marek Olšák wrote:
> So interleaved attribs are unsupported, right?
>
> is_interleaved_arrays was probably slowing things down, so I'm OK with that.
I am currently away from all the source code and be back at about the 22.4.
But out of my head: The main purpose of the is_interleaved_arrays that I could
spot is to minimize the vbo's that are send down the pipeline. In the non vbo
case the is_interleaved_arrays check did nothing I could finally spot?
The buffer itself is marked as user buffer and we need a new vbuffer because
of the pointer value anyway? Correct?
So, the VAO now contains all the redundancy information. And thanks to this
bitmask sieves we can easily collect the arrays belonging to a specific
precollapsed binding point.
So, the is_interleaved is fully there in the vbo case. Even better as before.
It sees even 4 attributes distributed across two pairwise interleaved vbo
arrays.
So even if you are fine, if you tell me that the user buffer code can make use
of the same sharing finally, I can take a look at that and establish the same
sort of sharing here.
best
Mathias
>
> Marek
>
> On Sun, Apr 1, 2018 at 2:13 PM, wrote:
> > From: Mathias Fröhlich
> >
> > Finally make use of the binding information in the VAO when
> > setting up arrays for draw.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> >
> > src/mesa/state_tracker/st_atom_array.c | 448
> >
> > +
> >
> > 1 file changed, 124 insertions(+), 324 deletions(-)
> >
> > diff --git a/src/mesa/state_tracker/st_atom_array.c
> > b/src/mesa/state_tracker/st_atom_array.c
> > index 2fd67e8d84..46934a718a 100644
> > --- a/src/mesa/state_tracker/st_atom_array.c
> > +++ b/src/mesa/state_tracker/st_atom_array.c
> > @@ -48,6 +48,7 @@
> >
> > #include "main/bufferobj.h"
> > #include "main/glformats.h"
> > #include "main/varray.h"
> >
> > +#include "main/arrayobj.h"
> >
> > /* vertex_formats[gltype - GL_BYTE][integer*2 + normalized][size - 1] */
> > static const uint16_t vertex_formats[][4][4] = {
> >
> > @@ -306,79 +307,6 @@ st_pipe_vertex_format(const struct
> > gl_array_attributes *attrib)
> >
> > return vertex_formats[type - GL_BYTE][index][size-1];
> >
> > }
> >
> > -static const struct gl_vertex_array *
> > -get_client_array(const struct gl_vertex_array *arrays,
> > - unsigned mesaAttr)
> > -{
> > - /* st_program uses 0x to denote a double placeholder attribute
> > */
> > - if (mesaAttr == ST_DOUBLE_ATTRIB_PLACEHOLDER)
> > - return NULL;
> > - return &arrays[mesaAttr];
> > -}
> > -
> > -/**
> > - * Examine the active arrays to determine if we have interleaved
> > - * vertex arrays all living in one VBO, or all living in user space.
> > - */
> > -static GLboolean
> > -is_interleaved_arrays(const struct st_vertex_program *vp,
> > - const struct gl_vertex_array *arrays,
> > - unsigned num_inputs)
> > -{
> > - GLuint attr;
> > - const struct gl_buffer_object *firstBufObj = NULL;
> > - GLint firstStride = -1;
> > - const GLubyte *firstPtr = NULL;
> > - GLboolean userSpaceBuffer = GL_FALSE;
> > -
> > - for (attr = 0; attr < num_inputs; attr++) {
> > - const struct gl_vertex_array *array;
> > - const struct gl_vertex_buffer_binding *binding;
> > - const struct gl_array_attributes *attrib;
> > - const GLubyte *ptr;
> > - const struct gl_buffer_object *bufObj;
> > - GLsizei stride;
> > -
> > - array = get_client_array(arrays, vp->index_to_input[attr]);
> > - if (!array)
> > -continue;
> > -
> > - binding = array->BufferBinding;
> > - attrib = array->VertexAttrib;
> > - stride = binding->Stride; /* in bytes */
> > - ptr = _mesa_vertex_attrib_address(attrib, binding);
> > -
> > - /* To keep things simple, don't allow interleaved zero-stride
> > attribs. */
> > - if (stride == 0)
> > - return false;
> > -
> > - bufObj = binding->BufferObj;
> > - if (attr == 0) {
> > - /* save info about the first array */
> > - firstStride = stride;
> > - firstPtr = ptr;
> > - firstBufObj = bufObj;
> > - userSpaceBuffer = !_mesa_is_bufferobj(bufObj);
> > - }
> > - else {
> > - /* check if other arrays interleave with the first, in same
> > buffer */
> > - if (stride != firstStride)
> > -return GL_FALSE; /* strides don't match */
> > -
> > - if (bufObj != firstBufObj)
> > -return GL_FALSE; /* arrays in different VBOs */
> > -
> > - if (llabs(ptr - firstPtr) > firstStride)
> > -return GL_FALSE; /* arrays start too far apart */
> > -
> > - if ((!_mesa_is_bufferobj(bufObj)) != userSpaceBuffer)
> > -return GL_FALSE; /* mix of VBO and user-space arrays */
> > - }
> > - }
> > -
> > - return GL_TRUE;
> > -}
> > -
> >
> > static void
Re: [Mesa-dev] [PATCH 04/11] gallium: Use Array._DrawVAO in st_atom_array.c.
So interleaved attribs are unsupported, right?
is_interleaved_arrays was probably slowing things down, so I'm OK with that.
Marek
On Sun, Apr 1, 2018 at 2:13 PM, wrote:
> From: Mathias Fröhlich
>
> Finally make use of the binding information in the VAO when
> setting up arrays for draw.
>
> Signed-off-by: Mathias Fröhlich
> ---
> src/mesa/state_tracker/st_atom_array.c | 448
> +
> 1 file changed, 124 insertions(+), 324 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_atom_array.c
> b/src/mesa/state_tracker/st_atom_array.c
> index 2fd67e8d84..46934a718a 100644
> --- a/src/mesa/state_tracker/st_atom_array.c
> +++ b/src/mesa/state_tracker/st_atom_array.c
> @@ -48,6 +48,7 @@
> #include "main/bufferobj.h"
> #include "main/glformats.h"
> #include "main/varray.h"
> +#include "main/arrayobj.h"
>
> /* vertex_formats[gltype - GL_BYTE][integer*2 + normalized][size - 1] */
> static const uint16_t vertex_formats[][4][4] = {
> @@ -306,79 +307,6 @@ st_pipe_vertex_format(const struct
> gl_array_attributes *attrib)
> return vertex_formats[type - GL_BYTE][index][size-1];
> }
>
> -static const struct gl_vertex_array *
> -get_client_array(const struct gl_vertex_array *arrays,
> - unsigned mesaAttr)
> -{
> - /* st_program uses 0x to denote a double placeholder attribute
> */
> - if (mesaAttr == ST_DOUBLE_ATTRIB_PLACEHOLDER)
> - return NULL;
> - return &arrays[mesaAttr];
> -}
> -
> -/**
> - * Examine the active arrays to determine if we have interleaved
> - * vertex arrays all living in one VBO, or all living in user space.
> - */
> -static GLboolean
> -is_interleaved_arrays(const struct st_vertex_program *vp,
> - const struct gl_vertex_array *arrays,
> - unsigned num_inputs)
> -{
> - GLuint attr;
> - const struct gl_buffer_object *firstBufObj = NULL;
> - GLint firstStride = -1;
> - const GLubyte *firstPtr = NULL;
> - GLboolean userSpaceBuffer = GL_FALSE;
> -
> - for (attr = 0; attr < num_inputs; attr++) {
> - const struct gl_vertex_array *array;
> - const struct gl_vertex_buffer_binding *binding;
> - const struct gl_array_attributes *attrib;
> - const GLubyte *ptr;
> - const struct gl_buffer_object *bufObj;
> - GLsizei stride;
> -
> - array = get_client_array(arrays, vp->index_to_input[attr]);
> - if (!array)
> -continue;
> -
> - binding = array->BufferBinding;
> - attrib = array->VertexAttrib;
> - stride = binding->Stride; /* in bytes */
> - ptr = _mesa_vertex_attrib_address(attrib, binding);
> -
> - /* To keep things simple, don't allow interleaved zero-stride
> attribs. */
> - if (stride == 0)
> - return false;
> -
> - bufObj = binding->BufferObj;
> - if (attr == 0) {
> - /* save info about the first array */
> - firstStride = stride;
> - firstPtr = ptr;
> - firstBufObj = bufObj;
> - userSpaceBuffer = !_mesa_is_bufferobj(bufObj);
> - }
> - else {
> - /* check if other arrays interleave with the first, in same
> buffer */
> - if (stride != firstStride)
> -return GL_FALSE; /* strides don't match */
> -
> - if (bufObj != firstBufObj)
> -return GL_FALSE; /* arrays in different VBOs */
> -
> - if (llabs(ptr - firstPtr) > firstStride)
> -return GL_FALSE; /* arrays start too far apart */
> -
> - if ((!_mesa_is_bufferobj(bufObj)) != userSpaceBuffer)
> -return GL_FALSE; /* mix of VBO and user-space arrays */
> - }
> - }
> -
> - return GL_TRUE;
> -}
> -
> static void init_velement(struct pipe_vertex_element *velement,
>int src_offset, int format,
>int instance_divisor, int vbo_index)
> @@ -392,13 +320,14 @@ static void init_velement(struct pipe_vertex_element
> *velement,
>
> static void init_velement_lowered(const struct st_vertex_program *vp,
>struct pipe_vertex_element *velements,
> - int src_offset, int format,
> - int instance_divisor, int vbo_index,
> - int nr_components, GLboolean doubles,
> - GLuint *attr_idx)
> + const struct gl_array_attributes
> *attrib,
> + int src_offset, int instance_divisor,
> + int vbo_index, int idx)
> {
> - int idx = *attr_idx;
> - if (doubles) {
> + const unsigned format = st_pipe_vertex_format(attrib);
> + const GLubyte nr_components = attrib->Size;
> +
> + if (attrib->Doubles) {
>int lower_format;
>
>if (nr_components < 2)
> @@ -427,15 +356,11 @@ static void init_velement_lowered(const struct
> st_vertex_program *vp,
> init_velement(&velements[idx], src_off
