On Sun, Sep 27, 2015 at 5:27 PM, Marek Olšák <[email protected]> wrote: > On Sun, Sep 27, 2015 at 11:10 PM, Ilia Mirkin <[email protected]> wrote: >> On Sun, Sep 27, 2015 at 2:48 PM, Marek Olšák <[email protected]> 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 _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
