[Mesa-dev] [PATCH] vbo: remove MaxVertexAttribStride assert check.

[Mathias Fröhlich]

Hi Dave,

On Friday, 18 May 2018 06:57:19 CEST Dave Airlie wrote:
> > May be I should take care of all of these type of asserts, also the ones
> > with MaxVertexAttribRelativeOffset and care for not checking them
> > when the extension version is unavailable or checking against the OpenGL
> > spec guaranteed minimum values for both constants instead of the actual ones.
> > ... means, there are more asserts of this kind.
> >
> > Well, alternatively since you probably aim for supporting GL4.4 at one point, you
> > could alternatively set the constant already? AFAICT the
> > PIPE_CAP_MAX_VERTEX_ATTRIB_STRIDE is only used to set the
> > constant and does not imply anything beyond.
> 
> Well it's not just virgl, won't this break things like r300 and i915?

I dont think so.

The constant is set to the opengl mandated minimum of 2048 in
_mesa_init_constants which seems not overwritten by any classic driver.
In gallium there is the cap which overwrites the constant past setting
it from mesa. As far as I greped yesterday, only virgil just returns 0.
The rest returns the mandated 2048, there is one exception with etnaviv
that returns 128 when being asked for PIPE_CAP_MAX_VERTEX_ATTRIB_STRIDE.
If you look into etnaviv this stems from having only 8 bits in the hardware
register for the stride.

So, I realize, that I have been taking the constant MaxVertexAttribStride 
as either a well maintained hardware limit or at least as a lower bound to 
that hardware limit. Means I did not expect this presence of that
value (well being != 0, I mean) to be dependent on the OpenGL version
but on the backing driver.

What does that mean to virgl. Well, I see you cant get (OpenGL < 4.4 or no
GL_ARB_vertex_attrib_binding) or dont know (virgil does not ask the host
currently) the real value.
Also the current implementation just checks this to give a some hint what goes
wrong, especially there should be no crash if the assert fails.
That together is why I just said 'fine, I have to treat that differently, you get my r-b
so that my assert gets out of your way'.

Putting that all together, I see the route:
* just remove the assert, cross you fingers and dont check at all.
   Most likely it all works.
   All modern hardware can do 2048 and beyond and the vbo module
   can only produce strides up to VBO_ATTRIB_MAX*4*4 < 2048.
   It wont work if the virtualization host runs etnaviv or equivalent
   limited and somebody does a high count of vertex attributes through
   virgil - unlikely.
* Extend the assert to
  assert(MaxVertexAttribStride == 0 || stride < MaxVertexAttribStride)
  Then virgil does not get the check.
* Replace the assert with a gl error. Then the application gets at least
   a hint that there was some resource exceeded. You will need to give
   some limit for virgl or we play the MaxVertexAttribStride == 0 game
   so that virgil just does not get that checked.

I think I need to downgrade the assert to an error at least as I cannot
internally to mesa guarantee that it has to hold.

I am more concerned about the MaxVertexAttribRelativeOffset constant
which is required from _mesa_update_vao_derived_arrays. It is again set to
the OpenGL mandated minimum of 2047 from _mesa_init_constants. Since
there is no gallium cap for that, it cannot just be set to 0 from virgil.
Etnaviv is again technically not able to handle the OpenGL mandated
minimum but should again work for most of the practical cases.
But finding a zero in MaxVertexAttribRelativeOffset would not work
for _mesa_update_vao_derived_arrays.

best

Mathias