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

[Ian Romanick]

On 05/17/2018 11:56 PM, Mathias Fröhlich wrote:
> 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.

Or maybe leave the assert but have virgl return a real value instead of
0?  The whole reason that _mesa_init_constants sets things to sensible
default values is so that we don't need workarounds in the main code for
drivers that don't support the functionality.

> 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
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>