[Mesa-dev] [PATCH 06/11] st/mesa: add atomic buffer support
Marek Olšák
maraeo at gmail.com
Sun Sep 27 14:27:26 PDT 2015
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
More information about the mesa-dev
mailing list