[virglrenderer-devel] [PATCH] vrend: Enable vertex_attrib_binding also on Hosts with GLES >= 3.1

[Gert Wollny]

Am Freitag, den 25.05.2018, 09:02 -0700 schrieb Gurchetan Singh:
> On Mon, May 21, 2018 at 8:46 AM Gert Wollny <gert.wollny at collabora.co
> m> wrote:
> > On GLES >= 3.1 the functionality that is currently relevant for
> > attribute 
> > binding in virglrenderer is available. by enabling this
> > functionality the new 
> > code path vrend_draw_bind_vertex_binding is used.
> > 
> > Fixes: 
> >   dEQP-GLES3.functional.clipping.point.wide_point_z_clip
> >   dEQP-
> > GLES3.functional.clipping.point.wide_point_z_clip_viewport_center
> >   dEQP-
> > GLES3.functional.clipping.point.wide_point_z_clip_viewport_corner
> > 
> > On an Intel Kabylake host this also fixes:
> > 
> >   dEQP-GLES3.functional.clipping.point.wide_point_clip
> >   dEQP-
> > GLES3.functional.clipping.point.wide_point_clip_viewport_center
> >   dEQP-
> > GLES3.functional.clipping.point.wide_point_clip_viewport_corner
> > 
> > but it should be noted that these three tests expect that also
> > points with a 
> > centre outside the clip volume will be partially drawn when they
> > overlap with 
> > the clip volume because of being "wide". The OpenGL standard
> > defines that these 
> > points should be clipped completely.
> 
> Is this a test problem or a problem
> with vrend_draw_bind_vertex_legacy?
> Would the test pass if the points are completely clipped?

Both, all tests don't pass with the legacy code path, but the latter
three test are not written against the OpenGL standard, that expects
the points to be clipped complitely (done, e.g. on r600). The test
expects that the points are partially drawn, even if the center is
outside (done on Intel and presumably also on the Nvidia blob). 
 
> > GLES 3.1 does not completely implement
> > GL_ARB_vertex_attrib_binding, so it may 
> > become necessary to add another flag that would indicate that only
> > GLES style 
> > vertex_attrib_binding is available if virglrenderer would want to
> > use this
> > additional functionality. Namely 
> >   VertexAttribLPointer
> > is not available, and a few query constants are missing from GLES
> > 3.1 with 
> > respect to the extension.
> > 
> > Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> > ---
> > PS: obviously the bug in vrend_draw_bind_vertex_legacy that made
> > these tests fail is still there. If someone wants to investigate:
> > The point size is not properly set.
> >  src/vrend_renderer.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index 8cd9109..464560e 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -4203,7 +4203,8 @@ int vrend_renderer_init(struct vrend_if_cbs
> > *cbs, uint32_t flags)
> > 
> >     if (epoxy_has_gl_extension("GL_MESA_pack_invert"))
> >        vrend_state.have_mesa_invert = true;
> > -   if (gl_ver >= 43 ||
> > epoxy_has_gl_extension("GL_ARB_vertex_attrib_binding"))
> > +   if (gl_ver >= 43 || (gles && gl_ver >= 31) ||
> > +       epoxy_has_gl_extension("GL_ARB_vertex_attrib_binding"))
> 
> Since GLES31 doesn't fully implement this, we can not advertise
> this.  You can also do
The problem is there are more places in the code where GLES31
functionality related to vertex_attrib_binding is used, and these code
path are used when this flag is set. If we use the flag to advertise
complete support for the extension, then you are of course right, but
then my proposal would be to rename this flag to indidate that this is
not the full extension, but only what GLES31 implements so that these
code path are used when available. We could then add another flag when
the additional functionality provided by the extension is actually
needed. 

> 
>    if (vrend_state.have_vertex_attrib_binding ||
> (vrend_state.use_gles && vrend_state.gl_major_ver >= 3 && 
> vrend_state.gl_minor_ver >= 1)
>       vrend_draw_bind_vertex_binding(ctx, ctx->sub->ve);
>    else 
>       vrend_draw_bind_vertex_legacy(ctx, ctx->sub->ve);
> 
> It's a little more verbose, but also more correct.
You forgot the possible future version gles 4.0 ;) 


Best, 
Gert 

>  
> >        vrend_state.have_vertex_attrib_binding = true;
> >     if (gl_ver >= 33 ||
> > epoxy_has_gl_extension("GL_ARB_sampler_objects"))
> >        vrend_state.have_samplers = true;
> > _______________________________________________
> > virglrenderer-devel mailing list
> > virglrenderer-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel