[Mesa-dev] [PATCH] virgl: add shader offset alignment to to v2 caps struct

[Gurchetan Singh]

Got it, thanks.  Let me send a patch to virglrenderer to change
tgsi_invariant to some sort of bitset, since when it lands it'll just
need one bit.
On Mon, Jun 4, 2018 at 9:39 AM Gert Wollny <gw.fossdev at gmail.com> wrote:
>
> Am Montag, den 04.06.2018, 09:07 -0700 schrieb Gurchetan Singh:
> > The tgsi_invariant code never landed in the Mesa side.  Is the caps
> > code is resilient enough to handle this case?  From my understanding
> > the guest caps and host caps don't have to match exactly, and the
> > "right thing" will be done.
>
> I don't think so. When I implemented what I sent as RFC to the virgl
> list, the tgsi_invariant and shader_buffer_offset_alignment were in the
> virglrenderer caps.v2 struct but not in the guest side version of the
> struct, and since I added the new values after these two in
> virglrenderer the output was garbage, but after adding these two values
> also in the guest side struct the output was okay.
>
> Brousing through the qemu code it seems to me that the caps structure
> is simply send as a blob, and both sides have to know the layout to
> make sense of it.
>
> Best,
> Gert
>
> > On Mon, Jun 4, 2018 at 6:03 AM Gert Wollny <gw.fossdev at gmail.com>
> > wrote:
> > >
> > > Am Donnerstag, den 12.04.2018, 18:11 -0700 schrieb
> > > gurchetansingh at chromium.org:
> > > > This is the SSBO analogue to fe0647. User supplied data must
> > > > be a multiple of GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT.
> > > >
> > > > This fixes 44 GLES31 tests on airlied@'s GLES31 sketch branches
> > > > with
> > > > Nvidia hardware, but this patch standalone can applied to master.
> > > > The
> > > > alignment restriction on Nvidia is 32, hence the default value.
> > > >
> > > > Example tests:
> > > >    dEQP-GLES31.functional.ssbo.layout.random.all_shared_buffer.0
> > > >    dEQP-
> > > > GLES31.functional.ssbo.layout.multi_basic_types.single_buffer.std
> > > > 430
> > > >
> > > > v2: Move to a better place in case statement
> > > > ---
> > > >  src/gallium/drivers/virgl/virgl_hw.h     | 1 +
> > > >  src/gallium/drivers/virgl/virgl_screen.c | 3 ++-
> > > >  src/gallium/drivers/virgl/virgl_winsys.h | 1 +
> > > >  3 files changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/gallium/drivers/virgl/virgl_hw.h
> > > > b/src/gallium/drivers/virgl/virgl_hw.h
> > > > index 93849c03dd..624be3b271 100644
> > > > --- a/src/gallium/drivers/virgl/virgl_hw.h
> > > > +++ b/src/gallium/drivers/virgl/virgl_hw.h
> > > > @@ -286,6 +286,7 @@ struct virgl_caps_v2 {
> > > >          int32_t max_texture_gather_offset;
> > > >          uint32_t texture_buffer_offset_alignment;
> > > >          uint32_t uniform_buffer_offset_alignment;
> > > > +        uint32_t shader_buffer_offset_alignment;
> > > >  };
> > >
> > > This conflicts with
> > >
> > >    https://patchwork.freedesktop.org/patch/216081/
> > >
> > > (Dave, you wrote there that this landed,  but I don't see it on the
> > > mesa side).
> > >
> > > In virglrenderer the order is
> > >     ...
> > >     uint32_t uniform_buffer_offset_alignment;
> > >     uint32_t tgsi_invariant;
> > >     uint32_t shader_buffer_offset_alignment;
> > >
> > > Best,
> > > Gert
> > >
> > > >
> > > >  union virgl_caps {
> > > > diff --git a/src/gallium/drivers/virgl/virgl_screen.c
> > > > b/src/gallium/drivers/virgl/virgl_screen.c
> > > > index 02613f1866..f183752d63 100644
> > > > --- a/src/gallium/drivers/virgl/virgl_screen.c
> > > > +++ b/src/gallium/drivers/virgl/virgl_screen.c
> > > > @@ -198,6 +198,8 @@ virgl_get_param(struct pipe_screen *screen,
> > > > enum
> > > > pipe_cap param)
> > > >        return vscreen->caps.caps.v1.bset.has_sample_shading;
> > > >     case PIPE_CAP_CULL_DISTANCE:
> > > >        return vscreen->caps.caps.v1.bset.has_cull;
> > > > +   case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
> > > > +      return vscreen-
> > > > >caps.caps.v2.shader_buffer_offset_alignment;
> > > >     case PIPE_CAP_TEXTURE_GATHER_SM5:
> > > >     case PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT:
> > > >     case PIPE_CAP_FAKE_SW_MSAA:
> > > > @@ -227,7 +229,6 @@ virgl_get_param(struct pipe_screen *screen,
> > > > enum
> > > > pipe_cap param)
> > > >     case PIPE_CAP_TGSI_PACK_HALF_FLOAT:
> > > >     case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
> > > >     case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
> > > > -   case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
> > > >     case PIPE_CAP_INVALIDATE_BUFFER:
> > > >     case PIPE_CAP_GENERATE_MIPMAP:
> > > >     case PIPE_CAP_SURFACE_REINTERPRET_BLOCKS:
> > > > diff --git a/src/gallium/drivers/virgl/virgl_winsys.h
> > > > b/src/gallium/drivers/virgl/virgl_winsys.h
> > > > index 99e98ad9c9..c36b957aba 100644
> > > > --- a/src/gallium/drivers/virgl/virgl_winsys.h
> > > > +++ b/src/gallium/drivers/virgl/virgl_winsys.h
> > > > @@ -134,5 +134,6 @@ static inline void
> > > > virgl_ws_fill_new_caps_defaults(struct virgl_drm_caps *caps)
> > > >     caps->caps.v2.max_texture_gather_offset = 7;
> > > >     caps->caps.v2.texture_buffer_offset_alignment = 32;
> > > >     caps->caps.v2.uniform_buffer_offset_alignment = 256;
> > > > +   caps->caps.v2.shader_buffer_offset_alignment = 32;
> > > >  }
> > > >  #endif