<div dir="ltr">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.<div> <div><div>Reviewed-by: Gurchetan Singh <<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>></div></div><div> </div></div></div> <div class="gmail_quote"><div dir="ltr">On Fri, May 25, 2018 at 3:26 PM Gert Wollny <<a href="mailto:gert.wollny@collabora.com" target="_blank">gert.wollny@collabora.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Am Freitag, den 25.05.2018, 09:02 -0700 schrieb Gurchetan Singh: > On Mon, May 21, 2018 at 8:46 AM Gert Wollny <<a href="mailto:gert.wollny@collabora.co" target="_blank">gert.wollny@collabora.co</a> > 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 <<a href="mailto:gert.wollny@collabora.com" target="_blank">gert.wollny@collabora.com</a>> > > --- > > 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 > > <a href="mailto:virglrenderer-devel@lists.freedesktop.org" target="_blank">virglrenderer-devel@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel</a> </blockquote></div>