[Mesa-dev] [PATCH 1/3] vbo: Reestablish VAO limit checking on imm VAOs.

[Mathias Fröhlich]

Hi Christian,

On Sunday, 3 June 2018 20:08:14 CEST Christian Gmeiner wrote:
> Am Sa., 2. Juni 2018 um 08:53 Uhr schrieb <Mathias.Froehlich at gmx.net>:
> >
> > From: Mathias Fröhlich <mathias.froehlich at web.de>
> >
> > Bail out with out of memory, when the required stride exceeds the
> > maximum possible stride as stored in Const.MaxVertexAttribStride.
> > Etnaviv can only handle less than the OpenGL standard mandated
> > minimum of 2048. For this one, at least give some 'resource
> > exhaustion error' to the application.
> > The virgl driver provides 0 for Const.MaxVertexAttribStride
> > as it may not be able to query the relevant informatition from the
> > underlying OpenGL stack. In this case we just make the assumption
> > that the underlying OpenGL stack is able to cope with then commonly
> > seen small strides. Note that this is the same than what applications
> > had to do before the queries to GL_MAX_VERTEX_ATTRIB_STRIDE, they had to
> > hope for the best that the driver and hardware can safely handle the
> > used stride values.
> >
> > Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> > ---
> >  src/mesa/vbo/vbo_exec_draw.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/vbo/vbo_exec_draw.c b/src/mesa/vbo/vbo_exec_draw.c
> > index 8d74725db3..0ab9a0d3ea 100644
> > --- a/src/mesa/vbo/vbo_exec_draw.c
> > +++ b/src/mesa/vbo/vbo_exec_draw.c
> > @@ -177,6 +177,29 @@ vbo_exec_bind_arrays(struct gl_context *ctx)
> >     struct gl_vertex_array_object *vao = vbo->VAO;
> >     struct vbo_exec_context *exec = &vbo->exec;
> >
> > +   const GLuint stride = exec->vtx.vertex_size*sizeof(GLfloat);
> > +
> > +   /* Bail out with out of memory, when the required stride exceeds the
> > +    * maximum possible stride.
> > +    * Etnaviv can only handle less than the OpenGL standard mandated
> > +    * minimum of 2048. For this one, at least give some 'resource
> > +    * exhaustion error' to the application.
> > +    * The virgl driver provides 0 for Const.MaxVertexAttribStride
> > +    * as it may not be able to query the relevant informatition from the
> > +    * underlying OpenGL stack. In this case we just make the assumption
> > +    * that the underlying OpenGL stack is able to cope with then commonly
> > +    * seen small strides. Note that this is the same than what applications
> > +    * had to do before the queries to GL_MAX_VERTEX_ATTRIB_STRIDE, they had to
> > +    * hope for the best that the driver and hardware can safely handle the
> > +    * used stride values.
> > +    */
> 
> Do we really need to duplicate the commit message text here? If the driver fetch
> up e.g. by supporting other/newer GPUs with a more sane limit the patch author
> needs to update this comment too.

No, we do not need to duplicate that.
What I wanted to do here initially is to describe which cases the code below is
supposed to handle. Then, to back up why these cases are needed name the driver
backends that currently need that behavior.
Still, I would consider virgil something special as it may not be able to know the limit.

So, how about:

  /* Bail out with out of memory, when the required stride exceeds the
   * maximum possible stride.
   * Note that mesa currently has drivers that can only handle less than
   * the OpenGL standard mandated minimum of 2048. For these, at least give
   * some 'resource exhaustion error' to the application.
   * The virgl driver provides 0 for Const.MaxVertexAttribStride
....
    */


best

Mathias