[virglrenderer-devel] [PATCH 6/6] renderer: add shader_storage_buffer_object support. (v2)

[Tomeu Vizoso]

On 07/18/2018 07:58 PM, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> This pulls the code out from the gles31 development,
> and modifies the caps to support two different limits
> 
> (so far I've only found fs/cs vs everyone else limits differ)
> 
> v2: fix buffer creation paths, limit maximums, handle indirect
> (don't pass -1 into gl funcs when we don't need to).
> 
> Co-authors: Gurchetan Singh <gurchetansingh at chromium.org>
> ---

[snip]

> @@ -2255,6 +2289,32 @@ void vrend_set_num_sampler_views(struct vrend_context *ctx,
>      ctx->sub->views[shader_type].num_views = last_slot;
>   }
>   
> +void vrend_set_single_ssbo(struct vrend_context *ctx,
> +                           uint32_t shader_type,
> +                           int index,
> +                           uint32_t offset, uint32_t length,
> +                           uint32_t handle)
> +{
> +   struct vrend_ssbo *ssbo = &ctx->sub->ssbo[shader_type][index];
> +   struct vrend_resource *res;
> +   if (handle) {
> +      res = vrend_renderer_ctx_res_lookup(ctx, handle);
> +      if (!res) {
> +         report_context_error(ctx, VIRGL_ERROR_CTX_ILLEGAL_RESOURCE, handle);
> +         return;
> +      }
> +      ssbo->buffer_offset = offset;
> +      ssbo->buffer_size = length;
> +      ssbo->res = res;
> +      ctx->sub->ssbo_used_mask[shader_type] |= (1 << index);
> +   } else {
> +      ssbo->res = 0;
> +      ssbo->buffer_offset = 0;
> +      ssbo->buffer_size = 0;

tiny nit: set these members in the same order as in the previous block?

> @@ -3148,6 +3208,32 @@ static void vrend_draw_bind_const_shader(struct vrend_context *ctx,
>      }
>   }
>   
> +static void vrend_draw_bind_ssbo_shader(struct vrend_context *ctx, int shader_type)
> +{
> +   uint32_t mask;
> +   struct vrend_ssbo *ssbo;
> +   struct vrend_resource *res;
> +   int i;
> +
> +   if (!ctx->sub->prog->ssbo_locs[shader_type])
> +      return;
> +
> +   if (!ctx->sub->ssbo_used_mask[shader_type])
> +      return;

Isn't this check redundant?

> @@ -7579,7 +7670,19 @@ void vrend_renderer_fill_caps(uint32_t set, uint32_t version,
>   
>      if (gl_ver >= 43) {
>         glGetIntegerv(GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT, (GLint*)&caps->v2.texture_buffer_offset_alignment);
> +   }
> +
> +   if (gl_ver >= 43 || epoxy_has_gl_extension("GL_ARB_shader_storage_buffer_object")) {
>         glGetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, (GLint*)&caps->v2.shader_buffer_offset_alignment);
> +
> +      glGetIntegerv(GL_MAX_VERTEX_SHADER_STORAGE_BLOCKS, &max);
> +      if (max > PIPE_MAX_SHADER_BUFFERS)
> +         max = PIPE_MAX_SHADER_BUFFERS;
> +      caps->v2.shader_storage_buffer_object_maximums = (max << 16);
> +      glGetIntegerv(GL_MAX_FRAGMENT_SHADER_STORAGE_BLOCKS, &max);
> +      if (max > PIPE_MAX_SHADER_BUFFERS)
> +         max = PIPE_MAX_SHADER_BUFFERS;
> +      caps->v2.shader_storage_buffer_object_maximums |= max & 0xffff;

Are we saving a few bytes at the expense of readability here or am I 
missing something?

Cheers,

Tomeu