[Mesa-dev] [PATCH 61/78] i965/nir/vec4: Add skeleton implementation of nir_emit_texture()
Jason Ekstrand
jason at jlekstrand.net
Mon Jul 13 07:26:16 PDT 2015
On Jul 13, 2015 7:17 AM, "Jason Ekstrand" <jason at jlekstrand.net> wrote:
>
>
> On Jul 13, 2015 7:14 AM, "Alejandro Piñeiro" <apinheiro at igalia.com> wrote:
> >
> >
> >
> > On 13/07/15 14:05, Jason Ekstrand wrote:
> > > On Mon, Jul 13, 2015 at 3:00 AM, Alejandro Piñeiro <
apinheiro at 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.
> >
> > 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]
> > >>
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 <
elima at igalia.com> wrote:
> > >>>> From: Alejandro Piñeiro <apinheiro at 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 at lists.freedesktop.org
> > >>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >>> _______________________________________________
> > >>> mesa-dev mailing list
> > >>> mesa-dev at lists.freedesktop.org
> > >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >> --
> > >> Alejandro Piñeiro (apinheiro at igalia.com)
> > >>
> > >> _______________________________________________
> > >> mesa-dev mailing list
> > >> mesa-dev at lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> > --
> > Alejandro Piñeiro (apinheiro at igalia.com)
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150713/6bb192b2/attachment-0001.html>
More information about the mesa-dev
mailing list