[Mesa-dev] [PATCH 06/11] st/mesa: add atomic buffer support

[Marek Olšák]

On Sun, Sep 27, 2015 at 11:10 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Sun, Sep 27, 2015 at 2:48 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> Patches 2-5 are:
>>> +static void st_bind_atomics(struct st_context *st,
>>> +                           struct gl_shader_program *prog,
>>> +                           unsigned shader_type)
>>> +{
>>> +   unsigned i;
>>> +
>>> +   if (!prog)
>>> +      return;
>>> +
>>> +   for (i = 0; i < prog->NumAtomicBuffers; i++) {
>>
>> This loops over all atomic buffers in a shader program, which can
>> contain 5 linked shader stages, so NumAtomicBuffers can be <=
>> MaxCombinedAtomicBuffers. I don't think drivers can handle so many.
>>
>>> +      struct gl_atomic_buffer_binding *binding =
>>> +         &st->ctx->AtomicBufferBindings[prog->AtomicBuffers[i].Binding];
>>> +      struct st_buffer_object *st_obj =
>>> +         st_buffer_object(binding->BufferObject);
>>> +      struct pipe_shader_buffer sb = {};
>>> +
>>> +      pipe_resource_reference(&sb.buffer, st_obj->buffer);
>>> +      sb.buffer_offset = binding->Offset;
>>> +      sb.buffer_size = st_obj->buffer->width0 - binding->Offset;
>>> +
>>> +      /* TODO: cso */
>>
>> You can remove the TODO. I don't see why this would need cso_context support.
>>
>>> +      st->pipe->set_shader_buffers(st->pipe, shader_type,
>>> +                                   i /* XXX */, 1, &sb);
>>
>> What does this XXX mean? This needs a better comment at least.
>
> This is what I was alluding to in the cover letter... it needs to look
> up the buffer index from somewhere, but it's not easily available.
> Timothy has a patch to fix up intel, if that's deemed to be the right
> solution, then I'll copy it in here as well. Perhaps I should just do
> that now and update it later as necessary.

We need tests to be able say for certain that it works before pushing.

>>> @@ -3025,13 +3053,72 @@ glsl_to_tgsi_visitor::get_function_signature(ir_function_signature *sig)
>>>  }
>>>
>>>  void
>>> +glsl_to_tgsi_visitor::visit_atomic_counter_intrinsic(ir_call *ir)
>>> +{
>>> +   const char *callee = ir->callee->function_name();
>>> +   ir_dereference *deref = static_cast<ir_dereference *>(
>>> +      ir->actual_parameters.get_head());
>>> +   ir_variable *location = deref->variable_referenced();
>>> +
>>> +   /* XXX use accept */
>>> +   st_src_reg buffer(
>>> +         PROGRAM_SAMPLER, location->data.binding /* XXX */, GLSL_TYPE_ATOMIC_UINT);
>>
>> Why don't you use accept? What's the second XXX about?
>
> location->data.binding is wrong. I need the equivalent of the sampler
> uniform lookup. But it's what i965 uses.

I think we need a test for this.

>>> @@ -5063,6 +5163,19 @@ compile_tgsi_instruction(struct st_translate *t,
>>>                      src, num_src);
>>>        return;
>>>
>>> +   case TGSI_OPCODE_LOAD:
>>> +   case TGSI_OPCODE_STORE:
>>> +   case TGSI_OPCODE_ATOMUADD:
>>> +   /* XXX the other atomic ops */
>>
>> Do we care about other atomic ops for ARB_shader_atomic_counters? I
>> think the extension only needs LOAD and ATOMUADD, so this XXX can be
>> removed.
>
> For atomic counters that's it -- actually store isn't required
> either.But I think I'm going to go the other way and add them all in
> now instead. This will be necessary for SSBO.

Alright.

Marek