Re: [Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.
On 12/20/2013 09:26 PM, Kenneth Graunke wrote:
On 11/27/2013 05:28 AM, Petri Latvala wrote:
v2: Don't add function parameters, pass the required size in
prog_data->nr_params.
Signed-off-by: Petri Latvala
---
src/mesa/drivers/dri/i965/brw_vec4.h | 5 +++--
src/mesa/drivers/dri/i965/brw_vec4_gs.c| 5 +
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 7 +++
src/mesa/drivers/dri/i965/brw_vs.c | 8
4 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
b/src/mesa/drivers/dri/i965/brw_vec4.h
index 5cec9f9..5f5f5cd 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4.h
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h
@@ -325,8 +325,9 @@ public:
*/
dst_reg output_reg[BRW_VARYING_SLOT_COUNT];
const char *output_reg_annotation[BRW_VARYING_SLOT_COUNT];
- int uniform_size[MAX_UNIFORMS];
- int uniform_vector_size[MAX_UNIFORMS];
+ int *uniform_size;
+ int *uniform_vector_size;
+ int uniform_param_count; /*< Size of uniform_[vector_]size arrays */
I'm not crazy about this variable name. Between the params arrays,
uniform_* arrays, nr_params count, and uniforms count...we already have
a lot of distinct things that sound alike.
How about:
int uniform_array_size; /**< Size of uniform_[vector_]size arrays. */
That seems clearer to me. Especially seeing that the value here is
really the size of the array, which is an overestimate/upper bound on
the number of uniforms, not the actual number of elements in the
uniforms or params arrays.
Sounds good. Changing the name.
int uniforms;
src_reg shader_start_time;
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
index 018b0b6..7cf9bac 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
+++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
@@ -64,6 +64,11 @@ do_gs_prog(struct brw_context *brw,
c.prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
c.prog_data.base.pull_param = rzalloc_array(NULL, const float *,
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.
+*/
+ c.prog_data.base.nr_params = param_count / 4 + gs->num_samplers;
Hmm. You're counting the number of vec4s, but...don't samplers take up
a single entry, since they're just integers? This seems odd to me.
No, that value is the number of float-size components. param_count,
calculated above that code, multiplies the number of components by 4 to
have room elsewhere for scalar params that eat up a vec4. I'm just
dividing the number back.
You might also consider doing ALIGN(param_count, 4) / 4 so that you
round up rather than truncating on the division.
Ok, changing that. Here and in vs.
I also would really like to keep nr_params in consistent units, i.e.
always uniform float-size components or always vec4s.
I would really like that too, but unfortunately the inconsistency was
already present.
The best option would be to have another variable for uniform memory
use, but I didn't want to make too invasive changes. nr_param gets used
here for uniform float-size counts, and later on for something else.
This results in some overuse of memory when uniforms are smaller than vec4s.
if (gp->program.OutputType == GL_POINTS) {
/* When the output type is points, the geometry shader may output data
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index a13eafb..b9226dc 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -3253,6 +3253,10 @@ vec4_visitor::vec4_visitor(struct brw_context *brw,
fail_msg(NULL),
first_non_payload_grf(0),
need_all_constants_in_pull_buffer(false),
+ /* Initialize uniform_param_count to at least 1 because gen6 VS requires
at
+ * least one. See setup_uniforms() in brw_vec4.cpp.
+ */
I think you mean "Gen4-5 requires at least one push constant", not "gen6
VS." At least, that's what setup_uniforms() is doing.
Argh, yes. Fixing that comment.
+ uniform_param_count(prog_data->nr_params ? prog_data->nr_params : 1),
I think this would be clearer as:
MAX2(prog_data->nr_params, 1)
Yes, changing.
--
Petri Latvala
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.
On 12/20/2013 08:54 PM, Ian Romanick wrote: This patch breaks the test_vec4_register_coalesce unit test. Did you run 'make check'? I thought I did, but turns out I didn't. Just ran piglit tests. Fix coming up. -- Petri Latvala ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.
v2: Don't add function parameters, pass the required size in
prog_data->nr_params.
v3:
- Use the name uniform_array_size instead of uniform_param_count.
- Round up when dividing param_count by 4.
- Use MAX2() instead of taking the maximum by hand.
- Don't crash if prog_data passed to vec4_visitor constructor is NULL
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71254
Signed-off-by: Petri Latvala
---
src/mesa/drivers/dri/i965/brw_vec4.h | 5 +++--
src/mesa/drivers/dri/i965/brw_vec4_gs.c| 5 +
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 11 +++
src/mesa/drivers/dri/i965/brw_vs.c | 8
4 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
b/src/mesa/drivers/dri/i965/brw_vec4.h
index d4029d8..798d8bd 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4.h
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h
@@ -325,8 +325,9 @@ public:
*/
dst_reg output_reg[BRW_VARYING_SLOT_COUNT];
const char *output_reg_annotation[BRW_VARYING_SLOT_COUNT];
- int uniform_size[MAX_UNIFORMS];
- int uniform_vector_size[MAX_UNIFORMS];
+ int *uniform_size;
+ int *uniform_vector_size;
+ int uniform_array_size; /*< Size of uniform_[vector_]size arrays */
int uniforms;
src_reg shader_start_time;
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
index 018b0b6..c749eb1 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
+++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
@@ -64,6 +64,11 @@ do_gs_prog(struct brw_context *brw,
c.prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
c.prog_data.base.pull_param = rzalloc_array(NULL, const float *,
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.
+*/
+ c.prog_data.base.nr_params = ALIGN(param_count, 4) / 4 + gs->num_samplers;
if (gp->program.OutputType == GL_POINTS) {
/* When the output type is points, the geometry shader may output data
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index 3b8cef6..a93fdc5 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -3321,6 +3321,17 @@ vec4_visitor::vec4_visitor(struct brw_context *brw,
this->max_grf = brw->gen >= 7 ? GEN7_MRF_HACK_START : BRW_MAX_GRF;
this->uniforms = 0;
+
+ /* Initialize uniform_array_size to at least 1 because pre-gen6 VS requires
+* at least one. See setup_uniforms() in brw_vec4.cpp.
+*/
+ this->uniform_array_size = 1;
+ if (prog_data) {
+ this->uniform_array_size = MAX2(prog_data->nr_params, 1);
+ }
+
+ this->uniform_size = rzalloc_array(mem_ctx, int, this->uniform_array_size);
+ this->uniform_vector_size = rzalloc_array(mem_ctx, int,
this->uniform_array_size);
}
vec4_visitor::~vec4_visitor()
diff --git a/src/mesa/drivers/dri/i965/brw_vs.c
b/src/mesa/drivers/dri/i965/brw_vs.c
index 5d50c3c..609a575 100644
--- a/src/mesa/drivers/dri/i965/brw_vs.c
+++ b/src/mesa/drivers/dri/i965/brw_vs.c
@@ -242,6 +242,14 @@ do_vs_prog(struct brw_context *brw,
prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
prog_data.base.pull_param = rzalloc_array(NULL, const float *, 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.
+*/
+ prog_data.base.nr_params = ALIGN(param_count, 4) / 4;
+ if (vs) {
+ prog_data.base.nr_params += vs->num_samplers;
+ }
GLbitfield64 outputs_written = vp->program.Base.OutputsWritten;
prog_data.inputs_read = vp->program.Base.InputsRead;
--
1.8.4.rc3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] i965: Assert array index on access to vec4_visitor's arrays.
Signed-off-by: Petri Latvala
---
src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 ++
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 6 ++
2 files changed, 8 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
b/src/mesa/drivers/dri/i965/brw_vec4.cpp
index fb57707..4dc0482 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
@@ -411,6 +411,7 @@ vec4_visitor::pack_uniform_registers()
/* Now, figure out a packing of the live uniform vectors into our
* push constants.
*/
+ assert(uniforms < uniform_array_size);
for (int src = 0; src < uniforms; src++) {
int size = this->uniform_vector_size[src];
@@ -1393,6 +1394,7 @@ vec4_visitor::setup_uniforms(int reg)
* matter what, or the GPU would hang.
*/
if (brw->gen < 6 && this->uniforms == 0) {
+ assert(this->uniforms < this->uniform_array_size);
this->uniform_vector_size[this->uniforms] = 1;
prog_data->param = reralloc(NULL, prog_data->param, const float *, 4);
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index a93fdc5..8d4c557 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -662,6 +662,7 @@ vec4_visitor::setup_uniform_values(ir_variable *ir)
storage->type->matrix_columns);
for (unsigned s = 0; s < vector_count; s++) {
+ assert(uniforms < uniform_array_size);
uniform_vector_size[uniforms] = storage->type->vector_elements;
int i;
@@ -685,6 +686,7 @@ vec4_visitor::setup_uniform_clipplane_values()
gl_clip_plane *clip_planes = brw_select_clip_planes(ctx);
for (int i = 0; i < key->nr_userclip_plane_consts; ++i) {
+ assert(this->uniforms < uniform_array_size);
this->uniform_vector_size[this->uniforms] = 4;
this->userplane[i] = dst_reg(UNIFORM, this->uniforms);
this->userplane[i].type = BRW_REGISTER_TYPE_F;
@@ -715,6 +717,7 @@ vec4_visitor::setup_builtin_uniform_values(ir_variable *ir)
(gl_state_index *)slots[i].tokens);
float *values = &this->prog->Parameters->ParameterValues[index][0].f;
+ assert(this->uniforms < uniform_array_size);
this->uniform_vector_size[this->uniforms] = 0;
/* Add each of the unique swizzled channels of the element.
* This will end up matching the size of the glsl_type of this field.
@@ -725,6 +728,7 @@ vec4_visitor::setup_builtin_uniform_values(ir_variable *ir)
last_swiz = swiz;
prog_data->param[this->uniforms * 4 + j] = &values[swiz];
+assert(this->uniforms < uniform_array_size);
if (swiz <= last_swiz)
this->uniform_vector_size[this->uniforms]++;
}
@@ -983,6 +987,7 @@ vec4_visitor::visit(ir_variable *ir)
/* Track how big the whole uniform variable is, in case we need to put a
* copy of its data into pull constants for array access.
*/
+ assert(this->uniforms < uniform_array_size);
this->uniform_size[this->uniforms] = type_size(ir->type);
if (!strncmp(ir->name, "gl_", 3)) {
@@ -3228,6 +3233,7 @@
vec4_visitor::move_uniform_array_access_to_pull_constants()
pull_constant_loc[uniform] = prog_data->nr_pull_params / 4;
+ assert(uniform < uniform_array_size);
for (int j = 0; j < uniform_size[uniform] * 4; j++) {
prog_data->pull_param[prog_data->nr_pull_params++]
= values[j];
--
1.8.4.rc3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] v4: Fix array overrun with too many uniforms
Fourth version of patch series. - Fixed vec4_register_coalesce unit test. That test passes NULL for prog_data, which Mesa proper doesn't do. - Renamed uniform_param_count to uniform_array_size. - Used ALIGN() to round up when dividing buffer size by 4. - Used MAX2() instead of taking maximum manually. Petri Latvala (2): i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically. i965: Assert array index on access to vec4_visitor's arrays. src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 ++ src/mesa/drivers/dri/i965/brw_vec4.h | 5 +++-- src/mesa/drivers/dri/i965/brw_vec4_gs.c| 5 + src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 17 + src/mesa/drivers/dri/i965/brw_vs.c | 8 5 files changed, 35 insertions(+), 2 deletions(-) -- 1.8.4.rc3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 73144] New: Queries on textures with borders give incorrect results
https://bugs.freedesktop.org/show_bug.cgi?id=73144 Priority: medium Bug ID: 73144 Assignee: mesa-dev@lists.freedesktop.org Summary: Queries on textures with borders give incorrect results Severity: normal Classification: Unclassified OS: All Reporter: bme...@gmail.com Hardware: Other Status: NEW Version: 10.0 Component: Drivers/X11 Product: Mesa When I create a 1D texture with width 6 and border 1, querying GL_TEXTURE_WIDTH and GL_TEXTURE_BORDER return 4 and 0 respectively, instead of 6 and 1. I will attach a piglit test. Mesa 10.0.1 configured with --enable-xlib-glx --disable-dri --with-gallium-drivers=swrast --with-egl-platforms=x11 and LD_LIBRARY_PATH=/lib (i.e. not the gallium subdirectory). -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 73144] Queries on textures with borders give incorrect results
https://bugs.freedesktop.org/show_bug.cgi?id=73144 --- Comment #1 from Bruce Merry --- Created attachment 91328 --> https://bugs.freedesktop.org/attachment.cgi?id=91328&action=edit Patch to add test to piglit -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: Fix null access on file read error
Signed-off-by: Juha-Pekka Heikkila
---
src/glsl/main.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/glsl/main.cpp b/src/glsl/main.cpp
index aa188b1..755bc9a 100644
--- a/src/glsl/main.cpp
+++ b/src/glsl/main.cpp
@@ -221,7 +221,7 @@ load_text_file(void *ctx, const char *file_name)
if (bytes < size - total_read) {
free(text);
text = NULL;
- break;
+ goto error;
}
if (bytes == 0) {
@@ -232,6 +232,7 @@ load_text_file(void *ctx, const char *file_name)
} while (total_read < size);
text[total_read] = '\0';
+error:
}
fclose(fp);
--
1.8.1.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa: remove _mesa_symbol_table_iterator structure
Nothing uses this structure, removal fixes Klocwork error about
the possible oom condition in _mesa_symbol_table_iterator_ctor.
Signed-off-by: Tapani Pälli
---
src/mesa/program/symbol_table.c | 86 -
src/mesa/program/symbol_table.h | 13 ---
2 files changed, 99 deletions(-)
diff --git a/src/mesa/program/symbol_table.c b/src/mesa/program/symbol_table.c
index 4f6f31f..2f41322 100644
--- a/src/mesa/program/symbol_table.c
+++ b/src/mesa/program/symbol_table.c
@@ -112,24 +112,6 @@ struct _mesa_symbol_table {
};
-struct _mesa_symbol_table_iterator {
-/**
- * Name space of symbols returned by this iterator.
- */
-int name_space;
-
-
-/**
- * Currently iterated symbol
- *
- * The next call to \c _mesa_symbol_table_iterator_get will return this
- * value. It will also update this value to the value that should be
- * returned by the next call.
- */
-struct symbol *curr;
-};
-
-
static void
check_symbol_table(struct _mesa_symbol_table *table)
{
@@ -201,74 +183,6 @@ find_symbol(struct _mesa_symbol_table *table, const char
*name)
}
-struct _mesa_symbol_table_iterator *
-_mesa_symbol_table_iterator_ctor(struct _mesa_symbol_table *table,
- int name_space, const char *name)
-{
-struct _mesa_symbol_table_iterator *iter = calloc(1, sizeof(*iter));
-struct symbol_header *const hdr = find_symbol(table, name);
-
-iter->name_space = name_space;
-
-if (hdr != NULL) {
-struct symbol *sym;
-
-for (sym = hdr->symbols; sym != NULL; sym = sym->next_with_same_name) {
-assert(sym->hdr == hdr);
-
-if ((name_space == -1) || (sym->name_space == name_space)) {
-iter->curr = sym;
-break;
-}
-}
-}
-
-return iter;
-}
-
-
-void
-_mesa_symbol_table_iterator_dtor(struct _mesa_symbol_table_iterator *iter)
-{
-free(iter);
-}
-
-
-void *
-_mesa_symbol_table_iterator_get(struct _mesa_symbol_table_iterator *iter)
-{
-return (iter->curr == NULL) ? NULL : iter->curr->data;
-}
-
-
-int
-_mesa_symbol_table_iterator_next(struct _mesa_symbol_table_iterator *iter)
-{
-struct symbol_header *hdr;
-
-if (iter->curr == NULL) {
-return 0;
-}
-
-hdr = iter->curr->hdr;
-iter->curr = iter->curr->next_with_same_name;
-
-while (iter->curr != NULL) {
-assert(iter->curr->hdr == hdr);
-(void)hdr;
-
-if ((iter->name_space == -1)
-|| (iter->curr->name_space == iter->name_space)) {
-return 1;
-}
-
-iter->curr = iter->curr->next_with_same_name;
-}
-
-return 0;
-}
-
-
/**
* Determine the scope "distance" of a symbol from the current scope
*
diff --git a/src/mesa/program/symbol_table.h b/src/mesa/program/symbol_table.h
index f9d9164..1027f47 100644
--- a/src/mesa/program/symbol_table.h
+++ b/src/mesa/program/symbol_table.h
@@ -24,7 +24,6 @@
#define MESA_SYMBOL_TABLE_H
struct _mesa_symbol_table;
-struct _mesa_symbol_table_iterator;
extern void _mesa_symbol_table_push_scope(struct _mesa_symbol_table *table);
@@ -47,16 +46,4 @@ extern struct _mesa_symbol_table
*_mesa_symbol_table_ctor(void);
extern void _mesa_symbol_table_dtor(struct _mesa_symbol_table *);
-extern struct _mesa_symbol_table_iterator *_mesa_symbol_table_iterator_ctor(
-struct _mesa_symbol_table *table, int name_space, const char *name);
-
-extern void _mesa_symbol_table_iterator_dtor(
-struct _mesa_symbol_table_iterator *);
-
-extern void *_mesa_symbol_table_iterator_get(
-struct _mesa_symbol_table_iterator *iter);
-
-extern int _mesa_symbol_table_iterator_next(
-struct _mesa_symbol_table_iterator *iter);
-
#endif /* MESA_SYMBOL_TABLE_H */
--
1.8.3.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72926] Memory corruption (crash) in draw/draw_pt_fetch_shade_pipeline_llvm.c:435
https://bugs.freedesktop.org/show_bug.cgi?id=72926 --- Comment #5 from Peter Wu --- Created attachment 91336 --> https://bugs.freedesktop.org/attachment.cgi?id=91336&action=edit Output for `LIBGL_ALWAYS_SOFTWARE=1 valgrind glretrace -v java.trace` Same config, but with `-v` added to `glretrace`. -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/23] i965/fs: Add support for specifying register horizontal strides.
On 19 December 2013 14:52, Francisco Jerez wrote: > Paul Berry writes: > > >[...] > > In v2 of this patch, you add the following code to > > fs_visitor::try_copy_propagate(): > > > > + /* Bail if the result of composing both strides cannot be expressed > > +* as another stride. > > +*/ > > + if (entry->src.stride != 1 && > > + (inst->src[arg].stride * > > +type_sz(inst->src[arg].type)) % type_sz(entry->src.type) != 0) > >return false; > > > > I don't understand. It seems like this is trying to make sure that > > (src_stride * src_type_sz) / entry_type_sz is an integer so we can later > > use the factor (src_type_sz / entry_type_sz) as a multiplicative factor > to > > correct the stride without creating a fractional result. But I don't see > > us applying this correction factor anywhere. Is there some code missing? > > That isn't exactly the purpose of that check. > > The problem arises when you are trying to compose two regions of > different base types. If the stride in bytes of 'inst->src[arg]' is not > a multiple of the element size of 'entry->src' you end up with an > inhomogeneous stride that can only be expressed as a two-dimensional > region. Instead of adding support for two-dimensional regions (which > would be hard, not very useful, and only a partial solution because then > we would need to handle composition of two-dimensional regions that > yield three- and four-dimensional regions as a result) I disallowed > copy-propagation in that case. > > Thanks. > Ok, I see. I had to spend quite a while fiddling with scratch paper to understand this. Would you mind adding an example to the comment to clarify this? Perhaps something like this: /* Bail if the result of composing both strides cannot be expressed as * another stride. This avoids, for example, trying to transform this: * * MOV (8) rX<1>UD rY<0;1,0>UD * FOO (8) ... rX<8;8,2>UW * * into this: * * FOO (8) ... rY<8;8,0>UW * * Which would have different semantics. */ Note: the code sequence in my example is bogus because it crosses data between SIMD execution paths, so it would never actually arise in practice. I wasn't able to come up with a realistic example; in fact, after trying and failing to come up with a non-bogus example, I'm pretty convinced that this particular bailout case will never happen unless there's a bug elsewhere in the compiler. You might want to consider adding an assert(false) into the code path so that if it ever does occur we'll have a chance to investigate. I don't have strong feelings about adding the assert, though. With the comment updated (and the other comment suggestion from my previous email about is_power_of_two()), this patch is: Reviwed-by: Paul Berry ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/23] i965/fs: Remove fs_reg::sechalf.
On 2 December 2013 11:31, Francisco Jerez wrote:
> +/**
> + * Get either of the 8-component halves of a 16-component register.
> + */
> +static inline fs_reg
> +half(const fs_reg ®, unsigned idx)
> +{
> + assert(idx == 0 || (reg.file != HW_REG && reg.file != IMM));
> + return byte_offset(reg, 8 * idx * reg.stride * type_sz(reg.type));
> +}
> +
>
I'd like to see a comment explaining that it's safe to call this function
on a register with 32 bits per component, since brw_reg.h's byte_offset()
handles byte offsets greater than 32 by incrementing the register number.
Also, for sanity's sake, it would be nice for this function to include the
assertion:
assert(idx < 2);
With those changes, this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72877] Wrong colors with Mesa 9.2 and Mesa 10.0 on PPC Linux systems
https://bugs.freedesktop.org/show_bug.cgi?id=72877
--- Comment #3 from Christian Zigotzky ---
(In reply to comment #2)
> This is probably due to commit 2151d893fbd4a4be092098170e2fbca8c35797a5
> ('gallium: Fix llvmpipe on big-endian machines'). The r600g driver needs to
> be adapted for the changed PIPE_FORMAT_* semantics.
I don't know if I have correct understand your answer. Is the bug fixed?
--
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 13/23] i965/fs: Take into account reg_offset consistently for MRF regs.
On 2 December 2013 11:31, Francisco Jerez wrote:
> Until now it was only being taken into account in the VEC4 back-end
> but not in the FS back-end. Do it in both cases.
> ---
> src/mesa/drivers/dri/i965/brw_fs.h | 2 +-
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 10 ++
> src/mesa/drivers/dri/i965/brw_shader.h | 7 ---
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index 2c36d9f..f918f7e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -615,4 +615,4 @@ bool brw_do_channel_expressions(struct exec_list
> *instructions);
> bool brw_do_vector_splitting(struct exec_list *instructions);
> bool brw_fs_precompile(struct gl_context *ctx, struct gl_shader_program
> *prog);
>
> -struct brw_reg brw_reg_from_fs_reg(fs_reg *reg);
> +struct brw_reg brw_reg_from_fs_reg(fs_reg *reg, unsigned dispatch_width);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 8d310a1..1de59eb 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -981,8 +981,9 @@ static uint32_t brw_file_from_reg(fs_reg *reg)
> }
>
> struct brw_reg
> -brw_reg_from_fs_reg(fs_reg *reg)
> +brw_reg_from_fs_reg(fs_reg *reg, unsigned dispatch_width)
> {
> + const int reg_size = 4 * dispatch_width;
>
What happens when reg.type is UW and dispatch_width is 16? In that case,
we would compute reg_size == 64, but the correct value seems like it's
actually 32 in this case.
Are we perhaps relying on reg.type being a 32-bit type? If so, maybe we
should add an assertion:
assert(type_sz(reg.type) == 4);
With that added, this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 17/23] i965/vec4: Trivial improvements to the with_writemask() function.
On 2 December 2013 11:31, Francisco Jerez wrote:
> Add assertion that the register is not in the HW_REG or IMM file,
> calculate the conjunction of the old and new mask instead of replacing
> the old [consistent with the behavior of brw_writemask(), causes no
> functional changes right now], make it static inline to let the
> compiler do a slightly better job at optimizing things, and shorten
> its name.
> ---
> src/mesa/drivers/dri/i965/brw_vec4.h | 9 +++--
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp| 11 +--
> src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp | 12 ++--
> 3 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 19de4c6..50e4794 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -192,8 +192,13 @@ offset(dst_reg reg, unsigned delta)
> return reg;
> }
>
> -dst_reg
> -with_writemask(dst_reg const &r, int mask);
> +static inline dst_reg
> +writemask(dst_reg reg, unsigned mask)
> +{
> + assert(reg.file != HW_REG && reg.file != IMM);
> + reg.writemask &= mask;
> + return reg;
> +}
>
IIRC, hardware behaviour is undefined if the destination of an instruction
has a writemask of 0. Should we add an assertion here to verify that the
new reg.writemask != 0?
With that addressed, this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 21/23] i965: Add helper function to find out the signedness of a register type.
On 2 December 2013 11:36, Francisco Jerez wrote:
> ---
> src/mesa/drivers/dri/i965/brw_reg.h | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h
> b/src/mesa/drivers/dri/i965/brw_reg.h
> index 37a2ca9..2591cbf 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -154,6 +154,27 @@ type_sz(unsigned type)
> }
> }
>
> +static inline bool
> +type_is_signed(unsigned type)
> +{
> + switch(type) {
> + case BRW_REGISTER_TYPE_D:
> + case BRW_REGISTER_TYPE_F:
> + case BRW_REGISTER_TYPE_HF:
> + case BRW_REGISTER_TYPE_W:
> + case BRW_REGISTER_TYPE_B:
> + return true;
> +
> + case BRW_REGISTER_TYPE_UD:
> + case BRW_REGISTER_TYPE_UW:
> + case BRW_REGISTER_TYPE_UB:
> + return false;
> +
> + default:
> + unreachable();
> + }
> +}
> +
>
If the call to unreachable() is replaced with an assertion (as we've
discussed elsewhere on the list), this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 23/23] i965: Add polymorphic backend_visitor method to extract the result of a visit.
On 2 December 2013 11:36, Francisco Jerez wrote: > This will be used by the generic implementation of the image and > atomic counter built-ins to extract the register location of its > arguments without having to be aware of the actual visitor type. > I have the same concerns about object slicing with this patch that I had with patch 06/23. I've sent comments on patches 2, 4, 6, 8, 10, 13, 17, and 21. The remainder of this series is: Reviewed-by: Paul Berry ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] glx: Add some missing null checks in glx_pbuffer.c
Signed-off-by: Juha-Pekka Heikkila
---
src/glx/glx_pbuffer.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 183fbaa..411d6e5 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -60,7 +60,7 @@ warn_GLX_1_3(Display * dpy, const char *function_name)
{
struct glx_display *priv = __glXInitialize(dpy);
- if (priv->minorVersion < 3) {
+ if (priv && priv->minorVersion < 3) {
fprintf(stderr,
"WARNING: Application calling GLX 1.3 function \"%s\" "
"when GLX 1.3 is not supported! This is an application bug!\n",
@@ -91,7 +91,7 @@ ChangeDrawableAttribute(Display * dpy, GLXDrawable drawable,
CARD8 opcode;
int i;
- if ((dpy == NULL) || (drawable == 0)) {
+ if ((priv == NULL) || (dpy == NULL) || (drawable == 0)) {
return;
}
@@ -197,6 +197,11 @@ CreateDRIDrawable(Display *dpy, struct glx_config *config,
__GLXDRIdrawable *pdraw;
struct glx_screen *psc;
+ if (priv == NULL) {
+ fprintf(stderr, "failed to create drawable\n");
+ return GL_FALSE;
+ }
+
psc = priv->screens[config->screen];
if (psc->driScreen == NULL)
return GL_TRUE;
@@ -226,7 +231,7 @@ DestroyDRIDrawable(Display *dpy, GLXDrawable drawable, int
destroy_xdrawable)
__GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, drawable);
XID xid;
- if (pdraw != NULL) {
+ if (priv != NULL && pdraw != NULL) {
xid = pdraw->xDrawable;
(*pdraw->destroyDrawable) (pdraw);
__glxHashDelete(priv->drawHash, drawable);
@@ -294,6 +299,9 @@ GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
}
priv = __glXInitialize(dpy);
+ if (priv == NULL)
+ return 0;
+
use_glx_1_3 = ((priv->majorVersion > 1) || (priv->minorVersion >= 3));
*value = 0;
@@ -504,6 +512,9 @@ CreatePbuffer(Display * dpy, struct glx_config *config,
Pixmap pixmap;
GLboolean glx_1_3 = GL_FALSE;
+ if (priv == NULL)
+ return None;
+
i = 0;
if (attrib_list) {
while (attrib_list[i * 2])
@@ -593,7 +604,7 @@ DestroyPbuffer(Display * dpy, GLXDrawable drawable)
struct glx_display *priv = __glXInitialize(dpy);
CARD8 opcode;
- if ((dpy == NULL) || (drawable == 0)) {
+ if ((priv == NULL) || (dpy == NULL) || (drawable == 0)) {
return;
}
--
1.8.1.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] glsl: Fix null access on file read error
Signed-off-by: Juha-Pekka Heikkila
---
src/glsl/main.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/glsl/main.cpp b/src/glsl/main.cpp
index aa188b1..ff69c9a 100644
--- a/src/glsl/main.cpp
+++ b/src/glsl/main.cpp
@@ -221,7 +221,7 @@ load_text_file(void *ctx, const char *file_name)
if (bytes < size - total_read) {
free(text);
text = NULL;
- break;
+ goto error;
}
if (bytes == 0) {
@@ -232,6 +232,7 @@ load_text_file(void *ctx, const char *file_name)
} while (total_read < size);
text[total_read] = '\0';
+error:;
}
fclose(fp);
--
1.8.1.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] two Klocwork related patches.
Some null checks into glx_pbuffer.c. Still wonder should these be made as asserts instead of regular "if"s because if __glXInitialize() return NULL here there is something very wrong. glsl patch is the same as I sent earlier today with one typo fixed. It somehow was missing one semicolon causing a build error. Juha-Pekka Heikkila (2): glsl: Fix null access on file read error glx: Add some missing null checks in glx_pbuffer.c src/glsl/main.cpp | 3 ++- src/glx/glx_pbuffer.c | 19 +++ 2 files changed, 17 insertions(+), 5 deletions(-) -- 1.8.1.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/25] i965/gen7: Factor out texture surface state set-up from gen7_update_texture_surface().
On 2 December 2013 11:39, Francisco Jerez wrote:
> This moves most of the surface state set-up logic that can be shared
> between textures and shader images to a separate function.
>
Let's make a note in the commit message that this causes the "Render Target
View Extent" field to be set on texture surfaces now. I don't think that's
a problem, but it's nice to have it documented so that in case a problem
later bisects to this commit we'll have a hint as to the behavioural change.
> +static void
> +gen7_update_texture_surface(struct gl_context *ctx,
> +unsigned unit,
> +uint32_t *surf_offset,
> +bool for_gather)
> +{
> + struct brw_context *brw = brw_context(ctx);
> + struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
> +
> + if (obj->Target == GL_TEXTURE_BUFFER) {
> + brw_update_buffer_texture_surface(ctx, unit, surf_offset);
> +
>
Spurious newline here.
> + } else {
> + struct intel_texture_object *intel_obj = intel_texture_object(obj);
> + struct intel_mipmap_tree *mt = intel_obj->mt;
> + struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
> + unsigned tex_format = translate_tex_format(
> + brw, mt->format, obj->DepthMode, sampler->sRGBDecode);
> +
> + if (for_gather && tex_format == BRW_SURFACEFORMAT_R32G32_FLOAT)
> + tex_format = BRW_SURFACEFORMAT_R32G32_FLOAT_LD;
> +
> + gen7_emit_texture_surface_state(brw, obj,
> + 0, mt->logical_depth0,
> + obj->BaseLevel,
> intel_obj->_MaxLevel,
> + tex_format, surf_offset,
> + false, for_gather);
> + }
> +}
> +
With those changes, this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/25] i965/gen7: Factor out texture surface state set-up from gen7_update_texture_surface().
+gen7_emit_texture_surface_state(struct brw_context *brw, +struct gl_texture_object *obj, +unsigned min_array_element, +unsigned max_array_element, +unsigned min_level, +unsigned max_level, [snip] + gen7_emit_texture_surface_state(brw, obj, + 0, mt->logical_depth0, + obj->BaseLevel, intel_obj->_MaxLevel, It seems ugly that max_array_element is exclusive, but max_level is inclusive. -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/25] i965/gen7: Implement surface state set-up for shader images.
On 2 December 2013 11:39, Francisco Jerez wrote:
> +static uint32_t
> +get_image_format(struct brw_context *brw, gl_format format)
> +{
>
It's not clear without additional context that this function is only used
in the case where the surface is both written to and read from. Can we
rename it to something like "get_image_format_for_rw"? At the very least
there should be a comment explaining this.
> + switch (format) {
> + case MESA_FORMAT_RGBA_UINT32:
> + case MESA_FORMAT_RGBA_INT32:
> + case MESA_FORMAT_RGBA_FLOAT32:
> + /* Fail... We need to fall back to untyped surface access for
> + * all 128 bpp formats.
> + */
> + return BRW_SURFACEFORMAT_RAW;
> +
> + case MESA_FORMAT_RGBA_UINT16:
> + case MESA_FORMAT_RGBA_INT16:
> + case MESA_FORMAT_RGBA_FLOAT16:
> + case MESA_FORMAT_RGBA_16:
> + case MESA_FORMAT_SIGNED_RGBA_16:
> + case MESA_FORMAT_RG_UINT32:
> + case MESA_FORMAT_RG_INT32:
> + case MESA_FORMAT_RG_FLOAT32:
> + /* HSW supports the R16G16B16A16_UINT format natively and
> + * handles the pixel packing, unpacking and type conversion in
> + * the shader for other 64 bpp formats. IVB falls back to
> + * untyped.
> + */
> + return (brw->is_haswell ? BRW_SURFACEFORMAT_R16G16B16A16_UINT :
> + BRW_SURFACEFORMAT_RAW);
> +
> + case MESA_FORMAT_RGBA_UINT8:
> + case MESA_FORMAT_RGBA_INT8:
> + case MESA_FORMAT_RGBA_REV:
> + case MESA_FORMAT_SIGNED_RGBA_REV:
> + /* HSW supports the R8G8B8A8_UINT format natively, type
> + * conversion to other formats is handled in the shader. IVB
> + * uses R32_UINT and handles the pixel packing, unpacking and
> + * type conversion in the shader.
> + */
> + return (brw->is_haswell ? BRW_SURFACEFORMAT_R8G8B8A8_UINT :
> + BRW_SURFACEFORMAT_R32_UINT);
> +
> + case MESA_FORMAT_RG_UINT16:
> + case MESA_FORMAT_RG_INT16:
> + case MESA_FORMAT_RG_FLOAT16:
> + case MESA_FORMAT_GR1616:
> + case MESA_FORMAT_SIGNED_GR1616:
> + /* HSW supports the R16G16_UINT format natively, type conversion
> + * to other formats is handled in the shader. IVB uses R32_UINT
> + * and handles the pixel packing, unpacking and type conversion
> + * in the shader.
> + */
> + return (brw->is_haswell ? BRW_SURFACEFORMAT_R16G16_UINT :
> + BRW_SURFACEFORMAT_R32_UINT);
> +
> + case MESA_FORMAT_ABGR2101010_UINT:
> + case MESA_FORMAT_ABGR2101010:
> + case MESA_FORMAT_R11_G11_B10_FLOAT:
> + case MESA_FORMAT_R_UINT32:
> + /* Neither the 2/10/10/10 nor the 11/11/10 packed formats are
> + * supported by the hardware. Use R32_UINT and handle the pixel
> + * packing, unpacking, and type conversion in the shader.
> + */
> + return BRW_SURFACEFORMAT_R32_UINT;
> +
> + case MESA_FORMAT_R_INT32:
> + return BRW_SURFACEFORMAT_R32_SINT;
> +
> + case MESA_FORMAT_R_FLOAT32:
> + return BRW_SURFACEFORMAT_R32_FLOAT;
> +
> + case MESA_FORMAT_RG_UINT8:
> + case MESA_FORMAT_RG_INT8:
> + case MESA_FORMAT_GR88:
> + case MESA_FORMAT_SIGNED_RG88_REV:
> + /* HSW supports the R8G8_UINT format natively, type conversion
> + * to other formats is handled in the shader. IVB uses R16_UINT
> + * and handles the pixel packing, unpacking and type conversion
> + * in the shader. Note that this relies on the undocumented
> + * behavior that typed reads from R16_UINT surfaces actually do
> + * a 32-bit misaligned read on IVB. The alternative would be to
> + * use two surface state entries with different formats for each
> + * image, one for reading (using R32_UINT) and another one for
> + * writing (using R8G8_UINT), but that would complicate the
> + * shaders we generate even more.
> + */
> + return (brw->is_haswell ? BRW_SURFACEFORMAT_R8G8_UINT :
> + BRW_SURFACEFORMAT_R16_UINT);
> +
> + case MESA_FORMAT_R_UINT16:
> + case MESA_FORMAT_R_FLOAT16:
> + case MESA_FORMAT_R_INT16:
> + case MESA_FORMAT_R16:
> + case MESA_FORMAT_SIGNED_R16:
> + /* HSW supports the R16_UINT format natively, type conversion to
> + * other formats is handled in the shader. IVB relies on the
> + * same undocumented behavior described above.
> + */
> + return BRW_SURFACEFORMAT_R16_UINT;
> +
> + case MESA_FORMAT_R_UINT8:
> + case MESA_FORMAT_R_INT8:
> + case MESA_FORMAT_R8:
> + case MESA_FORMAT_SIGNED_R8:
> + /* HSW supports the R8_UINT format natively, type conversion to
> + * other formats is handled in the shader. IVB relies on the
> + * same undocumented behavior described above.
> + */
> + return BRW_SURFACEFORMAT_R8_UINT;
> +
> + default:
> + unreachable();
>
Change to an assert.
> + }
> +}
>
With those changes, this patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.
Re: [Mesa-dev] [PATCH 04/25] i965: Add helper functions to calculate the slice pitch of an array or 3D miptree.
On 2 December 2013 11:39, Francisco Jerez wrote: > --- > src/mesa/drivers/dri/i965/brw_tex_layout.c| 51 > +-- > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 10 ++ > 2 files changed, 50 insertions(+), 11 deletions(-) > Since "horizontal slice pitch" and "vertical slice pitch" are not defined in the bspec, these funtions should have documentation explaining what they mean. Correct me if I'm wrong, but I think what you mean by "horizontal slice pitch" is: - For a 3D texture, the horizontal spacing between slices at a given miplevel. - For any other texture, not used. And by "vertical slice pitch" - For a 3D texture, the vertical spacing between each row of slices. - For any other texture, the vertical spacing between array slices. With additional comments added, this patch is: Reviewed-by: Paul Berry ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2 v3] i965: Extend fast texture upload
The patches look good to me, and I verified that this caused no Piglit regressions on Ivybridge when applied to master-8ab47b4. Reviewed-by: Chad Versace I committed them to master. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 73174] New: Account request
https://bugs.freedesktop.org/show_bug.cgi?id=73174 Priority: medium Bug ID: 73174 Assignee: mesa-dev@lists.freedesktop.org Summary: Account request Severity: normal Classification: Unclassified OS: All Reporter: j...@lunarg.com Hardware: Other Status: NEW Version: unspecified Component: Other Product: Mesa Created attachment 91352 --> https://bugs.freedesktop.org/attachment.cgi?id=91352&action=edit SSH public key and gpg key Open freedesktop.org acccount -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 73174] Account request
https://bugs.freedesktop.org/show_bug.cgi?id=73174 --- Comment #1 from Jon Ashburn --- Jon Ashburn j...@lunarg.com Preferred account name: jashburn -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2 v3] i965: Extend fast texture upload
Awesome! Thanks Chad. Courtney On Mon, Dec 30, 2013 at 4:02 PM, Chad Versace wrote: > The patches look good to me, and I verified that this caused no Piglit > regressions on Ivybridge when applied to master-8ab47b4. > > Reviewed-by: Chad Versace > > I committed them to master. > -- Courtney Goeltzenleuchter LunarG ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 70591] glxext.h:275: error: redefinition of typedef ‘GLXContextID’
https://bugs.freedesktop.org/show_bug.cgi?id=70591 --- Comment #4 from Vinson Lee --- mesa: 5a51c1b01a16d3256f9769a76d8293fea5853b1f (master) The build error appears to be compiler dependent. I can reproduce the build error with gcc 4.4 or clang 2.8 on CentOS 6. I can also reproduce the build error with gcc-4.4, but not with gcc 4.8, on Ubuntu 13.10. $ ./autogen.sh --disable-dri3 --with-dri-drivers=swrast --with-gallium-drivers= $ make CC clientattrib.lo In file included from ../../include/GL/glx.h:333, from glxclient.h:45, from clientattrib.c:32: ../../include/GL/glxext.h:275: error: redefinition of typedef ‘GLXContextID’ ../../include/GL/glx.h:171: note: previous declaration of ‘GLXContextID’ was here -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/25] i965: Define and initialize image meta-data structure.
On 2 December 2013 11:39, Francisco Jerez wrote:
> This will be used to pass image information to the shader when we
> cannot use typed surface reads and writes. All entries except
> surface_idx and size are otherwise unused and will get eliminated by
> the uniform packing pass. size will be used for bounds checking with
> some image formats and will be useful for ARB_shader_image_size too.
> surface_idx is always used.
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 42 +
> src/mesa/drivers/dri/i965/brw_program.c | 5 ++
> src/mesa/drivers/dri/i965/brw_vec4_gs.c | 4 ++
> src/mesa/drivers/dri/i965/brw_vs.c| 7 ++-
> src/mesa/drivers/dri/i965/brw_wm.c| 7 ++-
> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 76
> +++
> 6 files changed, 138 insertions(+), 3 deletions(-)
>
There's something going on in this patch that seems really dodgy to me. I
haven't yet figured out whether it will lead to user-visible bugs, but it
breaks a key assumption that we make elsewhere in the driver. That
assumption is: the data contained in brw_stage_prog_data (and transitively
contained in other data structures owned by brw_stage_prog_data) is
initialized at the time the shader is compiled, and is thereafter
constant. The only circumstance in which it's ok for brw_stage_prog_data
to point to data that gets modified later is if that data is owned by
another data structure (this is what we do for uniforms, and it's the
reason why the types of brw_stage_prog_data::param and
brw_stage_prog_data::pull_param are "const float **". What's going on is
that brw_stage_prog_data points to an array of const float *'s, which it
owns, and then those pointers point to the current values of the uniforms,
which are owned by core mesa.
Your patch introduces data that is owned by brw_stage_prog_data (in the
sense that it gets deleted when brw_stage_prog_data gets deleted), but
which is not initialized at shader compilation time; instead it is
initialized at the time of brw_upload_image_surfaces(). Additionally,
pointers in brw_stage_prog_data::param and brw_stage_prog_data::pull_param
point to these values. That leads to an unexpected asymmetry--push and
pull parameters that are image parameters are owned by brw_stage_prog_data,
and pointed to in two separate ways, whereas all other push and pull
parameters are owned by other data structures.
Another problem is that brw_stage_prog_data_compare() compares the contents
of the image data. However, brw_stage_prog_data_compare() is called just
after compilation, to see if the program that has just been compiled
matches a program that was previously compiled; if so we can re-use the
previous program. (This allows us to avoid inefficiency in the case where
the we thought we had to do a state-dependent recompile, but once the
recompile completes we have the same program we had before). Since the
data isn't initialized until brw_upload_image_surfaces(), your patch will
cause brw_stage-prog_data_compare() to compare initialized data with
uninitialized data, defeating this optimization.
What I think we should do instead is have a fixed array of BRW_MAX_IMAGES
brw_image_param objects in brw_stage_state. brw_upload_image_surfaces()
already has a pointer to the brw_stage_state, so it can just refer to this
directly when calling brw->vtbl.update_image_surface(). And
backend_visitor can easily be modified to have a pointer to the
brw_stage_state as well, so backend_visitor::setup_image_uniform_values()
can use that to find the brw_image_param objects. That way we can remove
the brw_image_param pointer from brw_stage_prog_data, and then image params
will work exactly the same way that all other pull/push constants work.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 0816912..dc606c0f 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -352,6 +352,7 @@ struct brw_stage_prog_data {
>
> GLuint nr_params; /**< number of float params/constants */
> GLuint nr_pull_params;
> + GLuint nr_image_params;
>
> /* Pointers to tracked values (only valid once
> * _mesa_load_state_parameters has been called at runtime).
> @@ -361,6 +362,47 @@ struct brw_stage_prog_data {
> */
> const float **param;
> const float **pull_param;
> + struct brw_image_param *image_param;
> +};
> +
> +/*
> + * Image meta-data structure as laid out in the shader parameter
> + * buffer. Entries have to be 16B-aligned for the vec4 back-end to be
> + * able to use them. That's okay because the padding and any unused
> + * entries [most of them except when we're doing untyped surface
> + * access] will be removed by the uniform packing pass.
> + */
> +#define BRW_IMAGE_PARAM_SURFACE_IDX_OFFSET 0
> +#define BRW_IMAGE_PARAM_OFFSET_OFFSET 4
> +#define BRW_IMAGE_PA
Re: [Mesa-dev] [PATCH 06/25] i965: Hook up image state upload.
On 2 December 2013 11:39, Francisco Jerez wrote:
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 2 +
> src/mesa/drivers/dri/i965/brw_gs_surface_state.c | 24
> src/mesa/drivers/dri/i965/brw_state.h| 3 ++
> src/mesa/drivers/dri/i965/brw_state_upload.c | 6 +++
> src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 24
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 50
>
> 6 files changed, 109 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index dc606c0f..4586005 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -181,6 +181,7 @@ enum brw_state_id {
> BRW_STATE_STATS_WM,
> BRW_STATE_UNIFORM_BUFFER,
> BRW_STATE_ATOMIC_BUFFER,
> + BRW_STATE_IMAGE_UNITS,
> BRW_STATE_META_IN_PROGRESS,
> BRW_STATE_INTERPOLATION_MAP,
> BRW_STATE_PUSH_CONSTANT_ALLOCATION,
> @@ -220,6 +221,7 @@ enum brw_state_id {
> #define BRW_NEW_STATS_WM (1 << BRW_STATE_STATS_WM)
> #define BRW_NEW_UNIFORM_BUFFER (1 << BRW_STATE_UNIFORM_BUFFER)
> #define BRW_NEW_ATOMIC_BUFFER (1 << BRW_STATE_ATOMIC_BUFFER)
> +#define BRW_NEW_IMAGE_UNITS (1 << BRW_STATE_IMAGE_UNITS)
> #define BRW_NEW_META_IN_PROGRESS(1 << BRW_STATE_META_IN_PROGRESS)
> #define BRW_NEW_INTERPOLATION_MAP (1 << BRW_STATE_INTERPOLATION_MAP)
> #define BRW_NEW_PUSH_CONSTANT_ALLOCATION (1 <<
> BRW_STATE_PUSH_CONSTANT_ALLOCATION)
> diff --git a/src/mesa/drivers/dri/i965/brw_gs_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_gs_surface_state.c
> index 5661941..6db061d 100644
> --- a/src/mesa/drivers/dri/i965/brw_gs_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_gs_surface_state.c
> @@ -110,3 +110,27 @@ const struct brw_tracked_state brw_gs_abo_surfaces = {
> },
> .emit = brw_upload_gs_abo_surfaces,
> };
> +
> +static void
> +brw_upload_gs_image_surfaces(struct brw_context *brw)
> +{
> + struct gl_context *ctx = &brw->ctx;
> + /* _NEW_PROGRAM */
> + struct gl_shader_program *prog = ctx->Shader.CurrentGeometryProgram;
>
Since you only need the geometry program here, you can depend on
BRW_NEW_GEOMETRY_PROGRAM instead of _NEW_PROGRAM, and avoid this function
getting called unnecessarily when the geometry shader doesn't change (e.g.
when switching between two programs that don't contain a geometry shader).
A similar improvement cound be made in the vs and fs cases, but the benefit
is less (since nearly all programs contain a vertex and a fragment shader).
It's not a huge deal, though. Either way, the patch is:
Reviewed-by: Paul Berry
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72619] [llvmpipe] piglit copyteximage 2D regression
https://bugs.freedesktop.org/show_bug.cgi?id=72619 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #4 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72659] [llvmpipe] piglit getteximage-formats init-by-rendering regression
https://bugs.freedesktop.org/show_bug.cgi?id=72659 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72658] [llvmpipe] piglit copyteximage RECT regression
https://bugs.freedesktop.org/show_bug.cgi?id=72658 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72656] [llvmpipe] piglit arb_color_buffer_float-render GL_RGBA16F sanity fog regression
https://bugs.freedesktop.org/show_bug.cgi?id=72656 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72657] [llvmpipe] piglit copyteximage CUBE regression
https://bugs.freedesktop.org/show_bug.cgi?id=72657 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 72655] [llvmpipe] piglit arb_color_buffer_float-render GL_RGBA16F sanity regression
https://bugs.freedesktop.org/show_bug.cgi?id=72655 Vinson Lee changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Vinson Lee --- commit 27d47bd42f417db96842c9453092acf68944a4c8 Author: Roland Scheidegger Date: Fri Dec 13 21:20:05 2013 +0100 gallivm: fix pointer type for stmxcsr/ldmxcsr The argument is a i8 pointer not a i32 pointer (even though the value actually stored/loaded IS i32). Older llvm versions didn't care but 3.2 and newer do leading to crashes. Reviewed-by: Zack Rusin -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
