On Thu, 2015-10-01 at 07:58 -0700, Jason Ekstrand wrote: > On Thu, Oct 1, 2015 at 7:52 AM, Iago Toral <ito...@igalia.com> wrote: > > On Wed, 2015-09-30 at 18:41 -0700, Jason Ekstrand wrote: > >> Previously, we had a bunch of code in each stage to figure out how many > >> slots we needed in stage_prog_data.param. This code was mostly identical > >> across the stages and had been copied and pasted around. Unfortunately, > >> this meant that any time you did something special, you had to add code for > >> it to each of these places. In particular, none of the stages took > >> subroutines into account; they were working entirely by accident. By > >> taking this data from the NIR shader, we know the exact number of entries > >> we need and everything goes a bit smoother. > >> --- > >> src/mesa/drivers/dri/i965/brw_cs.c | 4 ++-- > >> src/mesa/drivers/dri/i965/brw_gs.c | 5 ++--- > >> src/mesa/drivers/dri/i965/brw_vs.c | 16 ++++------------ > >> src/mesa/drivers/dri/i965/brw_wm.c | 10 +++------- > >> 4 files changed, 11 insertions(+), 24 deletions(-) > >> > >> diff --git a/src/mesa/drivers/dri/i965/brw_cs.c > >> b/src/mesa/drivers/dri/i965/brw_cs.c > >> index 02eeeda..24120fb 100644 > >> --- a/src/mesa/drivers/dri/i965/brw_cs.c > >> +++ b/src/mesa/drivers/dri/i965/brw_cs.c > >> @@ -30,6 +30,7 @@ > >> #include "intel_mipmap_tree.h" > >> #include "brw_state.h" > >> #include "intel_batchbuffer.h" > >> +#include "glsl/nir/nir.h" > >> > >> static bool > >> brw_codegen_cs_prog(struct brw_context *brw, > >> @@ -55,8 +56,7 @@ brw_codegen_cs_prog(struct brw_context *brw, > >> * prog_data associated with the compiled program, and which will be > >> freed > >> * by the state cache. > >> */ > >> - int param_count = cs->base.num_uniform_components + > >> - cs->base.NumImages * BRW_IMAGE_PARAM_SIZE; > >> + int param_count = cp->program.Base.nir->num_uniforms; > >> > >> /* The backend also sometimes adds params for texture size. */ > >> param_count += 2 * > >> ctx->Const.Program[MESA_SHADER_COMPUTE].MaxTextureImageUnits; > >> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c > >> b/src/mesa/drivers/dri/i965/brw_gs.c > >> index 61e7b2a..0cf7ec8 100644 > >> --- a/src/mesa/drivers/dri/i965/brw_gs.c > >> +++ b/src/mesa/drivers/dri/i965/brw_gs.c > >> @@ -32,6 +32,7 @@ > >> #include "brw_vec4_gs_visitor.h" > >> #include "brw_state.h" > >> #include "brw_ff_gs.h" > >> +#include "glsl/nir/nir.h" > >> > >> > >> bool > >> @@ -60,9 +61,7 @@ brw_codegen_gs_prog(struct brw_context *brw, > >> * every uniform is a float which gets padded to the size of a vec4. > >> */ > >> struct gl_shader *gs = prog->_LinkedShaders[MESA_SHADER_GEOMETRY]; > >> - int param_count = gs->num_uniform_components * 4; > >> - > >> - param_count += gs->NumImages * BRW_IMAGE_PARAM_SIZE; > >> + int param_count = gp->program.Base.nir->num_uniforms * 4; > > > > I think the vec4 nir backend does not handle image uniforms at the > > moment, does it? At least I see that the FS backend has code > > specifically for that in fs_visitor::nir_setup_uniform. Not sure if we > > support images in geometry stages though, but the code you remove seems > > to account for that... > > We don't. When Curro initially implemented it, he did have vec4 > support. However, that was dropped in favor of doing things > differently in the fs compiler. It may yet come back though. This > code was probably merged a bit speculatively but doesn't really hurt > anything.
Yeah, that's what I recall too, I was surprised to find image stuff here and thought that maybe I missed something. > >> c.prog_data.base.base.param = > >> rzalloc_array(NULL, const gl_constant_value *, param_count); > >> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c > >> b/src/mesa/drivers/dri/i965/brw_vs.c > >> index e1a0d9c..391411c 100644 > >> --- a/src/mesa/drivers/dri/i965/brw_vs.c > >> +++ b/src/mesa/drivers/dri/i965/brw_vs.c > >> @@ -109,18 +109,10 @@ brw_codegen_vs_prog(struct brw_context *brw, > >> * prog_data associated with the compiled program, and which will be > >> freed > >> * by the state cache. > >> */ > >> - int param_count; > >> - if (vs) { > >> - /* We add padding around uniform values below vec4 size, with the > >> worst > >> - * case being a float value that gets blown up to a vec4, so be > >> - * conservative here. > >> - */ > >> - param_count = vs->base.num_uniform_components * 4 + > >> - vs->base.NumImages * BRW_IMAGE_PARAM_SIZE; > >> - stage_prog_data->nr_image_params = vs->base.NumImages; > >> - } else { > >> - param_count = vp->program.Base.Parameters->NumParameters * 4; > >> - } > >> + int param_count = vp->program.Base.nir->num_uniforms; > >> + if (!brw->intelScreen->compiler->scalar_vs) > >> + param_count *= 4; > > > > Same thing here. > > > > Also, I guess this also means that for the scalar_vs cases we were > > computing a larger param_count than we really should, right? > > They were, but it had nothing to do with images. It only had to do > with the fact that we were multiplying num_uniform_components by 4 > which means that if your uniforms were all a bunch of floats, it would > be 4x the size needed. Yes, I was talking about that too. Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> > >> /* vec4_visitor::setup_uniform_clipplane_values() also uploads user > >> clip > >> * planes as uniforms. > >> */ > >> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c > >> b/src/mesa/drivers/dri/i965/brw_wm.c > >> index cc97d6a..08f2416 100644 > >> --- a/src/mesa/drivers/dri/i965/brw_wm.c > >> +++ b/src/mesa/drivers/dri/i965/brw_wm.c > >> @@ -35,6 +35,7 @@ > >> #include "program/prog_parameter.h" > >> #include "program/program.h" > >> #include "intel_mipmap_tree.h" > >> +#include "glsl/nir/nir.h" > >> > >> #include "util/ralloc.h" > >> > >> @@ -173,14 +174,9 @@ brw_codegen_wm_prog(struct brw_context *brw, > >> * prog_data associated with the compiled program, and which will be > >> freed > >> * by the state cache. > >> */ > >> - int param_count; > >> - if (fs) { > >> - param_count = fs->base.num_uniform_components + > >> - fs->base.NumImages * BRW_IMAGE_PARAM_SIZE; > >> + int param_count = fp->program.Base.nir->num_uniforms; > >> + if (fs) > >> prog_data.base.nr_image_params = fs->base.NumImages; > >> - } else { > >> - param_count = fp->program.Base.Parameters->NumParameters * 4; > >> - } > >> /* The backend also sometimes adds params for texture size. */ > >> param_count += 2 * > >> ctx->Const.Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits; > >> prog_data.base.param = > > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev