On Mon, Oct 19, 2015 at 12:07 AM, Pohjolainen, Topi
<topi.pohjolai...@intel.com> wrote:
> On Fri, Oct 16, 2015 at 08:24:11AM -0700, Jason Ekstrand wrote:
>> On Fri, Oct 16, 2015 at 12:35 AM, Pohjolainen, Topi
>> <topi.pohjolai...@intel.com> wrote:
>> > On Fri, Oct 09, 2015 at 05:50:22AM -0700, Jason Ekstrand wrote:
>> >> On Fri, Oct 9, 2015 at 12:10 AM, Pohjolainen, Topi
>> >> <topi.pohjolai...@intel.com> wrote:
>> >> > On Thu, Oct 08, 2015 at 05:22:47PM -0700, Jason Ekstrand wrote:
>> >> >> This commit moves the common/modern stuff. Some legacy stuff such as
>> >> >> setting use_alt_mode was left because it needs to know whether or not
>> >> >> we're
>> >> >> an ARB program.
>> >> >> ---
>> >> >> src/mesa/drivers/dri/i965/brw_fs.cpp | 98
>> >> >> ++++++++++++++++++++++++++++++++++++
>> >> >> src/mesa/drivers/dri/i965/brw_wm.c | 98
>> >> >> ------------------------------------
>> >> >> 2 files changed, 98 insertions(+), 98 deletions(-)
>> >> >>
>> >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> >> index 146f4b4..0e39b50 100644
>> >> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> >> @@ -5114,6 +5114,90 @@ fs_visitor::run_cs()
>> >> >> return !failed;
>> >> >> }
>> >> >>
>> >> >> +/**
>> >> >> + * Return a bitfield where bit n is set if barycentric interpolation
>> >> >> mode n
>> >> >> + * (see enum brw_wm_barycentric_interp_mode) is needed by the
>> >> >> fragment shader.
>> >> >> + */
>> >> >> +static unsigned
>> >> >> +brw_compute_barycentric_interp_modes(const struct brw_device_info
>> >> >> *devinfo,
>> >> >> + bool shade_model_flat,
>> >> >> + bool persample_shading,
>> >> >> + const nir_shader *shader)
>> >> >> +{
>> >> >> + unsigned barycentric_interp_modes = 0;
>> >> >> +
>> >> >> + nir_foreach_variable(var, &shader->inputs) {
>> >> >> + enum glsl_interp_qualifier interp_qualifier =
>> >> >> + (enum glsl_interp_qualifier)var->data.interpolation;
>> >> >> + bool is_centroid = var->data.centroid && !persample_shading;
>> >> >> + bool is_sample = var->data.sample || persample_shading;
>> >> >> + bool is_gl_Color = (var->data.location == VARYING_SLOT_COL0) ||
>> >> >> + (var->data.location == VARYING_SLOT_COL1);
>> >> >> +
>> >> >> + /* Ignore WPOS and FACE, because they don't require
>> >> >> interpolation. */
>> >> >> + if (var->data.location == VARYING_SLOT_POS ||
>> >> >> + var->data.location == VARYING_SLOT_FACE)
>> >> >> + continue;
>> >> >> +
>> >> >> + /* Determine the set (or sets) of barycentric coordinates
>> >> >> needed to
>> >> >> + * interpolate this variable. Note that when
>> >> >> + * brw->needs_unlit_centroid_workaround is set, centroid
>> >> >> interpolation
>> >> >> + * uses PIXEL interpolation for unlit pixels and CENTROID
>> >> >> interpolation
>> >> >> + * for lit pixels, so we need both sets of barycentric
>> >> >> coordinates.
>> >> >> + */
>> >> >> + if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) {
>> >> >> + if (is_centroid) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC;
>> >> >> + } else if (is_sample) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_NONPERSPECTIVE_SAMPLE_BARYCENTRIC;
>> >> >> + }
>> >> >> + if ((!is_centroid && !is_sample) ||
>> >> >> + devinfo->needs_unlit_centroid_workaround) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC;
>> >> >> + }
>> >> >> + } else if (interp_qualifier == INTERP_QUALIFIER_SMOOTH ||
>> >> >> + (!(shade_model_flat && is_gl_Color) &&
>> >> >> + interp_qualifier == INTERP_QUALIFIER_NONE)) {
>> >> >> + if (is_centroid) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC;
>> >> >> + } else if (is_sample) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_PERSPECTIVE_SAMPLE_BARYCENTRIC;
>> >> >> + }
>> >> >> + if ((!is_centroid && !is_sample) ||
>> >> >> + devinfo->needs_unlit_centroid_workaround) {
>> >> >> + barycentric_interp_modes |=
>> >> >> + 1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;
>> >> >> + }
>> >> >> + }
>> >> >> + }
>> >> >> +
>> >> >> + return barycentric_interp_modes;
>> >> >> +}
>> >> >> +
>> >> >> +static uint8_t
>> >> >> +computed_depth_mode(const nir_shader *shader)
>> >> >> +{
>> >> >> + if (shader->info.outputs_written &
>> >> >> BITFIELD64_BIT(FRAG_RESULT_DEPTH)) {
>> >> >> + switch (shader->info.fs.depth_layout) {
>> >> >> + case FRAG_DEPTH_LAYOUT_NONE:
>> >> >> + case FRAG_DEPTH_LAYOUT_ANY:
>> >> >> + return BRW_PSCDEPTH_ON;
>> >> >> + case FRAG_DEPTH_LAYOUT_GREATER:
>> >> >> + return BRW_PSCDEPTH_ON_GE;
>> >> >> + case FRAG_DEPTH_LAYOUT_LESS:
>> >> >> + return BRW_PSCDEPTH_ON_LE;
>> >> >> + case FRAG_DEPTH_LAYOUT_UNCHANGED:
>> >> >> + return BRW_PSCDEPTH_OFF;
>> >> >> + }
>> >> >> + }
>> >> >> + return BRW_PSCDEPTH_OFF;
>> >> >> +}
>> >> >> +
>> >> >> const unsigned *
>> >> >> brw_wm_fs_emit(const struct brw_compiler *compiler, void *log_data,
>> >> >> void *mem_ctx,
>> >> >> @@ -5126,6 +5210,20 @@ brw_wm_fs_emit(const struct brw_compiler
>> >> >> *compiler, void *log_data,
>> >> >> unsigned *final_assembly_size,
>> >> >> char **error_str)
>> >> >> {
>> >> >> + /* key->alpha_test_func means simulating alpha testing via
>> >> >> discards,
>> >> >> + * so the shader definitely kills pixels.
>> >> >> + */
>> >> >> + prog_data->uses_kill = shader->info.fs.uses_discard ||
>> >> >> key->alpha_test_func;
>> >> >> + prog_data->uses_omask =
>> >> >> + shader->info.outputs_written &
>> >> >> BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);
>> >> >> + prog_data->computed_depth_mode = computed_depth_mode(shader);
>> >> >> +
>> >> >> + prog_data->barycentric_interp_modes =
>> >> >> + brw_compute_barycentric_interp_modes(compiler->devinfo,
>> >> >> + key->flat_shade,
>> >> >> + key->persample_shading,
>> >> >> + shader);
>> >> >> +
>> >> >> fs_visitor v(compiler, log_data, mem_ctx, key,
>> >> >> &prog_data->base, prog, shader, 8,
>> >> >> shader_time_index8);
>> >> >> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
>> >> >> b/src/mesa/drivers/dri/i965/brw_wm.c
>> >> >> index ab9461a..91bda35 100644
>> >> >> --- a/src/mesa/drivers/dri/i965/brw_wm.c
>> >> >> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
>> >> >> @@ -39,89 +39,6 @@
>> >> >>
>> >> >> #include "util/ralloc.h"
>> >> >>
>> >> >> -/**
>> >> >> - * Return a bitfield where bit n is set if barycentric interpolation
>> >> >> mode n
>> >> >> - * (see enum brw_wm_barycentric_interp_mode) is needed by the
>> >> >> fragment shader.
>> >> >> - */
>> >> >> -static unsigned
>> >> >> -brw_compute_barycentric_interp_modes(const struct brw_device_info
>> >> >> *devinfo,
>> >> >> - bool shade_model_flat,
>> >> >> - bool persample_shading,
>> >> >> - nir_shader *shader)
>> >> >> -{
>> >> >> - unsigned barycentric_interp_modes = 0;
>> >> >> -
>> >> >> - nir_foreach_variable(var, &shader->inputs) {
>> >> >> - enum glsl_interp_qualifier interp_qualifier =
>> >> >> var->data.interpolation;
>> >> >> - bool is_centroid = var->data.centroid && !persample_shading;
>> >> >> - bool is_sample = var->data.sample || persample_shading;
>> >> >> - bool is_gl_Color = (var->data.location == VARYING_SLOT_COL0) ||
>> >> >> - (var->data.location == VARYING_SLOT_COL1);
>> >> >> -
>> >> >> - /* Ignore WPOS and FACE, because they don't require
>> >> >> interpolation. */
>> >> >> - if (var->data.location == VARYING_SLOT_POS ||
>> >> >> - var->data.location == VARYING_SLOT_FACE)
>> >> >> - continue;
>> >> >> -
>> >> >> - /* Determine the set (or sets) of barycentric coordinates
>> >> >> needed to
>> >> >> - * interpolate this variable. Note that when
>> >> >> - * brw->needs_unlit_centroid_workaround is set, centroid
>> >> >> interpolation
>> >> >> - * uses PIXEL interpolation for unlit pixels and CENTROID
>> >> >> interpolation
>> >> >> - * for lit pixels, so we need both sets of barycentric
>> >> >> coordinates.
>> >> >> - */
>> >> >> - if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) {
>> >> >> - if (is_centroid) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC;
>> >> >> - } else if (is_sample) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_NONPERSPECTIVE_SAMPLE_BARYCENTRIC;
>> >> >> - }
>> >> >> - if ((!is_centroid && !is_sample) ||
>> >> >> - devinfo->needs_unlit_centroid_workaround) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC;
>> >> >> - }
>> >> >> - } else if (interp_qualifier == INTERP_QUALIFIER_SMOOTH ||
>> >> >> - (!(shade_model_flat && is_gl_Color) &&
>> >> >> - interp_qualifier == INTERP_QUALIFIER_NONE)) {
>> >> >> - if (is_centroid) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC;
>> >> >> - } else if (is_sample) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_PERSPECTIVE_SAMPLE_BARYCENTRIC;
>> >> >> - }
>> >> >> - if ((!is_centroid && !is_sample) ||
>> >> >> - devinfo->needs_unlit_centroid_workaround) {
>> >> >> - barycentric_interp_modes |=
>> >> >> - 1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;
>> >> >> - }
>> >> >> - }
>> >> >> - }
>> >> >> -
>> >> >> - return barycentric_interp_modes;
>> >> >> -}
>> >> >> -
>> >> >> -static uint8_t
>> >> >> -computed_depth_mode(struct gl_fragment_program *fp)
>> >> >> -{
>> >> >> - if (fp->Base.OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_DEPTH)) {
>> >> >> - switch (fp->FragDepthLayout) {
>> >> >> - case FRAG_DEPTH_LAYOUT_NONE:
>> >> >> - case FRAG_DEPTH_LAYOUT_ANY:
>> >> >> - return BRW_PSCDEPTH_ON;
>> >> >> - case FRAG_DEPTH_LAYOUT_GREATER:
>> >> >> - return BRW_PSCDEPTH_ON_GE;
>> >> >> - case FRAG_DEPTH_LAYOUT_LESS:
>> >> >> - return BRW_PSCDEPTH_ON_LE;
>> >> >> - case FRAG_DEPTH_LAYOUT_UNCHANGED:
>> >> >> - return BRW_PSCDEPTH_OFF;
>> >> >> - }
>> >> >> - }
>> >> >> - return BRW_PSCDEPTH_OFF;
>> >> >> -}
>> >> >> -
>> >> >> static void
>> >> >> assign_fs_binding_table_offsets(const struct brw_device_info *devinfo,
>> >> >> const struct gl_shader_program
>> >> >> *shader_prog,
>> >> >> @@ -166,15 +83,6 @@ brw_codegen_wm_prog(struct brw_context *brw,
>> >> >> fs = (struct brw_shader
>> >> >> *)prog->_LinkedShaders[MESA_SHADER_FRAGMENT];
>> >> >>
>> >> >> memset(&prog_data, 0, sizeof(prog_data));
>> >> >> - /* key->alpha_test_func means simulating alpha testing via
>> >> >> discards,
>> >> >> - * so the shader definitely kills pixels.
>> >> >> - */
>> >> >> - prog_data.uses_kill = fp->program.UsesKill || key->alpha_test_func;
>> >> >> - prog_data.uses_omask =
>> >> >> - fp->program.Base.OutputsWritten &
>> >> >> BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);
>> >> >> - prog_data.computed_depth_mode = computed_depth_mode(&fp->program);
>> >> >> -
>> >> >> - prog_data.early_fragment_tests = fs && fs->base.EarlyFragmentTests;
>> >> >
>> >> > You store "early_fragment_tests" in patch 6 into shader info but this
>> >> > patch
>> >> > doesn't add logic to read that and set it for prog_data (I was
>> >> > expecting to
>> >> > see that in brw_wm_fs_emit()). To me it seems that the corresponding
>> >> > field
>> >> > in prog_data is now always left to false.
>> >>
>> >> Good catch! You're absolutely correct. Some how that got dropped in
>> >> my copy-and-pasting. I've added
>> >>
>> >> + prog_data->early_fragment_tests =
>> >> shader->info.fs.early_fragment_tests;
>> >>
>> >> to the hunk above where I set the other prog_data stuff.
>> >
>> > Sounds right, this patch and the remaining 6, 11, 13, 14 and 16 are also:
>> >
>> > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>> >
>> > (There is no patch number 17 in the series, is there?)
>>
>> There was. It just failed to send for whatever reason. You can see it here:
>>
>> http://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=review/i965-compiler-cleanups2.2&id=f245a68ff94c0612013c3babe52c7d7b77405993
>>
>> It's kind of the cap-stone of the whole series so it's a bummer I
>> couldn't get it to send.
>
> That patch looks fine to me except the change in "i965/Makefile.sources"
> looks to have spaces instead of tab. If you double check that, it is
You were correct. Good catch. I've got that fixed now.
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
Thanks!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
