On Jul 13, 2015 7:17 AM, "Jason Ekstrand" <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > > On Jul 13, 2015 7:14 AM, "Alejandro Piñeiro" <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote: > > > > > > > > On 13/07/15 14:05, Jason Ekstrand wrote: > > > On Mon, Jul 13, 2015 at 3:00 AM, Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote: > > >> On 01/07/15 01:37, Jason Ekstrand wrote: > > >>> If we can avoid duplication in the texturing code, that would be > > >>> really nice. Could we do this as a refactor of the old code and then > > >>> a much smaller NIR function that calls some shared code? That's what > > >>> we did for FS and it worked ok. I looked at the layout of your code > > >>> and, after you finish reading instruction inputs, very little of it is > > >>> actually NIR-specific. > > >> Our wip second batch of commits has now a version that refactors the old > > >> code. But I have a doubt. As part of this refactor, I created a new > > >> method to be used by brw_fs and brw_vec4, that returns the ir texture > > >> opcode based on the nir texture opcode. You can take a look to that > > >> method here [1]. The issue with this method is that ir_texture_opcode is > > >> defined at glsl/ir.h inside a #ifdef __cplusplus. So I needed to rename > > >> brw_nir.c to brw_nir.cpp. On a previous early review [2] you mentioned > > >> that it would be better to keep it as brw_nir.c unless we have a good > > >> reason. Is using this enum good reason to do the renaming? If not, the > > >> other option I see is modify glsl/ir.h and move the enum definitions to > > >> the top, outside the ifdef __cplusplus, as other headers (like > > >> glsl/glsl_types.h) have. > > > Go ahead and make it a cpp file if you have to. > > > > Ok, thanks. > > > > > Another option would > > > be to do the map the other way: ir_opcode -> nir_tex_op and refactor > > > the code to use the nir_tex_op. > > > > But even if we do the map in the other way, the helper function would be > > using the enum type defined at glsl/ir.h, so we would need to make it a > > cpp file in any case. So for now, I will keep things simple, and keep > > the current mapping, and do the change later. > > The difference is that, with the patch I sent a few hours ago, you only need to do the conversion one place so there's no need for the helper. For whatever it's worth I'm also OK with just duplicating the 10/lines of code to do the conversion and not bothering with the helper. > > > That's probably what we should be > > > doing in the FS now that the old visitor is gone. > > > --Jason > > > > > >> BR > > >> > > >> [1] > > >> <a href="https://github.com/Igalia/mesa/commit/e81ce150ef931e50b6cb1aae0b42a79f448863f5">https://github.com/Igalia/mesa/commit/e81ce150ef931e50b6cb1aae0b42a79f448863f5</a> > > >> [2] <a href="https://bugs.freedesktop.org/show_bug.cgi?id=89580#c7">https://bugs.freedesktop.org/show_bug.cgi?id=89580#c7</a> > > >>> --Jason > > >>> > > >>> On Fri, Jun 26, 2015 at 1:07 AM, Eduardo Lima Mitev <<a href="mailto:elima@igalia.com">elima@igalia.com</a>> wrote: > > >>>> From: Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> > > >>>> > > >>>> This sets up the basic structure and placeholders for the implementation of > > >>>> texture operations, which will be added incrementally in subsequent patches. > > >>>> > > >>>> Apart from helping split the texture support into smaller patches, this patch > > >>>> is useful to get a simplified picture of the steps involved in the emission of > > >>>> texture operations. > > >>>> > > >>>> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=89580">https://bugs.freedesktop.org/show_bug.cgi?id=89580</a> > > >>>> --- > > >>>> src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 120 ++++++++++++++++++++++++++++- > > >>>> 1 file changed, 119 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp > > >>>> index 08a70b0..c3638a0 100644 > > >>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp > > >>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp > > >>>> @@ -1412,7 +1412,125 @@ vec4_visitor::nir_swizzle_result(nir_tex_instr *instr, dst_reg dest, > > >>>> void > > >>>> vec4_visitor::nir_emit_texture(nir_tex_instr *instr) > > >>>> { > > >>>> - /* @TODO: Not yet implemented */ > > >>>> + unsigned sampler = instr->sampler_index; > > >>>> + src_reg sampler_reg = src_reg(sampler); > > >>>> + > > >>>> + const glsl_type *dest_type; > > >>>> + > > >>>> + /* Load the texture operation sources */ > > >>>> + for (unsigned i = 0; i < instr->num_srcs; i++) { > > >>>> + src_reg src = get_nir_src(instr->src[i].src); > > >>>> + > > >>>> + switch (instr->src[i].src_type) { > > >>>> + case nir_tex_src_comparitor: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_coord: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_ddx: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_ddy: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_lod: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_ms_index: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_offset: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_sampler_offset: > > >>>> + /* @TODO: not yet implemented */ > > >>>> + break; > > >>>> + > > >>>> + case nir_tex_src_projector: > > >>>> + unreachable("Should be lowered by do_lower_texture_projection"); > > >>>> + > > >>>> + case nir_tex_src_bias: > > >>>> + unreachable("LOD bias is not valid for vertex shaders.\n"); > > >>>> + > > >>>> + default: > > >>>> + unreachable("unknown texture source"); > > >>>> + } > > >>>> + } > > >>>> + > > >>>> + /* Get the texture operation opcode */ > > >>>> + enum opcode opcode = shader_opcode_for_nir_opcode(instr->op); > > >>>> + > > >>>> + /* Build the instruction */ > > >>>> + enum glsl_base_type dest_base_type = > > >>>> + brw_glsl_base_type_for_nir_type(instr->dest_type); > > >>>> + > > >>>> + dest_type = > > >>>> + glsl_type::get_instance(dest_base_type, nir_tex_instr_dest_size(instr), 1); > > >>>> + > > >>>> + vec4_instruction *inst = new(mem_ctx) > > >>>> + vec4_instruction(opcode, dst_reg(this, dest_type)); > > >>>> + > > >>>> + for (unsigned i = 0; i < 3; i++) { > > >>>> + if (instr->const_offset[i] != 0) { > > >>>> + inst->offset = brw_texture_offset(instr->const_offset, 3); > > >>>> + break; > > >>>> + } > > >>>> + } > > >>>> + > > >>>> + /* The message header is necessary for: > > >>>> + * - Texel offsets > > >>>> + * - Gather channel selection > > >>>> + * - Sampler indices too large to fit in a 4-bit value. > > >>>> + */ > > >>>> + inst->header_size = > > >>>> + (inst->offset != 0 || > > >>>> + instr->op == nir_texop_tg4 || > > >>>> + is_high_sampler(sampler_reg)) ? 1 : 0; > > >>>> + inst->base_mrf = 2; > > >>>> + inst->mlen = inst->header_size + 1; > > >>>> + inst->dst.writemask = WRITEMASK_XYZW; > > >>>> + > > >>>> + inst->src[1] = sampler_reg; > > >>>> + > > >>>> + /* Calculate the MRF for the first parameter */ > > >>>> + int param_base = inst->base_mrf + inst->header_size; > > >>>> + > > >>>> + if (instr->op == nir_texop_txs || instr->op == nir_texop_query_levels) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } else { > > >>>> + /* @TODO: Load the coordinate */ > > >>>> + > > >>>> + /* @TODO: Load the shadow comparitor */ > > >>>> + > > >>>> + /* Load the LOD info */ > > >>>> + if (instr->op == nir_texop_tex || instr->op == nir_texop_txl) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } else if (instr->op == nir_texop_txf) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } else if (instr->op == nir_texop_txf_ms) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } else if (instr->op == nir_texop_txd) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } else if (instr->op == nir_texop_tg4) { > > >>>> + /* @TODO: not yet implemented */ > > >>>> + } > > >>>> + } > > >>>> + > > >>>> + /* Emit the instruction */ > > >>>> + emit(inst); > > >>>> + > > >>>> + /* Move the result to the destination registry, applying swizzle */ > > >>>> + dst_reg dest = get_nir_dest(instr->dest, brw_type_for_base_type(dest_type)); > > >>>> + > > >>>> + nir_swizzle_result(instr, dest, src_reg(inst->dst), sampler, dest_type); > > >>>> } > > >>>> > > >>>> } > > >>>> -- > > >>>> 2.1.4 > > >>>> > > >>>> _______________________________________________ > > >>>> mesa-dev mailing list > > >>>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > >>>> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > >>> _______________________________________________ > > >>> mesa-dev mailing list > > >>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > >>> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > >> -- > > >> Alejandro Piñeiro (<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>) > > >> > > >> _______________________________________________ > > >> mesa-dev mailing list > > >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > -- > > Alejandro Piñeiro (<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>) > >