[virglrenderer-devel] [PATCH 3/4] shader: add basic shader_storage_buffer_object parsing. (v2)

[Gurchetan Singh]

On Tue, Jul 17, 2018 at 9:11 PM Dave Airlie <airlied at gmail.com> wrote:
>
> On 18 July 2018 at 13:33, Gurchetan Singh <gurchetansingh at chromium.org> wrote:
> > On Tue, Jul 17, 2018 at 7:26 PM Dave Airlie <airlied at gmail.com> wrote:
> >>
> >> From: Dave Airlie <airlied at redhat.com>
> >>
> >> This adds the basic shader parsing for the SSBO extension,
> >>
> >> v2: drop qualifier, cleanup unused var, add indirect support,
> >> use a bitmask to track declared ssbos.
> >> ---
> >>  src/vrend_shader.c | 331 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>  src/vrend_shader.h |   1 +
> >>  2 files changed, 305 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/src/vrend_shader.c b/src/vrend_shader.c
> >> index cbbfbbc..b20d681 100644
> >> --- a/src/vrend_shader.c
> >> +++ b/src/vrend_shader.c
> >> @@ -108,6 +108,28 @@ struct vrend_io_range {
> >>     bool used;
> >>  };
> >>
> >> +enum vrend_type_qualifier {
> >> +   TYPE_CONVERSION_NONE = 0,
> >> +   FLOAT = 1,
> >> +   VEC2 = 2,
> >> +   VEC3 = 3,
> >> +   VEC4 = 4,
> >> +   INT = 5,
> >> +   IVEC2 = 6,
> >> +   IVEC3 = 7,
> >> +   IVEC4 = 8,
> >> +   UINT = 9,
> >> +   UVEC2 = 10,
> >> +   UVEC3 = 11,
> >> +   UVEC4 = 12,
> >> +   FLOAT_BITS_TO_UINT = 13,
> >> +   UINT_BITS_TO_FLOAT = 14,
> >> +   FLOAT_BITS_TO_INT = 15,
> >> +   INT_BITS_TO_FLOAT = 16,
> >> +   DOUBLE = 17,
> >> +   DVEC2 = 18,
> >> +};
> >> +
> >>  struct dump_ctx {
> >>     struct tgsi_iterate_context iter;
> >>     struct vrend_shader_cfg *cfg;
> >> @@ -136,7 +158,12 @@ struct dump_ctx {
> >>
> >>     struct vrend_shader_sampler samplers[32];
> >>     uint32_t samplers_used;
> >> -   bool sviews_used;
> >> +
> >> +   uint32_t ssbo_used_mask;
> >> +   uint32_t ssbo_atomic_mask;
> >> +   uint32_t ssbo_array_base;
> >> +   uint32_t ssbo_atomic_array_base;
> >
> > The v1 representation:
> >
> > struct vrend_ssbo {
> >    uint32_t id;
> >    unsigned atomic;
> >    enum vrend_type_qualifier type;
> > };
> >
> > would also allow us to fix:
> >
> > https://gitlab.freedesktop.org/airlied/virglrenderer/issues/3
> >
> > We'll have to loop over active SSBOs and change the type depending on
> > opcodes (and throw an error if the typing isn't consistent).  We can
> > add in the qualifier in follow-up patches, but we'll need an array
> > representation ...
>
> I think fixing that will be a thing all on it's own, I think we'd have
> to have another
> mask for ones that are int based instead of uint, and that would
> likely cover it.

STGM, a separate mask for int should also works.

>
> I'd rather not worry about that until after we land the initial work.
>
> >> +         if (decl->Range.First < ctx->ssbo_atomic_array_base)
> >> +            ctx->ssbo_atomic_array_base = decl->Range.First;
> >> +         ctx->ssbo_atomic_mask |= (1 << decl->Range.First);
> >> +      } else {
> >> +         if (decl->Range.First < ctx->ssbo_array_base)
> >> +            ctx->ssbo_array_base = decl->Range.First;
> >
> > Both ctx->ssbo_atomic_array_base and ctx->ssbo_array_base are
> > decl->Range.First.  What's the difference?
>
> We have two potential arrays, one of atomic buffers and one of ssbos,
> usually we'd end up with something like
> buffer[0], ATOMIC
> buffer[6]
> buffer[7]
> in the decls, so we need to track the base for both atomic and non-atomic cases
> so we get the indirect calculations correct.

Thank, got it.  This series LGTM, though I'll let Tomeu add the r-b
once his comments are addressed.

> Dave.