[Mesa-dev] [PATCH] virgl: add initial shader_storage_buffer_object support. (v2)

[Dave Airlie]

On 20 July 2018 at 21:13, Gert Wollny <gert.wollny at collabora.com> wrote:
> There still seem to be a few quirks (see below)
>
> Best,

>>
>> diff --git a/src/gallium/drivers/virgl/virgl_buffer.c

>>
>> +static void virgl_set_shader_buffers(struct pipe_context *ctx,
>> +                                     enum pipe_shader_type shader,
>> +                                     unsigned start_slot, unsigned
>> count,
>> +                                     const struct pipe_shader_buffer
>> *buffers)
>> +{
>> +   struct virgl_context *vctx = virgl_context(ctx);
>> +   struct virgl_screen *rs = virgl_screen(ctx->screen);
>> +
>> +   for (unsigned i = 0; i < count; i++) {
>> +      unsigned idx = start_slot + i;
>> +
>> +      if (buffers) {
>> +         if (buffers[i].buffer) {
>> +            pipe_resource_reference(&vctx->ssbos[shader][idx],
>> buffers[i].buffer);
>> +            continue;
>> +         }
>> +      }
>> +      pipe_resource_reference(&vctx->ssbos[shader][idx], NULL);
>> +   }
>> +
>> +   uint32_t max_shader_buffer = shader == PIPE_SHADER_FRAGMENT ?
>> +      rs->caps.caps.v2.max_shader_buffer_frag_compute :
> Shouldn't max_shader_buffer_frag_compute also be used for
> PIPE_SHADER_COMPUTE?

virgl doesn't support compute shaders yet. Adding support for them is in
the future patches.

>> +   struct pipe_resource
>> *ssbos[PIPE_SHADER_TYPES][PIPE_MAX_SHADER_BUFFERS];
>>     int num_transfers;
>>     int num_draws;
>>     struct list_head to_flush_bufs;
>> diff --git a/src/gallium/drivers/virgl/virgl_encode.c
>> b/src/gallium/drivers/virgl/virgl_encode.c
>> index c1af01b6fdf..b09366dcee6 100644
>> --- a/src/gallium/drivers/virgl/virgl_encode.c
>> +++ b/src/gallium/drivers/virgl/virgl_encode.c
>> @@ -918,3 +918,28 @@ int virgl_encode_set_tess_state(struct
>> virgl_context *ctx,
>>        virgl_encoder_write_dword(ctx->cbuf, fui(inner[i]));
>>     return 0;
>>  }
>> +
>> +int virgl_encode_set_shader_buffers(struct virgl_context *ctx,
>> +                                    enum pipe_shader_type shader,
>> +                                    unsigned start_slot, unsigned
>> count,
>> +                                    const struct pipe_shader_buffer
>> *buffers)
>> +{
>> +   int i;
>> +   virgl_encoder_write_cmd_dword(ctx,
>> VIRGL_CMD0(VIRGL_CCMD_SET_SHADER_BUFFERS, 0,
>> VIRGL_SET_SHADER_BUFFER_SIZE(count)));
>> +
>> +   virgl_encoder_write_dword(ctx->cbuf, shader);
>> +   virgl_encoder_write_dword(ctx->cbuf, start_slot);
>> +   for (i = 0; i < count; i++) {
>> +      if (buffers) {
> Is it legal to have (count > 0) but buffers= NULL?

Yes, it appears we get 0, 16, NULL to reset all the buffers,
at least I definitely remember adding this because it was possible.


>
>> @@ -302,6 +303,8 @@ struct virgl_caps_v2 {
>>          uint32_t capability_bits;
>>          uint32_t msaa_sample_positions[8];
>>          uint32_t max_vertex_attrib_stride;
>> +        uint32_t max_shader_buffer_frag_compute;
>> +        uint32_t max_shader_buffer_other_stages;
> On the host you use a combined value
>   uint32_t shader_storage_buffer_object_maximums;
> Maybe using two uint16_t on both sides would be better?

The final git version on those host side looks like this.

>> +      case PIPE_SHADER_CAP_MAX_SHADER_BUFFERS:
>> +         if (shader == PIPE_SHADER_FRAGMENT)
>> +            return vscreen-
>> >caps.caps.v2.max_shader_buffer_frag_compute;
> Same like above: PIPE_SHADER_COMPUTE?

We don't support COMPUTE shaders yet.

Dave.