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

[Gurchetan Singh]

I see, vrend_state.have_vertex_attrib_binding is indeed used in multiple
other places.  However, we only seem to use the functionality available in
GLES31.  This looks like the most sensible approach then.

Reviewed-by: Gurchetan Singh <gurchetansingh at chromium.org>


On Fri, May 25, 2018 at 3:26 PM Gert Wollny <gert.wollny at collabora.com>
wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180525/54ce1ec3/attachment.html>