On Sun, Apr 14, 2019 at 11:27 AM Christian Gmeiner
<christian.gmei...@gmail.com> wrote:
>
> Am So., 14. Apr. 2019 um 17:45 Uhr schrieb Alyssa Rosenzweig
> <aly...@rosenzweig.io>:
> >
> > On Mali hardware (supported by Panfrost and Lima), the fixed-function
> > transformation from world-space to screen-space coordinates is done in
> > the vertex shader prior to writing out the gl_Position varying, rather
> > than in dedicated hardware. This commit adds a shared NIR pass for
> > implementing coordinate transformation and lowering gl_Position writes
> > into screen-space gl_Position writes.
> >
> > v2: Run directly on derefs before io/vars are lowered to cleanup the
> > code substantially. Thank you to Qiang for this suggestion!
> >
> > v3: Bikeshed continues.
> >
> > v4: Add to Makefile.sources (per Jason's comment). Bikeshed comment.
> > (Trivial -- reviews are from v3)
> >
> > Signed-off-by: Alyssa Rosenzweig <aly...@rosenzweig.io>
> > Suggested-by: Qiang Yu <yuq...@gmail.com>
> > Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
> > Reviewed-by: Qiang Yu <yuq...@gmail.com>
> > Cc: Jason Ekstrand <ja...@jlekstrand.net>
> > Cc: Eric Anholt <e...@anholt.net>
> > ---
> > src/compiler/Makefile.sources | 1 +
> > src/compiler/nir/meson.build | 1 +
> > src/compiler/nir/nir.h | 1 +
> > .../nir/nir_lower_viewport_transform.c | 102 ++++++++++++++++++
> > 4 files changed, 105 insertions(+)
> > create mode 100644 src/compiler/nir/nir_lower_viewport_transform.c
> >
> > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> > index 5737a827daa..35dd7ff1abe 100644
> > --- a/src/compiler/Makefile.sources
> > +++ b/src/compiler/Makefile.sources
> > @@ -273,6 +273,7 @@ NIR_FILES = \
> > nir/nir_lower_vars_to_ssa.c \
> > nir/nir_lower_var_copies.c \
> > nir/nir_lower_vec_to_movs.c \
> > + nir/nir_lower_viewport_transform.c \
> > nir/nir_lower_wpos_center.c \
> > nir/nir_lower_wpos_ytransform.c \
> > nir/nir_metadata.c \
> > diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
> > index 4e5039e28e0..d6e7b2cc167 100644
> > --- a/src/compiler/nir/meson.build
> > +++ b/src/compiler/nir/meson.build
> > @@ -152,6 +152,7 @@ files_libnir = files(
> > 'nir_lower_vars_to_ssa.c',
> > 'nir_lower_var_copies.c',
> > 'nir_lower_vec_to_movs.c',
> > + 'nir_lower_viewport_transform.c',
> > 'nir_lower_wpos_center.c',
> > 'nir_lower_wpos_ytransform.c',
> > 'nir_lower_bit_size.c',
> > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> > index ad72a5c9222..4323f5e0413 100644
> > --- a/src/compiler/nir/nir.h
> > +++ b/src/compiler/nir/nir.h
> > @@ -3114,6 +3114,7 @@ void nir_lower_io_to_scalar(nir_shader *shader,
> > nir_variable_mode mask);
> > void nir_lower_io_to_scalar_early(nir_shader *shader, nir_variable_mode
> > mask);
> > bool nir_lower_io_to_vector(nir_shader *shader, nir_variable_mode mask);
> >
> > +void nir_lower_viewport_transform(nir_shader *shader);
> > bool nir_lower_uniforms_to_ubo(nir_shader *shader, int multiplier);
> >
> > typedef struct nir_lower_subgroups_options {
> > diff --git a/src/compiler/nir/nir_lower_viewport_transform.c
> > b/src/compiler/nir/nir_lower_viewport_transform.c
> > new file mode 100644
> > index 00000000000..94b54524ab7
> > --- /dev/null
> > +++ b/src/compiler/nir/nir_lower_viewport_transform.c
> > @@ -0,0 +1,102 @@
> > +/*
> > + * Copyright (C) 2019 Alyssa Rosenzweig
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +/* On some hardware (particularly, all current versions of Mali GPUs),
> > + * vertex shaders do not output gl_Position in world-space. Instead, they
> > + * output gl_Position in transformed screen space via the "pseudo"
> > + * position varying. Thus, this pass finds writes to gl_Position and
> > + * changes them to transformed writes, still to gl_Position. The
> > + * outputted screen space is still written back to VARYING_SLOT_POS,
> > + * which is semantically ambiguous but nevertheless a good match for
> > + * Gallium/NIR/Mali.
> > + *
> > + * Implements coordinate transformation as defined in section 12.5
> > + * "Coordinate Transformation" of the OpenGL ES 3.2 full specification.
> > + *
> > + * This pass must run before lower_vars/lower_io such that derefs are
> > + * still in place.
> > + */
> > +
> > +#include "nir/nir.h"
> > +#include "nir/nir_builder.h"
> > +
> > +void
> > +nir_lower_viewport_transform(nir_shader *shader)
> > +{
> > + assert(shader->info.stage == MESA_SHADER_VERTEX);
> > +
> > + nir_foreach_function(func, shader) {
> > + nir_foreach_block(block, func->impl) {
> > + nir_foreach_instr_safe(instr, block) {
> > + if (instr->type != nir_instr_type_intrinsic)
> > + continue;
> > +
>
> All other nir lowering passes are easier to read then this one.
> Maybe splitting this big function into multiple smaller ones
> to help readability?
tbh this pass is simple enough that I'm not really sure it matters..
Some of the other passes have a lot of boilerplate that you describe
below. I guess there might be some room for a helper to remove some
boilerplate.. until then I'm not really sure it is an improvement for
a pass like this
and I guess that counts as my r-b ;-)
BR,
-R
>
> static bool
> lower_frexp_impl(nir_function_impl *impl)
> {
> ...
> }
>
> bool
> lower_viewport_transform_impl(nir_shader *shader)
> {
> bool progress = false;
>
> nir_foreach_function(function, shader) {
> if (function->impl)
> progress |= lower_viewport_transform_impl(function->impl);
> }
>
> return progress;
> }
>
>
> > + nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
> > + if (intr->intrinsic != nir_intrinsic_store_deref)
> > + continue;
> > +
> > + nir_variable *var = nir_intrinsic_get_var(intr, 0);
> > + if (var->data.location != VARYING_SLOT_POS)
> > + continue;
> > +
> > + nir_builder b;
> > + nir_builder_init(&b, func->impl);
> > + b.cursor = nir_before_instr(instr);
> > +
> > + /* Grab the source and viewport */
> > + nir_ssa_def *input_point = nir_ssa_for_src(&b, intr->src[1],
> > 4);
> > + nir_ssa_def *scale = nir_load_viewport_scale(&b);
> > + nir_ssa_def *offset = nir_load_viewport_offset(&b);
> > +
> > + /* World space to normalised device coordinates to screen
> > space */
> > +
> > + nir_ssa_def *w_recip = nir_frcp(&b, nir_channel(&b,
> > input_point, 3));
> > +
> > + nir_ssa_def *ndc_point = nir_fmul(&b,
> > + nir_channels(&b, input_point, 0x7), w_recip);
> > +
> > + nir_ssa_def *screen = nir_fadd(&b,
> > + nir_fmul(&b, ndc_point, scale), offset);
> > +
> > + /* gl_Position will be written out in screenspace xyz, with w
> > set to
> > + * the reciprocal we computed earlier. The transformed w
> > component is
> > + * then used for perspective-correct varying interpolation. The
> > + * transformed w component must preserve its original sign;
> > this is
> > + * used in depth clipping computations
> > + */
> > +
> > + nir_ssa_def *screen_space = nir_vec4(&b,
> > + nir_channel(&b, screen, 0),
> > + nir_channel(&b, screen, 1),
> > + nir_channel(&b, screen, 2),
> > + w_recip);
> > +
> > + nir_instr_rewrite_src(instr, &intr->src[1],
> > + nir_src_for_ssa(screen_space));
> > + }
> > + }
> > +
> > + nir_metadata_preserve(func->impl, nir_metadata_block_index |
> > + nir_metadata_dominance);
> > + }
> > +}
> > --
> > 2.20.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> --
> greets
> --
> Christian Gmeiner, MSc
>
> https://christian-gmeiner.info
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
