On Mon, Jul 13, 2015 at 3:00 AM, Alejandro Piñeiro <apinhe...@igalia.com> 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. 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. That's probably what we should be doing in the FS now that the old visitor is gone. --Jason > BR > > [1] > https://github.com/Igalia/mesa/commit/e81ce150ef931e50b6cb1aae0b42a79f448863f5 > [2] https://bugs.freedesktop.org/show_bug.cgi?id=89580#c7 >> --Jason >> >> On Fri, Jun 26, 2015 at 1:07 AM, Eduardo Lima Mitev <el...@igalia.com> wrote: >>> From: Alejandro Piñeiro <apinhe...@igalia.com> >>> >>> 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: https://bugs.freedesktop.org/show_bug.cgi?id=89580 >>> --- >>> 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 >>> mesa-dev@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > -- > Alejandro Piñeiro (apinhe...@igalia.com) > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev