[Mesa-dev] [PATCH] i965: Store uniform constant values in a gl_constant_value instead of float
Neil Roberts
neil at linux.intel.com
Tue Aug 12 11:04:23 PDT 2014
Hi,
Here is a replacement patch for the one to use memcpy when copying
uniform values into the batch buffer here:
http://lists.freedesktop.org/archives/mesa-dev/2014-August/065179.html
Eric Anholt suggested instead of using memcpy we should change the
type of the pointer to uint32_t. I started to implement that idea but
I found there were quite a few bits of debugging code that are trying
to print the values as floats. I thought it might be cleaner if we
make the pointers gl_constant_value instead because that will avoid
having ugly casts in this debugging code and also makes it more
obvious that the values are being overloaded.
This patch also ends up fixing a few other potential problems in
places where we are copying floatings for example when copying into
the pull params.
I've run the patch through all of the Piglit tests on Ivybridge with a
32-bit build and it doesn't cause any changes except to fix the five
test cases mentioned in the bug report.
- Neil
------- >8 --------------- (use git am --scissors to automatically chop here)
The brw_stage_prog_data struct previously contained an array of float pointers
to the values of parameters. These were then copied into a batch buffer to
upload the values using a regular assignment. However the float values were
also being overloaded to store integer values for integer uniforms. This can
break if x87 floating-point registers are used to do the assignment because
the fst instruction tries to fix up invalid float values. If an integer
constant happened to look like an invalid float value then it would get
altered when it was copied into the batch buffer.
This patch changes the pointers to be gl_constant_value instead so that the
assignment should end up copying without any alteration. This also makes it
more obvious that the values being stored here are overloaded for multiple
types.
There are some static asserts where the values are uploaded to ensure that the
size of gl_constant_value is the same as a float.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81150
---
src/mesa/drivers/dri/i965/brw_context.h | 5 +++--
src/mesa/drivers/dri/i965/brw_curbe.c | 22 ++++++++++++----------
src/mesa/drivers/dri/i965/brw_fs.cpp | 6 +++---
src/mesa/drivers/dri/i965/brw_fs_fp.cpp | 2 +-
src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 6 +++---
src/mesa/drivers/dri/i965/brw_vec4.cpp | 6 +++---
src/mesa/drivers/dri/i965/brw_vec4_gs.c | 4 ++--
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 13 ++++++++-----
src/mesa/drivers/dri/i965/brw_vec4_vp.cpp | 4 ++--
src/mesa/drivers/dri/i965/brw_vs.c | 6 ++++--
src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 10 ++++++----
src/mesa/drivers/dri/i965/brw_wm.c | 5 +++--
src/mesa/drivers/dri/i965/gen6_vs_state.c | 8 +++++---
13 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
index 1bbcf46..157a605 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -41,6 +41,7 @@
#include "main/mtypes.h"
#include "brw_structs.h"
#include "intel_aub.h"
+#include "program/prog_parameter.h"
#ifdef __cplusplus
extern "C" {
@@ -309,8 +310,8 @@ struct brw_stage_prog_data {
* These must be the last fields of the struct (see
* brw_stage_prog_data_compare()).
*/
- const float **param;
- const float **pull_param;
+ const gl_constant_value **param;
+ const gl_constant_value **pull_param;
};
/* Data about a particular attempt to compile a program. Note that
diff --git a/src/mesa/drivers/dri/i965/brw_curbe.c b/src/mesa/drivers/dri/i965/brw_curbe.c
index 02eda5f..1a828ed 100644
--- a/src/mesa/drivers/dri/i965/brw_curbe.c
+++ b/src/mesa/drivers/dri/i965/brw_curbe.c
@@ -196,7 +196,7 @@ brw_upload_constant_buffer(struct brw_context *brw)
/* BRW_NEW_CURBE_OFFSETS */
const GLuint sz = brw->curbe.total_size;
const GLuint bufsz = sz * 16 * sizeof(GLfloat);
- GLfloat *buf;
+ gl_constant_value *buf;
GLuint i;
gl_clip_plane *clip_planes;
@@ -207,6 +207,8 @@ brw_upload_constant_buffer(struct brw_context *brw)
buf = intel_upload_space(brw, bufsz, 64,
&brw->curbe.curbe_bo, &brw->curbe.curbe_offset);
+ STATIC_ASSERT(sizeof(gl_constant_value) == sizeof(float));
+
/* fragment shader constants */
if (brw->curbe.wm_size) {
/* BRW_NEW_CURBE_OFFSETS */
@@ -226,10 +228,10 @@ brw_upload_constant_buffer(struct brw_context *brw)
/* If any planes are going this way, send them all this way:
*/
for (i = 0; i < 6; i++) {
- buf[offset + i * 4 + 0] = fixed_plane[i][0];
- buf[offset + i * 4 + 1] = fixed_plane[i][1];
- buf[offset + i * 4 + 2] = fixed_plane[i][2];
- buf[offset + i * 4 + 3] = fixed_plane[i][3];
+ buf[offset + i * 4 + 0].f = fixed_plane[i][0];
+ buf[offset + i * 4 + 1].f = fixed_plane[i][1];
+ buf[offset + i * 4 + 2].f = fixed_plane[i][2];
+ buf[offset + i * 4 + 3].f = fixed_plane[i][3];
}
/* Clip planes: _NEW_TRANSFORM plus _NEW_PROJECTION to get to
@@ -238,10 +240,10 @@ brw_upload_constant_buffer(struct brw_context *brw)
clip_planes = brw_select_clip_planes(ctx);
for (j = 0; j < MAX_CLIP_PLANES; j++) {
if (ctx->Transform.ClipPlanesEnabled & (1<<j)) {
- buf[offset + i * 4 + 0] = clip_planes[j][0];
- buf[offset + i * 4 + 1] = clip_planes[j][1];
- buf[offset + i * 4 + 2] = clip_planes[j][2];
- buf[offset + i * 4 + 3] = clip_planes[j][3];
+ buf[offset + i * 4 + 0].f = clip_planes[j][0];
+ buf[offset + i * 4 + 1].f = clip_planes[j][1];
+ buf[offset + i * 4 + 2].f = clip_planes[j][2];
+ buf[offset + i * 4 + 3].f = clip_planes[j][3];
i++;
}
}
@@ -260,7 +262,7 @@ brw_upload_constant_buffer(struct brw_context *brw)
if (0) {
for (i = 0; i < sz*16; i+=4)
fprintf(stderr, "curbe %d.%d: %f %f %f %f\n", i/8, i&4,
- buf[i+0], buf[i+1], buf[i+2], buf[i+3]);
+ buf[i+0].f, buf[i+1].f, buf[i+2].f, buf[i+3].f);
}
/* Because this provokes an action (ie copy the constants into the
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 3aee822..8573a3d 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -961,7 +961,7 @@ fs_visitor::setup_uniform_values(ir_variable *ir)
slots *= storage->array_elements;
for (unsigned i = 0; i < slots; i++) {
- stage_prog_data->param[uniforms++] = &storage->storage[i].f;
+ stage_prog_data->param[uniforms++] = &storage->storage[i];
}
}
@@ -1000,7 +1000,7 @@ fs_visitor::setup_builtin_uniform_values(ir_variable *ir)
last_swiz = swiz;
stage_prog_data->param[uniforms++] =
- &fp->Base.Parameters->ParameterValues[index][swiz].f;
+ &fp->Base.Parameters->ParameterValues[index][swiz];
}
}
}
@@ -1806,7 +1806,7 @@ fs_visitor::move_uniform_array_access_to_pull_constants()
* add it.
*/
if (pull_constant_loc[uniform] == -1) {
- const float **values = &stage_prog_data->param[uniform];
+ const gl_constant_value **values = &stage_prog_data->param[uniform];
assert(param_size[uniform]);
diff --git a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
index 8d07be2..98df299 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
@@ -583,7 +583,7 @@ fs_visitor::setup_fp_regs()
p < prog->Parameters->NumParameters; p++) {
for (unsigned int i = 0; i < 4; i++) {
stage_prog_data->param[uniforms++] =
- &prog->Parameters->ParameterValues[p][i].f;
+ &prog->Parameters->ParameterValues[p][i];
}
}
}
diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
index c16401b..2d5f18d 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
@@ -1639,7 +1639,7 @@ fs_visitor::rescale_texcoord(ir_texture *ir, fs_reg coordinate,
/* 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].f) {
+ &prog->Parameters->ParameterValues[index][0]) {
scale_x = fs_reg(UNIFORM, i);
scale_y = fs_reg(UNIFORM, i + 1);
break;
@@ -1652,9 +1652,9 @@ fs_visitor::rescale_texcoord(ir_texture *ir, fs_reg coordinate,
scale_y = fs_reg(UNIFORM, uniforms + 1);
stage_prog_data->param[uniforms++] =
- &prog->Parameters->ParameterValues[index][0].f;
+ &prog->Parameters->ParameterValues[index][0];
stage_prog_data->param[uniforms++] =
- &prog->Parameters->ParameterValues[index][1].f;
+ &prog->Parameters->ParameterValues[index][1];
}
}
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
index 9a73f8f..b9dfc37 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
@@ -667,7 +667,7 @@ vec4_visitor::move_push_constants_to_pull_constants()
pull_constant_loc[i / 4] = -1;
if (i >= max_uniform_components) {
- const float **values = &stage_prog_data->param[i];
+ const gl_constant_value **values = &stage_prog_data->param[i];
/* Try to find an existing copy of this uniform in the pull
* constants if it was part of an array access already.
@@ -1490,10 +1490,10 @@ vec4_visitor::setup_uniforms(int reg)
this->uniform_vector_size[this->uniforms] = 1;
stage_prog_data->param =
- reralloc(NULL, stage_prog_data->param, const float *, 4);
+ reralloc(NULL, stage_prog_data->param, const gl_constant_value *, 4);
for (unsigned int i = 0; i < 4; i++) {
unsigned int slot = this->uniforms * 4 + i;
- static float zero = 0.0;
+ static gl_constant_value zero = { .f = 0.0 };
stage_prog_data->param[slot] = &zero;
}
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
index 6428291..8daa555 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
+++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
@@ -65,9 +65,9 @@ do_gs_prog(struct brw_context *brw,
param_count += MAX_CLIP_PLANES * 4;
c.prog_data.base.base.param =
- rzalloc_array(NULL, const float *, param_count);
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
c.prog_data.base.base.pull_param =
- rzalloc_array(NULL, const float *, param_count);
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
/* Setting nr_params here NOT to the size of the param and pull_param
* arrays, but to the number of uniform components vec4_visitor
* needs. vec4_visitor::setup_uniforms() will set it back to a proper value.
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index 1b46850..4863cae 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -691,11 +691,11 @@ vec4_visitor::setup_uniform_values(ir_variable *ir)
int i;
for (i = 0; i < uniform_vector_size[uniforms]; i++) {
- stage_prog_data->param[uniforms * 4 + i] = &components->f;
+ stage_prog_data->param[uniforms * 4 + i] = components;
components++;
}
for (; i < 4; i++) {
- static float zero = 0;
+ static gl_constant_value zero = { .f = 0.0 };
stage_prog_data->param[uniforms * 4 + i] = &zero;
}
@@ -715,7 +715,8 @@ vec4_visitor::setup_uniform_clipplane_values()
this->userplane[i] = dst_reg(UNIFORM, this->uniforms);
this->userplane[i].type = BRW_REGISTER_TYPE_F;
for (int j = 0; j < 4; ++j) {
- stage_prog_data->param[this->uniforms * 4 + j] = &clip_planes[i][j];
+ stage_prog_data->param[this->uniforms * 4 + j] =
+ (gl_constant_value *) &clip_planes[i][j];
}
++this->uniforms;
}
@@ -739,7 +740,8 @@ vec4_visitor::setup_builtin_uniform_values(ir_variable *ir)
*/
int index = _mesa_add_state_reference(this->prog->Parameters,
(gl_state_index *)slots[i].tokens);
- float *values = &this->prog->Parameters->ParameterValues[index][0].f;
+ gl_constant_value *values =
+ &this->prog->Parameters->ParameterValues[index][0];
assert(this->uniforms < uniform_array_size);
this->uniform_vector_size[this->uniforms] = 0;
@@ -3309,7 +3311,8 @@ vec4_visitor::move_uniform_array_access_to_pull_constants()
* add it.
*/
if (pull_constant_loc[uniform] == -1) {
- const float **values = &stage_prog_data->param[uniform * 4];
+ const gl_constant_value **values =
+ &stage_prog_data->param[uniform * 4];
pull_constant_loc[uniform] = stage_prog_data->nr_pull_params / 4;
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_vp.cpp b/src/mesa/drivers/dri/i965/brw_vec4_vp.cpp
index f9c23ca..610fea2 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_vp.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_vp.cpp
@@ -401,7 +401,7 @@ vec4_vs_visitor::emit_program_code()
unsigned i;
for (i = 0; i < params->NumParameters * 4; i++) {
stage_prog_data->pull_param[i] =
- ¶ms->ParameterValues[i / 4][i % 4].f;
+ ¶ms->ParameterValues[i / 4][i % 4];
}
stage_prog_data->nr_pull_params = i;
}
@@ -432,7 +432,7 @@ vec4_vs_visitor::setup_vp_regs()
this->uniform_vector_size[this->uniforms] = components;
for (unsigned i = 0; i < 4; i++) {
stage_prog_data->param[this->uniforms * 4 + i] = i >= components
- ? 0 : &plist->ParameterValues[p][i].f;
+ ? 0 : &plist->ParameterValues[p][i];
}
this->uniforms++; /* counted in vec4 units */
}
diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
index 19b1d3b..4574c3e 100644
--- a/src/mesa/drivers/dri/i965/brw_vs.c
+++ b/src/mesa/drivers/dri/i965/brw_vs.c
@@ -233,8 +233,10 @@ do_vs_prog(struct brw_context *brw,
*/
param_count += c.key.base.nr_userclip_plane_consts * 4;
- stage_prog_data->param = rzalloc_array(NULL, const float *, param_count);
- stage_prog_data->pull_param = rzalloc_array(NULL, const float *, param_count);
+ stage_prog_data->param =
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
+ stage_prog_data->pull_param =
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
/* Setting nr_params here NOT to the size of the param and pull_param
* arrays, but to the number of uniform components vec4_visitor
diff --git a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
index ef4a77a..1cc96cf 100644
--- a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
@@ -76,8 +76,10 @@ brw_upload_pull_constants(struct brw_context *brw,
uint32_t size = prog_data->nr_pull_params * 4;
drm_intel_bo *const_bo = NULL;
uint32_t const_offset;
- float *constants = intel_upload_space(brw, size, 64,
- &const_bo, &const_offset);
+ gl_constant_value *constants = intel_upload_space(brw, size, 64,
+ &const_bo, &const_offset);
+
+ STATIC_ASSERT(sizeof(gl_constant_value) == sizeof(float));
for (i = 0; i < prog_data->nr_pull_params; i++) {
constants[i] = *prog_data->pull_param[i];
@@ -85,9 +87,9 @@ brw_upload_pull_constants(struct brw_context *brw,
if (0) {
for (i = 0; i < ALIGN(prog_data->nr_pull_params, 4) / 4; i++) {
- float *row = &constants[i * 4];
+ const gl_constant_value *row = &constants[i * 4];
fprintf(stderr, "const surface %3d: %4.3f %4.3f %4.3f %4.3f\n",
- i, row[0], row[1], row[2], row[3]);
+ i, row[0].f, row[1].f, row[2].f, row[3].f);
}
}
diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
index 6e068d3..2e3cd4b 100644
--- a/src/mesa/drivers/dri/i965/brw_wm.c
+++ b/src/mesa/drivers/dri/i965/brw_wm.c
@@ -169,9 +169,10 @@ bool do_wm_prog(struct brw_context *brw,
}
/* The backend also sometimes adds params for texture size. */
param_count += 2 * ctx->Const.Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits;
- prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
+ prog_data.base.param =
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
prog_data.base.pull_param =
- rzalloc_array(NULL, const float *, param_count);
+ rzalloc_array(NULL, const gl_constant_value *, param_count);
prog_data.base.nr_params = param_count;
prog_data.barycentric_interp_modes =
diff --git a/src/mesa/drivers/dri/i965/gen6_vs_state.c b/src/mesa/drivers/dri/i965/gen6_vs_state.c
index 905e123..77f566c 100644
--- a/src/mesa/drivers/dri/i965/gen6_vs_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_vs_state.c
@@ -67,13 +67,15 @@ gen6_upload_push_constants(struct brw_context *brw,
if (prog_data->nr_params == 0) {
stage_state->push_const_size = 0;
} else {
- float *param;
+ gl_constant_value *param;
int i;
param = brw_state_batch(brw, type,
- prog_data->nr_params * sizeof(float),
+ prog_data->nr_params * sizeof(gl_constant_value),
32, &stage_state->push_const_offset);
+ STATIC_ASSERT(sizeof(gl_constant_value) == sizeof(float));
+
/* _NEW_PROGRAM_CONSTANTS
*
* Also _NEW_TRANSFORM -- we may reference clip planes other than as a
@@ -91,7 +93,7 @@ gen6_upload_push_constants(struct brw_context *brw,
if ((i & 7) == 0)
fprintf(stderr, "g%d: ",
prog_data->dispatch_grf_start_reg + i / 8);
- fprintf(stderr, "%8f ", param[i]);
+ fprintf(stderr, "%8f ", param[i].f);
if ((i & 7) == 7)
fprintf(stderr, "\n");
}
--
1.9.3
More information about the mesa-dev
mailing list