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. > > 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>) >