[virglrenderer-devel] [PATCH v4 2/7] vrend, caps: Unify feature code path and remove duplicates

[Gert Wollny]

Am Mittwoch, den 01.08.2018, 12:45 +0200 schrieb Erik Faye-Lund:
> 
> On 01. aug. 2018 12:15, Gert Wollny wrote:
> > Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> > Reviewed-by: Gurchetan Singh <gurchetansingh at chromium.org>
> > ---
> >   src/vrend_renderer.c | 52 ++++++++-------------------------------
> > -----
> >   1 file changed, 9 insertions(+), 43 deletions(-)
> > 
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index 8664b91..7471abc 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -7846,13 +7846,9 @@ static void
> > vrend_renderer_fill_caps_v2_common(union virgl_caps *caps)
> >      glGetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT,
> > (GLint*)&caps->v2.uniform_buffer_offset_alignment);
> >   }
> >   
> > -static void vrend_renderer_fill_caps_gles(bool fill_capset2, int
> > gles_ver,
> > -					  union virgl_caps *caps)
> > +static void vrend_renderer_fill_caps_gles(UNUSED bool
> > fill_capset2, int gles_ver,
> > +                                          union virgl_caps *caps)
> 
> Why not just remove the fill_capset2 argument? There's just one 
> call-site at this point, and no obvious reason to keep the API as-
> is...
This function goes away in later patches, so this is kind of moot ... 

> 
> >   {
> > -   GLint max;
> > -
> > -   caps->v1.max_viewports = 1;
> > -
> >      if (gles_ver >= 30) {
> >         caps->v1.glsl_level = 130;
> >      } else {
> > @@ -7860,44 +7856,9 @@ static void
> > vrend_renderer_fill_caps_gles(bool fill_capset2, int gles_ver,
> >      }
> >   
> >      if (gles_ver >= 30) {
> > -      glGetIntegerv(GL_MAX_ARRAY_TEXTURE_LAYERS, &max);
> > -      caps->v1.max_texture_array_layers = max;
> >         caps->v1.bset.primitive_restart = 1;
> >      }
> >   
> > -   if (gles_ver >= 30) {
> > -      caps->v1.bset.instanceid = 1;
> > -      glGetIntegerv(GL_MAX_VERTEX_UNIFORM_BLOCKS, &max);
> > -      vrend_state.max_uniform_blocks = max;
> > -      caps->v1.max_uniform_blocks = max + 1;
> > -   }
> > -
> > -   if (has_feature(feat_transform_feedback)) {
> > -      glGetIntegerv(GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS,
> > &max);
> > -      /* As with the earlier version of transform feedback this
> > min 4. */
> > -      if (max >= 4) {
> > -         caps->v1.max_streamout_buffers = 4;
> > -      }
> > -   }
> > -
> > -   if (gles_ver >= 30) {
> > -      caps->v1.bset.texture_multisample = 1;
> > -   }
> > -
> > -   if (!fill_capset2) {
> > -      return;
> > -   }
> > -
> > -   if (gles_ver >= 31)
> > -      glGetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT,
> > (GLint*)&caps->v2.shader_buffer_offset_alignment);
> > -
> > -   if (gles_ver >= 31)
> > -      glGetIntegerv(GL_MAX_VERTEX_ATTRIB_STRIDE, (GLint*)&caps-
> > >v2.max_vertex_attrib_stride);
> > -
> > -   caps->v1.max_samples =
> > vrend_renderer_query_multisample_caps(max, &caps->v2);
> > -
> > -   if (has_feature(feat_copy_image))
> > -      caps->v2.capability_bits |= VIRGL_CAP_COPY_IMAGE;
> >   }
> >   
> >   static void vrend_renderer_fill_caps_gl(bool fill_capset2, int
> > gl_ver,
> > @@ -7977,7 +7938,6 @@ void vrend_renderer_fill_caps(uint32_t set,
> > UNUSED uint32_t version,
> >      /* GLES has it's own path */
> >      if (vrend_state.use_gles) {
> >         vrend_renderer_fill_caps_gles(fill_capset2, gles_ver,
> > caps);
> > -      return;
> >      } else
> >         vrend_renderer_fill_caps_gl(fill_capset2, gl_ver, caps);
> >   
> > @@ -8102,6 +8062,12 @@ void vrend_renderer_fill_caps(uint32_t set,
> > UNUSED uint32_t version,
> >         if (has_feature(feat_transform_feedback3)) {
> >            glGetIntegerv(GL_MAX_TRANSFORM_FEEDBACK_BUFFERS, &max);
> >            caps->v1.max_streamout_buffers = max;
> > +      } else if (gles_ver > 0) {
> > +         glGetIntegerv(GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS,
> > &max);
> > +         /* As with the earlier version of transform feedback this
> > min 4. */
> > +         if (max >= 4) {
> > +            caps->v1.max_streamout_buffers = 4;
> > +         }
> 
> This used to be inside an "if 
> (has_feature(feat_transform_feedback))"-block instead of an "if 
> (gles_ver > 0) {"-block.

It still is (note the indention), it's just not visible in the patch
;) 
 
> 
> Dropping this will generate errors on GLES 2 without 
> GL_EXT_transform_feedback, where 
> GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS isn't a legal enum.
> 
> >         } else
> >            caps->v1.max_streamout_buffers = 4;
> >      }
> > @@ -8183,7 +8149,7 @@ void vrend_renderer_fill_caps(uint32_t set,
> > UNUSED uint32_t version,
> >   
> >      caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT |
> > VIRGL_CAP_SET_MIN_SAMPLES | VIRGL_CAP_TGSI_PRECISE;
> >   
> > -   if (gl_ver >= 44)
> > +   if (gl_ver >= 44 || gles_ver >= 31)
> >         glGetIntegerv(GL_MAX_VERTEX_ATTRIB_STRIDE, (GLint*)&caps-
> > >v2.max_vertex_attrib_stride);
> >   
> >      if (has_feature(feat_compute_shader)) {
> 
> With those two fixed:
> 
> Reviewed-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel