On Mon, 2015-11-16 at 07:55 -0800, Jason Ekstrand wrote:
> On Mon, Nov 16, 2015 at 6:27 AM, Iago Toral <ito...@igalia.com> wrote:
> > On Mon, 2015-11-16 at 11:33 +0100, Iago Toral wrote:
> >> On Wed, 2015-11-11 at 17:26 -0800, Jason Ekstrand wrote:
> >> > Previously, we had a rescale_texcoords helper in the FS backend for
> >> > handling rescaling of texture coordinates. Now that we can do variants
> >> > in
> >> > NIR, we can use nir_lower_tex to do the rescaling for us. This allows us
> >> > to delete the i965-specific code and gives us proper TEXTURE_RECTANGLE
> >> > and
> >> > GL_CLAMP handling in vertex and geometry shaders.
> >> > ---
> >> > src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +
> >> > src/mesa/drivers/dri/i965/brw_fs.h | 3 -
> >> > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 4 +-
> >> > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 125
> >> > ----------------------
> >> > src/mesa/drivers/dri/i965/brw_nir.c | 23 ++++
> >> > src/mesa/drivers/dri/i965/brw_nir.h | 6 ++
> >> > src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 +
> >> > src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 2 +
> >> > 8 files changed, 36 insertions(+), 131 deletions(-)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > index b8713ab..c56cafe 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > @@ -5468,6 +5468,7 @@ brw_compile_fs(const struct brw_compiler
> >> > *compiler, void *log_data,
> >> > char **error_str)
> >> > {
> >> > nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> >> > + brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
> >> > true);
> >>
> >> This looks like it is part of the post-processing process. In fact, you
> >> call this right before brw_postprocess_nir() for every stage. Why not
> >> just add a key parameter to brw_postprocess_nir() and call
> >> brw_nir_apply_sampler_key from there instead?
>
> Well, right now, brw_nir_apply_sampler_key is used for all stages.
> However, if we do more variant stuff in NIR, we'll need to have
> brw_nir_apply_vs_key, brw_nir_apply_fs_key, etc. and we can't put it
> in postprocess_nir. I didn't want to join it prematurely.
Oh right, that makes sense.
> >> Either way,
> >> Reviewed-by: Iago Toral Quiroga <ito...@igalia.com>
>
> Thanks!
>
> >> > brw_postprocess_nir(shader, compiler->devinfo, true);
> >> >
> >> > /* key->alpha_test_func means simulating alpha testing via discards,
> >> > @@ -5628,6 +5629,7 @@ brw_compile_cs(const struct brw_compiler
> >> > *compiler, void *log_data,
> >> > char **error_str)
> >> > {
> >> > nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> >> > + brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
> >> > true);
> >> > brw_postprocess_nir(shader, compiler->devinfo, true);
> >> >
> >> > prog_data->local_size[0] = shader->info.cs.local_size[0];
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> >> > b/src/mesa/drivers/dri/i965/brw_fs.h
> >> > index 2dfcab1..8a181d7 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs.h
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> >> > @@ -217,8 +217,6 @@ public:
> >> > void emit_interpolation_setup_gen4();
> >> > void emit_interpolation_setup_gen6();
> >> > void compute_sample_position(fs_reg dst, fs_reg int_sample_pos);
> >> > - fs_reg rescale_texcoord(fs_reg coordinate, int coord_components,
> >> > - bool is_rect, uint32_t sampler);
> >> > void emit_texture(ir_texture_opcode op,
> >> > const glsl_type *dest_type,
> >> > fs_reg coordinate, int components,
> >> > @@ -229,7 +227,6 @@ public:
> >> > fs_reg mcs,
> >> > int gather_component,
> >> > bool is_cube_array,
> >> > - bool is_rect,
> >> > uint32_t sampler,
> >> > fs_reg sampler_reg);
> >> > fs_reg emit_mcs_fetch(const fs_reg &coordinate, unsigned components,
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > index 02b9f5b..3d83d7c 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > @@ -2411,8 +2411,6 @@ fs_visitor::nir_emit_texture(const fs_builder
> >> > &bld, nir_tex_instr *instr)
> >> >
> >> > int gather_component = instr->component;
> >> >
> >> > - bool is_rect = instr->sampler_dim == GLSL_SAMPLER_DIM_RECT;
> >> > -
> >> > bool is_cube_array = instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE &&
> >> > instr->is_array;
> >> >
> >> > @@ -2549,7 +2547,7 @@ fs_visitor::nir_emit_texture(const fs_builder
> >> > &bld, nir_tex_instr *instr)
> >> > emit_texture(op, dest_type, coordinate, instr->coord_components,
> >> > shadow_comparitor, lod, lod2, lod_components,
> >> > sample_index,
> >> > tex_offset, mcs, gather_component,
> >> > - is_cube_array, is_rect, sampler, sampler_reg);
> >> > + is_cube_array, sampler, sampler_reg);
> >> >
> >> > fs_reg dest = get_nir_dest(instr->dest);
> >> > dest.type = this->result.type;
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> > index 213c912..faf304c 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> > @@ -79,122 +79,6 @@ fs_visitor::emit_vs_system_value(int location)
> >> > return reg;
> >> > }
> >> >
> >> > -fs_reg
> >> > -fs_visitor::rescale_texcoord(fs_reg coordinate, int coord_components,
> >> > - bool is_rect, uint32_t sampler)
> >> > -{
> >> > - bool needs_gl_clamp = true;
> >> > - fs_reg scale_x, scale_y;
> >> > -
> >> > - /* The 965 requires the EU to do the normalization of GL rectangle
> >> > - * texture coordinates. We use the program parameter state
> >> > - * tracking to get the scaling factor.
> >> > - */
> >> > - if (is_rect &&
> >> > - (devinfo->gen < 6 ||
> >> > - (devinfo->gen >= 6 && (key_tex->gl_clamp_mask[0] & (1 <<
> >> > sampler) ||
> >> > - key_tex->gl_clamp_mask[1] & (1 <<
> >> > sampler))))) {
> >> > - struct gl_program_parameter_list *params = prog->Parameters;
> >> > -
> >> > -
> >> > - /* FINISHME: We're failing to recompile our programs when the
> >> > sampler is
> >> > - * updated. This only matters for the texture rectangle scale
> >> > - * parameters (pre-gen6, or gen6+ with GL_CLAMP).
> >> > - */
> >> > - int tokens[STATE_LENGTH] = {
> >> > - STATE_INTERNAL,
> >> > - STATE_TEXRECT_SCALE,
> >> > - prog->SamplerUnits[sampler],
> >> > - 0,
> >> > - 0
> >> > - };
> >> > -
> >> > - no16("rectangle scale uniform setup not supported on SIMD16\n");
> >> > - if (dispatch_width == 16) {
> >> > - return coordinate;
> >> > - }
> >> > -
> >> > - GLuint index = _mesa_add_state_reference(params,
> >> > - (gl_state_index *)tokens);
> >> > - /* Try to find existing copies of the texrect scale uniforms. */
> >> > - for (unsigned i = 0; i < uniforms; i++) {
> >> > - if (stage_prog_data->param[i] ==
> >> > - &prog->Parameters->ParameterValues[index][0]) {
> >> > - scale_x = fs_reg(UNIFORM, i);
> >> > - scale_y = fs_reg(UNIFORM, i + 1);
> >> > - break;
> >> > - }
> >> > - }
> >> > -
> >> > - /* If we didn't already set them up, do so now. */
> >> > - if (scale_x.file == BAD_FILE) {
> >> > - scale_x = fs_reg(UNIFORM, uniforms);
> >> > - scale_y = fs_reg(UNIFORM, uniforms + 1);
> >> > -
> >> > - stage_prog_data->param[uniforms++] =
> >> > - &prog->Parameters->ParameterValues[index][0];
> >> > - stage_prog_data->param[uniforms++] =
> >> > - &prog->Parameters->ParameterValues[index][1];
> >> > - }
> >> > - }
> >> > -
> >> > - /* The 965 requires the EU to do the normalization of GL rectangle
> >> > - * texture coordinates. We use the program parameter state
> >> > - * tracking to get the scaling factor.
> >> > - */
> >> > - if (devinfo->gen < 6 && is_rect) {
> >> > - fs_reg dst = fs_reg(GRF, alloc.allocate(coord_components));
> >> > - fs_reg src = coordinate;
> >> > - coordinate = dst;
> >> > -
> >> > - bld.MUL(dst, src, scale_x);
> >> > - dst = offset(dst, bld, 1);
> >> > - src = offset(src, bld, 1);
> >> > - bld.MUL(dst, src, scale_y);
> >> > - } else if (is_rect) {
> >> > - /* On gen6+, the sampler handles the rectangle coordinates
> >> > - * natively, without needing rescaling. But that means we have
> >> > - * to do GL_CLAMP clamping at the [0, width], [0, height] scale,
> >> > - * not [0, 1] like the default case below.
> >> > - */
> >> > - needs_gl_clamp = false;
> >> > -
> >> > - for (int i = 0; i < 2; i++) {
> >> > - if (key_tex->gl_clamp_mask[i] & (1 << sampler)) {
> >> > - fs_reg chan = coordinate;
> >> > - chan = offset(chan, bld, i);
> >> > -
> >> > - set_condmod(BRW_CONDITIONAL_GE,
> >> > - bld.emit(BRW_OPCODE_SEL, chan, chan,
> >> > fs_reg(0.0f)));
> >> > -
> >> > - /* Our parameter comes in as 1.0/width or 1.0/height,
> >> > - * because that's what people normally want for doing
> >> > - * texture rectangle handling. We need width or height
> >> > - * for clamping, but we don't care enough to make a new
> >> > - * parameter type, so just invert back.
> >> > - */
> >> > - fs_reg limit = vgrf(glsl_type::float_type);
> >> > - bld.MOV(limit, i == 0 ? scale_x : scale_y);
> >> > - bld.emit(SHADER_OPCODE_RCP, limit, limit);
> >> > -
> >> > - set_condmod(BRW_CONDITIONAL_L,
> >> > - bld.emit(BRW_OPCODE_SEL, chan, chan, limit));
> >> > - }
> >> > - }
> >> > - }
> >> > -
> >> > - if (coord_components > 0 && needs_gl_clamp) {
> >> > - for (int i = 0; i < MIN2(coord_components, 3); i++) {
> >> > - if (key_tex->gl_clamp_mask[i] & (1 << sampler)) {
> >> > - fs_reg chan = coordinate;
> >> > - chan = offset(chan, bld, i);
> >> > - set_saturate(true, bld.MOV(chan, chan));
> >> > - }
> >> > - }
> >> > - }
> >> > - return coordinate;
> >> > -}
> >> > -
> >> > /* Sample from the MCS surface attached to this multisample texture. */
> >> > fs_reg
> >> > fs_visitor::emit_mcs_fetch(const fs_reg &coordinate, unsigned
> >> > components,
> >> > @@ -227,7 +111,6 @@ fs_visitor::emit_texture(ir_texture_opcode op,
> >> > fs_reg mcs,
> >> > int gather_component,
> >> > bool is_cube_array,
> >> > - bool is_rect,
> >> > uint32_t sampler,
> >> > fs_reg sampler_reg)
> >> > {
> >> > @@ -259,14 +142,6 @@ fs_visitor::emit_texture(ir_texture_opcode op,
> >> > lod = fs_reg(0u);
> >> > }
> >> >
> >> > - if (coordinate.file != BAD_FILE) {
> >> > - /* FINISHME: Texture coordinate rescaling doesn't work with
> >> > non-constant
> >> > - * samplers. This should only be a problem with GL_CLAMP on Gen7.
> >> > - */
> >> > - coordinate = rescale_texcoord(coordinate, coord_components,
> >> > is_rect,
> >> > - sampler);
> >> > - }
> >> > -
> >> > /* Writemasking doesn't eliminate channels on SIMD8 texture
> >> > * samples, so don't worry about them.
> >> > */
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
> >> > b/src/mesa/drivers/dri/i965/brw_nir.c
> >> > index 693b9cd..3999a60 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_nir.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> >> > @@ -395,6 +395,29 @@ brw_create_nir(struct brw_context *brw,
> >> > return nir;
> >> > }
> >> >
> >> > +void
> >> > +brw_nir_apply_sampler_key(nir_shader *nir,
> >> > + const struct brw_device_info *devinfo,
> >> > + const struct brw_sampler_prog_key_data
> >> > *key_tex,
> >> > + bool is_scalar)
> >> > +{
> >> > + nir_lower_tex_options tex_options = { 0 };
> >> > +
> >> > + /* Iron Lake and prior require lowering of all rectangle textures */
> >> > + if (devinfo->gen < 6)
> >> > + tex_options.lower_rect = true;
> >> > +
> >> > + /* Prior to Broadwell, our hardware can't actually do GL_CLAMP */
> >> > + if (devinfo->gen < 8) {
> >> > + tex_options.saturate_s = key_tex->gl_clamp_mask[0];
> >> > + tex_options.saturate_t = key_tex->gl_clamp_mask[1];
> >> > + tex_options.saturate_r = key_tex->gl_clamp_mask[2];
> >> > + }
> >> > +
> >> > + if (nir_lower_tex(nir, &tex_options))
> >> > + nir_optimize(nir, is_scalar);
> >
> > Based on the code above I guess we only want to execute nir_lower_tex if
> > gen < 8 too.
>
> It's a no-op in that case and returns false so it's not a big deal.
> The second series uses it to do texture swizzle and that we have to do
> on all gens so I didn't see a good reason put it in an if. I can if
> you'd like. It would be a tad more efficient. Or, for that matter,
> we could make nir_lower_tex early-exit if it has nothing interesting
> to do.
>
> > Iago
> >
> >> > +}
> >> > +
> >> > enum brw_reg_type
> >> > brw_type_for_nir_type(nir_alu_type type)
> >> > {
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_nir.h
> >> > b/src/mesa/drivers/dri/i965/brw_nir.h
> >> > index 1cdfe00..a399992 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_nir.h
> >> > +++ b/src/mesa/drivers/dri/i965/brw_nir.h
> >> > @@ -90,6 +90,12 @@ void brw_postprocess_nir(nir_shader *nir,
> >> > const struct brw_device_info *devinfo,
> >> > bool is_scalar);
> >> >
> >> > +
> >> > +void brw_nir_apply_sampler_key(nir_shader *nir,
> >> > + const struct brw_device_info *devinfo,
> >> > + const struct brw_sampler_prog_key_data
> >> > *key,
> >> > + bool is_scalar);
> >> > +
> >> > enum brw_reg_type brw_type_for_nir_type(nir_alu_type type);
> >> >
> >> > enum glsl_base_type brw_glsl_base_type_for_nir_type(nir_alu_type type);
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> > index 9f75bb6..d207180 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> > @@ -2036,6 +2036,8 @@ brw_compile_vs(const struct brw_compiler
> >> > *compiler, void *log_data,
> >> > char **error_str)
> >> > {
> >> > nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> >> > + brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
> >> > + compiler->scalar_vs);
> >> > brw_postprocess_nir(shader, compiler->devinfo, compiler->scalar_vs);
> >> >
> >> > const unsigned *assembly = NULL;
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> >> > index 92b15d9..4caf54e 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> >> > @@ -616,6 +616,8 @@ brw_compile_gs(const struct brw_compiler *compiler,
> >> > void *log_data,
> >> > c.key = *key;
> >> >
> >> > nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> >> > + brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
> >> > + compiler->scalar_gs);
> >> > brw_postprocess_nir(shader, compiler->devinfo, compiler->scalar_gs);
> >> >
> >> > prog_data->include_primitive_id =
> >>
> >
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
