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

[Erik Faye-Lund]

On 01. aug. 2018 13:37, Gert Wollny wrote:
> 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 ...

I would still fix it, if only to make it easier to understand the 
history in retrospect. But I don't really care that much either, so you 
can decide for yourself.

>>>    {
>>> -   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
> ;)
>   

Ah, thanks for pointing this out. Feel free to leave this patch as is if 
you want :)