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

[Ilia Mirkin]

On Sun, Sep 27, 2015 at 5:27 PM, Marek Olšák <maraeo at gmail.com> wrote:
> 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.

We got one, and it fails :) On i965 as well, for that matter --
arb_shader_atomic_counters-semantics. I'll update this change with
Timothy's technique and make sure it works.

>
>>>> @@ -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