[Mesa-dev] [RFC 2/4] glsl IR: only allow optimization of interstage variable
GL_ARB_separate_shader_objects allow to match by name variable or block
interface. Input varying can't be removed because it is will impact the
location assignment.
It fixes the bug 79783 and likely any application that uses
GL_ARB_separate_shader_objects extension.
piglit test: arb_separate_shader_object-rendezvous_by_name
Signed-off-by: Gregory Hainaut
---
src/glsl/opt_dead_code.cpp | 18 ++
1 file changed, 18 insertions(+)
diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
index 071485a..37329f6 100644
--- a/src/glsl/opt_dead_code.cpp
+++ b/src/glsl/opt_dead_code.cpp
@@ -75,6 +75,24 @@ do_dead_code(exec_list *instructions, bool
uniform_locations_assigned)
|| !entry->declaration)
continue;
+ /* Section 7.4.1 (Shader Interface Matching) of the OpenGL 4.5
+ * (Core Profile) spec says:
+ *
+ *"With separable program objects, interfaces between shader
+ *stages may involve the outputs from one program object and the
+ *inputs from a second program object. For such interfaces, it is
+ *not possible to detect mismatches at link time, because the
+ *programs are linked separately. When each such program is
+ *linked, all inputs or outputs interfacing with another program
+ *stage are treated as active."
+ */
+ if (entry->var->data.always_active_io &&
+(!entry->var->data.explicit_location ||
+ entry->var->data.location >= VARYING_SLOT_VAR0) &&
+(entry->var->data.mode == ir_var_shader_in ||
+ entry->var->data.mode == ir_var_shader_out))
+ continue;
+
if (entry->assign) {
/* Remove a single dead assignment to the variable we found.
* Don't do so if it's a shader or function output or a shader
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 0/4] V3: Improve GLSL support of GL_ARB_separate_shader_objects
New version that improves previous code quality and fixes several outstanding issues. Piglit wasn't run yet. v3: Squash commit 1&2 * Use a better name for the new attribute: always_active_io * Use ir_variable directly instead of ir_variable_refcount_visitor * Put related code in linker.cpp Add 2 new commits to fix wrong interface matching in more complex case. Commit 3: avoid collision between user and linker slot assignment Commit 4: avoid unpredictable sorting of varying Commit 1/2/3 fix the piglit test: arb_separate_shader_object-rendezvous_by_name posted on piglit ML Commit 4 was tested on the PCSX2 application. I need to implement a new test. Gregory Hainaut (4): glsl IR: add always_active_io attribute to ir_variable glsl IR: only allow optimization of interstage variable glsl: avoid linker and user varying location to overlap glsl: don't sort varying in separate shader mode src/glsl/ir.cpp| 1 + src/glsl/ir.h | 7 + src/glsl/link_varyings.cpp | 76 ++ src/glsl/linker.cpp| 73 src/glsl/opt_dead_code.cpp | 18 +++ 5 files changed, 169 insertions(+), 6 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 3/4] glsl: avoid linker and user varying location to overlap
Current behavior on the interface matching:
layout (location = 0) out0; // Assigned to VARYING_SLOT_VAR0 by user
out1; // Assigned to VARYING_SLOT_VAR0 by the linker
New behavior on the interface matching:
layout (location = 0) out0; // Assigned to VARYING_SLOT_VAR0 by user
out1; // Assigned to VARYING_SLOT_VAR1 by the linker
piglit: arb_separate_shader_object-rendezvous_by_name
Signed-off-by: Gregory Hainaut
---
src/glsl/link_varyings.cpp | 46 +++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index 7e77a67..6d6e9b5 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -766,7 +766,7 @@ public:
gl_shader_stage consumer_stage);
~varying_matches();
void record(ir_variable *producer_var, ir_variable *consumer_var);
- unsigned assign_locations();
+ unsigned assign_locations(uint64_t reserved_slots);
void store_locations() const;
private:
@@ -986,7 +986,7 @@ varying_matches::record(ir_variable *producer_var,
ir_variable *consumer_var)
* passed to varying_matches::record().
*/
unsigned
-varying_matches::assign_locations()
+varying_matches::assign_locations(uint64_t reserved_slots)
{
/* Sort varying matches into an order that makes them easy to pack. */
qsort(this->matches, this->num_matches, sizeof(*this->matches),
@@ -1013,6 +1013,10 @@ varying_matches::assign_locations()
!= this->matches[i].packing_class) {
*location = ALIGN(*location, 4);
}
+ while ((*location < MAX_VARYING * 4u) &&
+(reserved_slots & (1u << *location / 4u))) {
+ *location = ALIGN(*location + 1, 4);
+ }
this->matches[i].generic_location = *location;
@@ -1376,6 +1380,38 @@ canonicalize_shader_io(exec_list *ir, enum
ir_variable_mode io_mode)
}
/**
+ * Generate a bitfield map of the already reserved slots for a shader.
+ *
+ * In theory a 32 bits value will be enough but a 64 bits value is future
proof.
+ */
+uint64_t
+reserved_varying_slot(struct gl_shader *stage, ir_variable_mode io_mode)
+{
+ assert(mode == ir_var_shader_in || mode == ir_var_shader_out);
+ assert(MAX_VARYING <= 64); /* avoid an overflow of the returned value */
+
+ uint64_t slots = 0;
+ int var_slot;
+
+ if (!stage)
+ return slots;
+
+ foreach_in_list(ir_instruction, node, stage->ir) {
+ ir_variable *const var = node->as_variable();
+
+ if (var == NULL || var->data.mode != io_mode ||
!var->data.explicit_location)
+ continue;
+
+ var_slot = var->data.location - VARYING_SLOT_VAR0;
+ if (var_slot >= 0 && var_slot < MAX_VARYING)
+ slots |= 1u << var_slot;
+ }
+
+ return slots;
+}
+
+
+/**
* Assign locations for all variables that are produced in one pipeline stage
* (the "producer") and consumed in the next stage (the "consumer").
*
@@ -1550,7 +1586,11 @@ assign_varying_locations(struct gl_context *ctx,
matches.record(matched_candidate->toplevel_var, NULL);
}
- const unsigned slots_used = matches.assign_locations();
+ const uint64_t reserved_slots =
+ reserved_varying_slot(producer, ir_var_shader_out) |
+ reserved_varying_slot(consumer, ir_var_shader_in);
+
+ const unsigned slots_used = matches.assign_locations(reserved_slots);
matches.store_locations();
for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 4/4] glsl: don't sort varying in separate shader mode
Current issue is the addition of FLAT qualifier on varying_matches::record()
which break the varying expected order
Future issue is the removal of the interpolation qualifier matching constrain
In my humble opinion, it is the responsability of the GL developer to optimize
their slots assignment in SSO with the help of GL_ARB_enhanced_layouts
Signed-off-by: Gregory Hainaut
---
src/glsl/link_varyings.cpp | 36 ++--
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index 6d6e9b5..31efee7 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -766,7 +766,7 @@ public:
gl_shader_stage consumer_stage);
~varying_matches();
void record(ir_variable *producer_var, ir_variable *consumer_var);
- unsigned assign_locations(uint64_t reserved_slots);
+ unsigned assign_locations(uint64_t reserved_slots, bool separate_shader);
void store_locations() const;
private:
@@ -986,11 +986,34 @@ varying_matches::record(ir_variable *producer_var,
ir_variable *consumer_var)
* passed to varying_matches::record().
*/
unsigned
-varying_matches::assign_locations(uint64_t reserved_slots)
+varying_matches::assign_locations(uint64_t reserved_slots, bool
separate_shader)
{
- /* Sort varying matches into an order that makes them easy to pack. */
- qsort(this->matches, this->num_matches, sizeof(*this->matches),
- &varying_matches::match_comparator);
+ /* Disable the varying sorting for separate shader program
+* 1/ All programs must sort the code in the same order to guarantee the
+*interface matching. However varying_matches::record() will change the
+*interpolation qualifier of some stages.
+*
+* 2/ GLSL version 4.50 removes the matching constrain on the interpolation
+*qualifier.
+*
+* Chapter 4.5 of GLSL 4.40:
+*"The type and presence of interpolation qualifiers of variables with
+*the same name declared in all linked shaders for the same cross-stage
+*interface must match, otherwise the link command will fail.
+*
+*When comparing an output from one stage to an input of a subsequent
+*stage, the input and output don't match if their interpolation
+*qualifiers (or lack thereof) are not the same."
+*
+* Chapter 4.5 of GLSL 4.50:
+*"It is a link-time error if, within the same stage, the interpolation
+*qualifiers of variables of the same name do not match."
+*/
+ if (!separate_shader) {
+ /* Sort varying matches into an order that makes them easy to pack. */
+ qsort(this->matches, this->num_matches, sizeof(*this->matches),
+&varying_matches::match_comparator);
+ }
unsigned generic_location = 0;
unsigned generic_patch_location = MAX_VARYING*4;
@@ -1590,7 +1613,8 @@ assign_varying_locations(struct gl_context *ctx,
reserved_varying_slot(producer, ir_var_shader_out) |
reserved_varying_slot(consumer, ir_var_shader_in);
- const unsigned slots_used = matches.assign_locations(reserved_slots);
+ const unsigned slots_used = matches.assign_locations(reserved_slots,
+prog->SeparateShader);
matches.store_locations();
for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/4] glsl IR: add always_active_io attribute to ir_variable
The value will be set in separate-shader program when an input/output
must remains active (i.e. deadcode removal isn't allowed because it will create
interface location/name-matching mismatch)
v3:
* Rename the attribute
* Use ir_variable directly instead of ir_variable_refcount_visitor
* Move the foreach IR code in the linker file
Signed-off-by: Gregory Hainaut
---
src/glsl/ir.cpp | 1 +
src/glsl/ir.h | 7 +
src/glsl/linker.cpp | 73 +
3 files changed, 81 insertions(+)
diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
index 2c45b9e..f3f7355 100644
--- a/src/glsl/ir.cpp
+++ b/src/glsl/ir.cpp
@@ -1650,6 +1650,7 @@ ir_variable::ir_variable(const struct glsl_type *type,
const char *name,
this->data.pixel_center_integer = false;
this->data.depth_layout = ir_depth_layout_none;
this->data.used = false;
+ this->data.always_active_io = false;
this->data.read_only = false;
this->data.centroid = false;
this->data.sample = false;
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index 43a2bf0..a91add9 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -658,6 +658,13 @@ public:
unsigned assigned:1;
/**
+ * When separate shader programs are enabled, only interstage
+ * variables can be safely removed of the shader interface. Others
+ * input/output must remains active.
+ */
+ unsigned always_active_io:1;
+
+ /**
* Enum indicating how the variable was declared. See
* ir_var_declaration_type.
*
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index a97b4ef..3a30d0c 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -3599,6 +3599,70 @@ link_assign_subroutine_types(struct gl_shader_program
*prog)
}
}
+static void
+ir_set_always_active_io(exec_list *ir, ir_variable_mode io_mode)
+{
+ assert(mode == ir_var_shader_in || mode == ir_var_shader_out);
+
+ foreach_in_list(ir_instruction, node, ir) {
+ ir_variable *const var = node->as_variable();
+
+ if (var == NULL || var->data.mode != io_mode)
+ continue;
+
+ var->data.always_active_io = true;
+ }
+}
+
+static void
+set_always_active_io(struct gl_shader_program *prog)
+{
+ unsigned first, last;
+ assert(prog->SeparateShader);
+
+ first = MESA_SHADER_STAGES;
+ last = 0;
+
+ /* Determine first and last stage. Excluding the compute stage */
+ for (unsigned i = 0; i < MESA_SHADER_COMPUTE; i++) {
+ if (!prog->_LinkedShaders[i])
+ continue;
+ if (first == MESA_SHADER_STAGES)
+ first = i;
+ last = i;
+ }
+
+ if (first == MESA_SHADER_STAGES)
+ return;
+
+ for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
+ gl_shader *sh = prog->_LinkedShaders[stage];
+ if (!sh)
+ continue;
+
+ if (first == last) {
+ /* Single shader program: allowed inactive variable
+ * 1/ input of the VS
+ * 2/ output of the FS
+ */
+ if (stage != MESA_SHADER_VERTEX)
+ir_set_always_active_io(sh->ir, ir_var_shader_in);
+ if (stage != MESA_SHADER_FRAGMENT)
+ir_set_always_active_io(sh->ir, ir_var_shader_out);
+ } else {
+ /* Multiple shaders program: allowed inactive variable
+ * 1/ input of the VS
+ * 2/ output of the FS
+ * 3/ interstage variables
+ */
+ if (stage == first && stage != MESA_SHADER_VERTEX)
+ir_set_always_active_io(sh->ir, ir_var_shader_in);
+ else if (stage == last && stage != MESA_SHADER_FRAGMENT)
+ir_set_always_active_io(sh->ir, ir_var_shader_out);
+ }
+ }
+}
+
void
link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
{
@@ -3858,6 +3922,15 @@ link_shaders(struct gl_context *ctx, struct
gl_shader_program *prog)
}
}
+
+ /**
+* When separate shader programs are enabled, only interstage
+* variables can be safely removed of the shader interface. Others
+* input/output must remains active.
+*/
+ if (prog->SeparateShader)
+ set_always_active_io(prog);
+
if (!interstage_cross_validate_uniform_blocks(prog))
goto done;
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 2/4] glsl IR: only allow optimization of interstage variable
GL_ARB_separate_shader_objects allow to match by name variable or block
interface. Input varying can't be removed because it is will impact the
location assignment.
It fixes the bug 79783 and likely any application that uses
GL_ARB_separate_shader_objects extension.
piglit test: arb_separate_shader_object-rendezvous_by_name
Signed-off-by: Gregory Hainaut
---
src/glsl/opt_dead_code.cpp | 18 ++
1 file changed, 18 insertions(+)
diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
index c5be166..fef5e92 100644
--- a/src/glsl/opt_dead_code.cpp
+++ b/src/glsl/opt_dead_code.cpp
@@ -75,6 +75,24 @@ do_dead_code(exec_list *instructions, bool
uniform_locations_assigned)
|| !entry->declaration)
continue;
+ /* Section 7.4.1 (Shader Interface Matching) of the OpenGL 4.5
+ * (Core Profile) spec says:
+ *
+ *"With separable program objects, interfaces between shader
+ *stages may involve the outputs from one program object and the
+ *inputs from a second program object. For such interfaces, it is
+ *not possible to detect mismatches at link time, because the
+ *programs are linked separately. When each such program is
+ *linked, all inputs or outputs interfacing with another program
+ *stage are treated as active."
+ */
+ if (entry->var->data.always_active_io &&
+(!entry->var->data.explicit_location ||
+ entry->var->data.location >= VARYING_SLOT_VAR0) &&
+(entry->var->data.mode == ir_var_shader_in ||
+ entry->var->data.mode == ir_var_shader_out))
+ continue;
+
if (!entry->assign_list.is_empty()) {
/* Remove all the dead assignments to the variable we found.
* Don't do so if it's a shader or function output, though.
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/4] glsl IR: add always_active_io attribute to ir_variable
The value will be set in separate-shader program when an input/output
must remains active (i.e. deadcode removal isn't allowed because it will create
interface location/name-matching mismatch)
v3:
* Rename the attribute
* Use ir_variable directly instead of ir_variable_refcount_visitor
* Move the foreach IR code in the linker file
v4:
* Fix variable name in assert
Signed-off-by: Gregory Hainaut
---
src/glsl/ir.cpp | 1 +
src/glsl/ir.h | 7 +
src/glsl/linker.cpp | 73 +
3 files changed, 81 insertions(+)
diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
index 8933b23..0c1430b 100644
--- a/src/glsl/ir.cpp
+++ b/src/glsl/ir.cpp
@@ -1666,6 +1666,7 @@ ir_variable::ir_variable(const struct glsl_type *type,
const char *name,
this->data.pixel_center_integer = false;
this->data.depth_layout = ir_depth_layout_none;
this->data.used = false;
+ this->data.always_active_io = false;
this->data.read_only = false;
this->data.centroid = false;
this->data.sample = false;
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index 9c9f22d..0223665 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -658,6 +658,13 @@ public:
unsigned assigned:1;
/**
+ * When separate shader programs are enabled, only interstage
+ * variables can be safely removed of the shader interface. Others
+ * input/output must remains active.
+ */
+ unsigned always_active_io:1;
+
+ /**
* Enum indicating how the variable was declared. See
* ir_var_declaration_type.
*
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 424b92a..845 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -3896,6 +3896,70 @@ split_ubos_and_ssbos(void *mem_ctx,
assert(*num_ubos + *num_ssbos == num_blocks);
}
+static void
+ir_set_always_active_io(exec_list *ir, ir_variable_mode io_mode)
+{
+ assert(io_mode == ir_var_shader_in || io_mode == ir_var_shader_out);
+
+ foreach_in_list(ir_instruction, node, ir) {
+ ir_variable *const var = node->as_variable();
+
+ if (var == NULL || var->data.mode != io_mode)
+ continue;
+
+ var->data.always_active_io = true;
+ }
+}
+
+static void
+set_always_active_io(struct gl_shader_program *prog)
+{
+ unsigned first, last;
+ assert(prog->SeparateShader);
+
+ first = MESA_SHADER_STAGES;
+ last = 0;
+
+ /* Determine first and last stage. Excluding the compute stage */
+ for (unsigned i = 0; i < MESA_SHADER_COMPUTE; i++) {
+ if (!prog->_LinkedShaders[i])
+ continue;
+ if (first == MESA_SHADER_STAGES)
+ first = i;
+ last = i;
+ }
+
+ if (first == MESA_SHADER_STAGES)
+ return;
+
+ for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
+ gl_shader *sh = prog->_LinkedShaders[stage];
+ if (!sh)
+ continue;
+
+ if (first == last) {
+ /* Single shader program: allowed inactive variable
+ * 1/ input of the VS
+ * 2/ output of the FS
+ */
+ if (stage != MESA_SHADER_VERTEX)
+ir_set_always_active_io(sh->ir, ir_var_shader_in);
+ if (stage != MESA_SHADER_FRAGMENT)
+ir_set_always_active_io(sh->ir, ir_var_shader_out);
+ } else {
+ /* Multiple shaders program: allowed inactive variable
+ * 1/ input of the VS
+ * 2/ output of the FS
+ * 3/ interstage variables
+ */
+ if (stage == first && stage != MESA_SHADER_VERTEX)
+ir_set_always_active_io(sh->ir, ir_var_shader_in);
+ else if (stage == last && stage != MESA_SHADER_FRAGMENT)
+ir_set_always_active_io(sh->ir, ir_var_shader_out);
+ }
+ }
+}
+
void
link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
{
@@ -4155,6 +4219,15 @@ link_shaders(struct gl_context *ctx, struct
gl_shader_program *prog)
}
}
+
+ /**
+* When separate shader programs are enabled, only interstage
+* variables can be safely removed of the shader interface. Others
+* input/output must remains active.
+*/
+ if (prog->SeparateShader)
+ set_always_active_io(prog);
+
if (!interstage_cross_validate_uniform_blocks(prog))
goto done;
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 0/4] V4: Improve GLSL support of GL_ARB_separate_shader_objects
v4: Rebase against lastest master and fix bad variable name in assert. A new test was developed to properly check commit 4 behavior. I ran most of the piglit test without regression. v3: Squash old commit 1&2 * Use a better name for the new attribute: always_active_io * Use ir_variable directly instead of ir_variable_refcount_visitor * Put related code in linker.cpp Add 2 new commits to fix wrong interface matching in more complex case. Commit 3: avoid collision between user and linker slot assignment Commit 4: avoid unpredictable sorting of varying Commit 1/2/3 fix the piglit test: arb_separate_shader_object-rendezvous_by_name posted on piglit ML Commit 4 was tested on the PCSX2 application. Gregory Hainaut (4): glsl IR: add always_active_io attribute to ir_variable glsl IR: only allow optimization of interstage variable glsl: avoid linker and user varying location to overlap glsl: don't sort varying in separate shader mode src/glsl/ir.cpp| 1 + src/glsl/ir.h | 7 + src/glsl/link_varyings.cpp | 76 ++ src/glsl/linker.cpp| 73 src/glsl/opt_dead_code.cpp | 18 +++ 5 files changed, 169 insertions(+), 6 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 3/4] glsl: avoid linker and user varying location to overlap
Current behavior on the interface matching:
layout (location = 0) out0; // Assigned to VARYING_SLOT_VAR0 by user
out1; // Assigned to VARYING_SLOT_VAR0 by the linker
New behavior on the interface matching:
layout (location = 0) out0; // Assigned to VARYING_SLOT_VAR0 by user
out1; // Assigned to VARYING_SLOT_VAR1 by the linker
piglit: arb_separate_shader_object-rendezvous_by_name
v4:
* Fix variable name in assert
Signed-off-by: Gregory Hainaut
---
src/glsl/link_varyings.cpp | 46 +++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index 7e77a67..67d04cb 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -766,7 +766,7 @@ public:
gl_shader_stage consumer_stage);
~varying_matches();
void record(ir_variable *producer_var, ir_variable *consumer_var);
- unsigned assign_locations();
+ unsigned assign_locations(uint64_t reserved_slots);
void store_locations() const;
private:
@@ -986,7 +986,7 @@ varying_matches::record(ir_variable *producer_var,
ir_variable *consumer_var)
* passed to varying_matches::record().
*/
unsigned
-varying_matches::assign_locations()
+varying_matches::assign_locations(uint64_t reserved_slots)
{
/* Sort varying matches into an order that makes them easy to pack. */
qsort(this->matches, this->num_matches, sizeof(*this->matches),
@@ -1013,6 +1013,10 @@ varying_matches::assign_locations()
!= this->matches[i].packing_class) {
*location = ALIGN(*location, 4);
}
+ while ((*location < MAX_VARYING * 4u) &&
+(reserved_slots & (1u << *location / 4u))) {
+ *location = ALIGN(*location + 1, 4);
+ }
this->matches[i].generic_location = *location;
@@ -1376,6 +1380,38 @@ canonicalize_shader_io(exec_list *ir, enum
ir_variable_mode io_mode)
}
/**
+ * Generate a bitfield map of the already reserved slots for a shader.
+ *
+ * In theory a 32 bits value will be enough but a 64 bits value is future
proof.
+ */
+uint64_t
+reserved_varying_slot(struct gl_shader *stage, ir_variable_mode io_mode)
+{
+ assert(io_mode == ir_var_shader_in || io_mode == ir_var_shader_out);
+ assert(MAX_VARYING <= 64); /* avoid an overflow of the returned value */
+
+ uint64_t slots = 0;
+ int var_slot;
+
+ if (!stage)
+ return slots;
+
+ foreach_in_list(ir_instruction, node, stage->ir) {
+ ir_variable *const var = node->as_variable();
+
+ if (var == NULL || var->data.mode != io_mode ||
!var->data.explicit_location)
+ continue;
+
+ var_slot = var->data.location - VARYING_SLOT_VAR0;
+ if (var_slot >= 0 && var_slot < MAX_VARYING)
+ slots |= 1u << var_slot;
+ }
+
+ return slots;
+}
+
+
+/**
* Assign locations for all variables that are produced in one pipeline stage
* (the "producer") and consumed in the next stage (the "consumer").
*
@@ -1550,7 +1586,11 @@ assign_varying_locations(struct gl_context *ctx,
matches.record(matched_candidate->toplevel_var, NULL);
}
- const unsigned slots_used = matches.assign_locations();
+ const uint64_t reserved_slots =
+ reserved_varying_slot(producer, ir_var_shader_out) |
+ reserved_varying_slot(consumer, ir_var_shader_in);
+
+ const unsigned slots_used = matches.assign_locations(reserved_slots);
matches.store_locations();
for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 4/4] glsl: don't sort varying in separate shader mode
Current issue is the addition of FLAT qualifier on varying_matches::record()
which break the varying expected order
Future issue is the removal of the interpolation qualifier matching constrain
In my humble opinion, it is the responsability of the GL developer to optimize
their slots assignment in SSO with the help of GL_ARB_enhanced_layouts
Signed-off-by: Gregory Hainaut
---
src/glsl/link_varyings.cpp | 36 ++--
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index 67d04cb..f1794c3 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -766,7 +766,7 @@ public:
gl_shader_stage consumer_stage);
~varying_matches();
void record(ir_variable *producer_var, ir_variable *consumer_var);
- unsigned assign_locations(uint64_t reserved_slots);
+ unsigned assign_locations(uint64_t reserved_slots, bool separate_shader);
void store_locations() const;
private:
@@ -986,11 +986,34 @@ varying_matches::record(ir_variable *producer_var,
ir_variable *consumer_var)
* passed to varying_matches::record().
*/
unsigned
-varying_matches::assign_locations(uint64_t reserved_slots)
+varying_matches::assign_locations(uint64_t reserved_slots, bool
separate_shader)
{
- /* Sort varying matches into an order that makes them easy to pack. */
- qsort(this->matches, this->num_matches, sizeof(*this->matches),
- &varying_matches::match_comparator);
+ /* Disable the varying sorting for separate shader program
+* 1/ All programs must sort the code in the same order to guarantee the
+*interface matching. However varying_matches::record() will change the
+*interpolation qualifier of some stages.
+*
+* 2/ GLSL version 4.50 removes the matching constrain on the interpolation
+*qualifier.
+*
+* Chapter 4.5 of GLSL 4.40:
+*"The type and presence of interpolation qualifiers of variables with
+*the same name declared in all linked shaders for the same cross-stage
+*interface must match, otherwise the link command will fail.
+*
+*When comparing an output from one stage to an input of a subsequent
+*stage, the input and output don't match if their interpolation
+*qualifiers (or lack thereof) are not the same."
+*
+* Chapter 4.5 of GLSL 4.50:
+*"It is a link-time error if, within the same stage, the interpolation
+*qualifiers of variables of the same name do not match."
+*/
+ if (!separate_shader) {
+ /* Sort varying matches into an order that makes them easy to pack. */
+ qsort(this->matches, this->num_matches, sizeof(*this->matches),
+&varying_matches::match_comparator);
+ }
unsigned generic_location = 0;
unsigned generic_patch_location = MAX_VARYING*4;
@@ -1590,7 +1613,8 @@ assign_varying_locations(struct gl_context *ctx,
reserved_varying_slot(producer, ir_var_shader_out) |
reserved_varying_slot(consumer, ir_var_shader_in);
- const unsigned slots_used = matches.assign_locations(reserved_slots);
+ const unsigned slots_used = matches.assign_locations(reserved_slots,
+prog->SeparateShader);
matches.store_locations();
for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
Hello, > A note on synchronizations. Borderlands 2 has 170 thread syncs per > frame. That means the app thread has to stop and wait 170x per frame. > Despite that, it still has 70% higher performance in some cases. My > theory is that if you have a lot of draw calls, you can have a lot of > syncs, because the sheer amount of draw calls will just make those > syncs irrelevant. 200 syncs per 4k draw calls is like 1 sync per 20 > draw calls. Here a feedback of my quick test On PCSX2 (PS2 emulator), I noticed that synchronization badly impacts the perf. In my case, there are mostly related to texture transfer (CPU->GPU) and clear buffer functions. Strangely I didn't notice anything related to BufferSubData* but I guess it is the same. Those functions trigger a sync because of the pointer parameter. However texture transfer could use a PBO so it isn't a real pointer. And clear uses a pointer to a color hence a small payload (worst case is likely around 16/32B). IMHO, it can surely be inlined/memcpy in the gl dispatcher (otherwise the old GL2 clear API is sync free). I hacked the code to remove the sync on texture transfer and I got a major speed boost. I didn't count the number of draw call neither sync ratio. But I suspect that perf impact could depends on the sync repartition. Unlike me, I guess that Borderlands2 uploads/clears buffers/textures/uniform at the start of the frame. Which mean various small sync at the start of the frame (which might be optimized as a spin lock). Therefore the hot rendering loop might be sync free hence the speed boost. To conclude, based on my single testcase, current state of the code isn't yet optimal and it might explain why few apps see any perf improvement so far. But the potential is here. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
> > Hi, > > On Mon, 2017-02-06 at 13:43 +0100, Jan Ziak wrote: > >* Shadow of Mordor benchmark: 30 FPS w/o glthread -> 20 FPS with > *>* glthread > *> > For what it is worth, all the Feral games have a dispatch thread that > primarily calls GL functions. > > James > > Hello James, Did you have the opportunity to compare Feral's dispatcher threading implementation versus Nvidia's threading option. If yes, is there any performance difference? In other word, did you implement it because some (most) drivers miss this optimization or because it can really bring some performance improvements (when done carefully). Thanks you, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
> > >* Hello James, > *> > >* Did you have the opportunity to compare Feral's dispatcher threading > *>* implementation versus Nvidia's threading option. If yes, is there any > *>* performance difference? In other word, did you implement it because > *>* some (most) drivers miss this optimization or because it can really > *>* bring some performance improvements (when done carefully). > * > We can't directly compare to Nvidia's option, as our threaded GL > implementation is a necessity for us. OpenGL contexts must be bound to > a specific thread, unlike D3D devices, and we have games written for > D3D using multiple threads to dispatch graphics work (and sometimes > using multiple D3D devices from the same thread). We make one dispatch > thread for each GL context to avoid having to unbind the context from a > thread. There is some locking so that only one game thread may submit > to a context's dispatch thread at once, but that locking is very > lightweight compared to what would happen if we repeatedly attached and > detached GL contexts on game threads. GL_KHR_context_flush_control was > supposed to help with that, by allowing opting out of flushing the GL > context when unbinding it from a thread, but last time we tried that, > there were some synchronous X requests in SDL2 making it remain slow. > > Having said that, we did at one point have a path which used > GLX_MESA_multithreaded_make_current when it was available. We found the > GL dispatch threads was quicker on all the games we were working on at > the time. > > James > > Oh I see. I already got similar context threading issue when I ported my app from Dx to GL. Thanks for the full explanation (and for all games ported on Linux ^^). ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
> Regresses performance: > * Shadow of Mordor (with multiple game gfx settings & driver/HW?) > * PCSX2 emulator As Marek said the code still miss some optimization opportunities. FWIW, PCSX2 uses persistant PBO for texture transfer so it can be done without any sync. I removed the sync (with a hack), I won +25% on FPS and rendering was limited by the GPU (Nouveau). Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
Hello, I ran piglit (glthread false vs true) on the Marek branch and Nouveau driver. Of course we can still debate, if piglit is good enough to detect multi threading issue. I spot a small typo that generate ~400 fails (*mat2x4* uniform got 6 scalar instead of 8). piglit status with the typo patch : warn +2/ fail + 4/ crash +5 All regressions are on GLX tests but glsl-uniform-out-of-bounds. The latter reports a wrong error INVALID_VALUE (there is an extra glthread check because count is too big) instead of INVALID_OPERATION (not an array uniform must have a count of 1). I'm not sure it worth a fix. On GLX side, I can explain some crashes (didn't debug all neither fail) and I suspect there are related to the previous report. At the context creation a glthread dispatcher is created and the *TLS* variable u_current_table is set with the new gl marshal function. If deprecated non-VBO functions are detected (glBegin/glVertexPointer/... and others horror from the previous century), the code will disable glthread. 1/ thread will be deleted 2/ u_current_table will be set to the original value However u_current_table is local to a thread. So a single thread will get the correct dispatcher table. Remaining threads will keep the previous marshal thread. If a glthread function is called without the thread behind, then BOOM. I don't know how to fix it. However, is it really a necessity to care for such "old" application. Note: display list update also the table, I'm not sure of the impact. I don't know if piglit test display list Unfortunately it is way too late (at least for closed application, open source can easily be updated), but one could imagine a context flag to select a gl dispatcher. IMHO, the application is the best judge to enable it. Automatic detection on the driver is quite complicated. It is way cheaper to add an env value in the .desktop file. IMHO, it would be better to just drop the "automatic removal" of glthread. Old application will be slower if an user set wrongly an env variable which is more reasonable than a crash. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 14/26] mesa: Connect the generated GL command marshalling code to the build.
> >> > void
> >> > _mesa_glthread_init(struct gl_context *ctx)
> >> > {
> >> > struct glthread_state *glthread = calloc(1, sizeof(*glthread));
> >> >
> >> > if (!glthread)
> >> >return;
> >> >
> >> > + /* The marshalling dispatch table isn't integrated with the
> >> > Begin/End
> >> > +* dispatch table for desktop OpenGL, and the drawing functions
> >> > are
> >> > +* synchronous to support user vertex arrays on everything but
> >> > GL core
> >> > +* (even GLES 2/3) anyway, which means you'll end up with too
> >> > much overhead
> >> > +* from threading.
> >> > +*/
> >> > + if (ctx->API != API_OPENGL_CORE)
> >> > + return;
> >>
> >> Leaking glthread ?
> >>
> > This is odd, the version I have locally which I rebased from Marek's
> > git repo frees glthread.
> >
> > Marek, what's happened here? Are these patches different from the ones
> > in your git repo?
>
> No, they are the same. Maybe you fixed it locally?
>
> Marek
The code was removed patch 24.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
> > On 11/02/17 06:36 AM, gregory hainaut wrote: > > >* On GLX side, I can explain some crashes (didn't debug all neither fail) > *>* and I suspect there are related to the previous report. > *> >* At the context creation a glthread dispatcher is created and the *TLS* > *>* variable u_current_table is set with the new gl marshal function. > *>* If deprecated non-VBO functions are detected > *>* (glBegin/glVertexPointer/... and others horror from the previous > *>* century), the code will disable glthread. > *> >* 1/ thread will be deleted > *>* 2/ u_current_table will be set to the original value > *> >* However u_current_table is local to a thread. So a single thread will > *>* get the correct dispatcher table. Remaining threads will keep the > *>* previous marshal thread. If a glthread function is called without the > *>* thread behind, then BOOM. > * > If I understand correctly, this scenario can only happen if the > application takes advantage of the GLX_MESA_multithread_makecurrent > extension. I wonder if we shouldn't just stop advertising that extension > with glthread enabled. > > Actually, I wonder how well GLX_MESA_multithread_makecurrent works > regardless of glthread. I suspect the assumption of a 1:1 correspondence > between threads and contexts is quite widespread in the Mesa code. Case > in point, the piglit test glx at glx-multithread-texture > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev> has been failing > forever with radeonsi, though it passes with llvmpipe. Are there any > real world applications which require GLX_MESA_multithread_makecurrent, > or at least get a significant benefit from it? > > Hello, Yes you're right. I just found out yesterday that it was related to GLX_MESA_multithread_makecurrent. I did a small patch to not kill glthread when direct GL1 code is used. It avoid the crash but it is far from robust. You could imagine situation where 1 thread uses dispatcher function and another use std serial call (of course without any sync of glthread). Now if you use only modern GL (that will always use glthread), it might work. Note: Piglit is working as both thread will restore the standard dispatch function. Feral said that they tried the extension but a gl dispatcher was faster. Googling a bit I found Eric's initial mail. So it seems to be used. Extension isn't published on Kronos, so usage likely remain confidential. Potentially glthread will be as fast as GLX_MT on cairo. But you're right, I don't see the point to support both at the same time. > I'm happy now with the implementation of this extension, I've got > tests for most of the issues mentioned (the one that's really missing > is the bind-to-new-drawable one, but I don't anticipate issues with > it), and cairo-gl is getting a ~35% performance improvement from it > and is passing cairo's threading tests other than the one that fails > due to non-threading-related issuse. > > Actually glx-multithread-texture is pass with glthread (nouveau) As a quick summary: * there are now only 2 minors fail on piglit with my latest patches (sent to Marek) * I have a pending patch to allow asynchronous PBO transfer * Now that piglit is crash free I will give a try to both glxgear and glmark. Hopefully they will be both good. Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
Hum, I need to check it. I think I disabled EGL because I didn't have latest waffle on my Debian (and Nvidia didn't support fully EGL back then). However, I forgot to say that a fair number of piglit tests are based on GL1 direct call. (typically to draw a rectangle without a VBO). All those tests will disable glthread in the middle. In the current state, piglit is far from ideal to test glthread. Maybe CTS could help to increase the coverage. I will give it a shot. Gregory On 2/13/17, Marek Olšák wrote: > On Mon, Feb 13, 2017 at 11:10 AM, Gregory Hainaut > wrote: >>> On 11/02/17 06:36 AM, gregory hainaut wrote: >>> > >>> > On GLX side, I can explain some crashes (didn't debug all neither >>> > fail) >>> > and I suspect there are related to the previous report. >>> > >>> > At the context creation a glthread dispatcher is created and the *TLS* >>> > variable u_current_table is set with the new gl marshal function. >>> > If deprecated non-VBO functions are detected >>> > (glBegin/glVertexPointer/... and others horror from the previous >>> > century), the code will disable glthread. >>> > >>> > 1/ thread will be deleted >>> > 2/ u_current_table will be set to the original value >>> > >>> > However u_current_table is local to a thread. So a single thread will >>> > get the correct dispatcher table. Remaining threads will keep the >>> > previous marshal thread. If a glthread function is called without the >>> > thread behind, then BOOM. >>> >>> If I understand correctly, this scenario can only happen if the >>> application takes advantage of the GLX_MESA_multithread_makecurrent >>> extension. I wonder if we shouldn't just stop advertising that extension >>> with glthread enabled. >>> >>> Actually, I wonder how well GLX_MESA_multithread_makecurrent works >>> regardless of glthread. I suspect the assumption of a 1:1 correspondence >>> between threads and contexts is quite widespread in the Mesa code. Case >>> in point, the piglit test glx at glx-multithread-texture has been >>> failing >>> forever with radeonsi, though it passes with llvmpipe. Are there any >>> real world applications which require GLX_MESA_multithread_makecurrent, >>> or at least get a significant benefit from it? >> >> >> Hello, >> >> >> Yes you're right. I just found out yesterday that it was related to >> GLX_MESA_multithread_makecurrent. >> I did a small patch to not kill glthread when direct GL1 code is used. It >> avoid the crash but it is far >> from robust. You could imagine situation where 1 thread uses dispatcher >> function and another >> use std serial call (of course without any sync of glthread). Now if you >> use >> only modern GL >> (that will always use glthread), it might work. Note: Piglit is working >> as >> both thread will restore >> the standard dispatch function. >> >> Feral said that they tried the extension but a gl dispatcher was faster. >> Googling a bit I found >> Eric's initial mail. So it seems to be used. Extension isn't published on >> Kronos, so usage likely >> remain confidential. Potentially glthread will be as fast as GLX_MT on >> cairo. But you're right, >> I don't see the point to support both at the same time. >>> >>> I'm happy now with the implementation of this extension, I've got >>> tests for most of the issues mentioned (the one that's really missing >>> is the bind-to-new-drawable one, but I don't anticipate issues with >>> it), and cairo-gl is getting a ~35% performance improvement from it >>> and is passing cairo's threading tests other than the one that fails >>> due to non-threading-related issuse. >> >> >> Actually glx-multithread-texture is pass with glthread (nouveau) >> >> >> As a quick summary: >> * there are now only 2 minors fail on piglit with my latest patches >> (sent >> to Marek) >> * I have a pending patch to allow asynchronous PBO transfer >> * Now that piglit is crash free I will give a try to both glxgear and >> glmark. Hopefully they will be both good. > > Note that if you run piglit with egl/drm, it might not have glthread > enabled. > > Marek > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
If I remember correctly I got something like 4K-8K fails (total is 40K tests) when I broke the GL1 restore dispatch function. So it is around 10%-20%. Maybe it was only the test below 3.1. I was afraid that various modern extension could still be tested with some GL1 drawing. On 2/13/17, Marek Olšák wrote: > On Mon, Feb 13, 2017 at 3:56 PM, Gregory Hainaut > wrote: >> Hum, I need to check it. I think I disabled EGL because I didn't have >> latest waffle on my Debian (and Nvidia didn't support fully EGL back >> then). >> >> However, I forgot to say that a fair number of piglit tests are based >> on GL1 direct call. (typically to draw a rectangle without a VBO). All > > I'd disagree. All GL 3.1 tests and above have to use VBOs. GL core requires > it. > > Marek > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
On Mon, 13 Feb 2017 17:04:01 +0100 Marek Olšák wrote: > On Mon, Feb 13, 2017 at 4:14 PM, Gregory Hainaut > wrote: > > If I remember correctly I got something like 4K-8K fails (total is 40K > > tests) when I broke the GL1 restore dispatch function. So it is > > around 10%-20%. Maybe it was only the test below 3.1. I was afraid > > that various modern extension could still be tested with some GL1 > > drawing. > > The number of tests run doesn't necessarily correspond to the amount > of test coverage. 10 tests doing different things can be more useful > than 1 tests doing the same thing. > > Marek Fair point. As a side note, I tested both glxgear and glmark2 which are now crash-free :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] doc: GL_ARB_buffer_storage is supported on llvmpipe/swr
At least, the extension is exported (gallium capability PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT is 1) Signed-off-by: Gregory Hainaut --- docs/features.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/features.txt b/docs/features.txt index d9528e9..9d3a460 100644 --- a/docs/features.txt +++ b/docs/features.txt @@ -191,7 +191,7 @@ GL 4.3, GLSL 4.30 -- all DONE: i965/gen8+, nvc0, radeonsi GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all drivers) - GL_ARB_buffer_storage DONE (i965, nv50, r600) + GL_ARB_buffer_storage DONE (i965, nv50, r600, llvmpipe, swr) GL_ARB_clear_texture DONE (i965, nv50, r600, llvmpipe, softpipe) GL_ARB_enhanced_layouts DONE (i965, nv50, llvmpipe, softpipe) - compile-time constant expressions DONE -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] doc: GL_ARB_buffer_storage is supported on llvmpipe/swr
On 2/25/17, Edward O'Callaghan wrote: > Acked-by: Edward O'Callaghan > > On 02/25/2017 07:45 AM, Gregory Hainaut wrote: >> At least, the extension is exported (gallium capability >> PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT is 1) >> >> Signed-off-by: Gregory Hainaut >> --- >> docs/features.txt | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/docs/features.txt b/docs/features.txt >> index d9528e9..9d3a460 100644 >> --- a/docs/features.txt >> +++ b/docs/features.txt >> @@ -191,7 +191,7 @@ GL 4.3, GLSL 4.30 -- all DONE: i965/gen8+, nvc0, >> radeonsi >> GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi >> >>GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all >> drivers) >> - GL_ARB_buffer_storage DONE (i965, nv50, >> r600) >> + GL_ARB_buffer_storage DONE (i965, nv50, >> r600, llvmpipe, swr) >>GL_ARB_clear_texture DONE (i965, nv50, >> r600, llvmpipe, softpipe) >>GL_ARB_enhanced_layouts DONE (i965, nv50, >> llvmpipe, softpipe) >>- compile-time constant expressions DONE >> > > Hello, I don't have git access. Could you push it too ? Thanks you, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/12] sso: Create extensions entry points
On Fri, 24 May 2013 16:27:59 -0700 Ian Romanick wrote: > I'm finally getting off my lazy behind and reviewing this series. I'll > probably only be able to review one or two patches today, but I should > be able to get through the rest shortly. > > So far, it's just little stuff. > Hello. Did you review "the rest" (5-9, 11) ? Or did I miss your reply. By the way did you get any news on the various "spec bug" that you had opened. Cheers ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Status of GL_ARB_separate_shader_objects? I would like to help.
Hello, I would like to run PCSX2 on mesa drivers but unfortunately it miss a couple of features, mostly the GL_ARB_separate_shader_objects extension. I think I can help to implement this extension at least the opengl (ie not glsl) plumbering part (with some piglit tests). The extension GL_EXT_separate_shader_object is marked supported by the driver but I don't know if GLSL fully support separate program or with limitation. I already begun to implement the extension, enough to write some piglit tests. However 2 parts on the specs are not clear for me. * GenProgramPipelines doesn't create object! ... Spec extract: These names are marked as used, for the purposes of GenBuffers only, but they acquire buffer state only when they are first bound with BindBuffer (see below), just as if they were unused. ... Basically any command (like BindBuffer) that access the pipeline will create the pipeline. It seems like vertex array object. From an implemention point of view it seems much easier to create the object during GenProgramPipelines call. However I don't know if IsProgramPipeline must return FALSE if the object was never really created (bind) like VAO. * Mix between Pipeline object and the standard UseProgram/ActiveProgram. Here some extract of the spec: # BindProgramPipeline may also be used to bind an existing program pipeline object. If no current program object has been established by UseProgram, the pro- gram objects used for each shader stage and for uniform updates are taken from the bound program pipeline object, if any. If there is a current program object established by UseProgram, the bound program pipeline object has no effect on rendering or uniform updates. When a bound program pipeline object is used for rendering, individual shader executables are taken from its program objects as de- scribed in the discussion of UseProgram in section 7.3). # glUseProgram If program is non-zero, this command will make program the current program object. This will install executable code as part of the current rendering state for each shader stage present when the program was last successfully linked. If UsePro- gram is called with program set to zero, then there is no current program object. From http://www.opengl.org/registry/specs/ARB/separate_shader_objects.txt If both extensions are supported, the rule giving priority to UseProgram over pipeline objects needs to be updated, given that the single UseProgram binding point is replaced by a collection of binding points. We effectively treat this collection of binding points as another pipeline object, and treat that object as higher priority if it has a program attached to *any* attachment point. The priority rules in this spec are rewritten as follows: The executable code for an individual shader stage is taken from the current program for that stage. If there is a current program object for any shader stage or for uniform updates established by UseProgram, UseShaderProgramEXT, or ActiveProgramEXT, the current program for that stage (if any) is considered current. Otherwise, if there is a bound program pipeline object ... Note that with these rules, it's not possible to mix program objects bound to the context with program objects bound to a program pipeline object; if any program is bound to the context, the current pipeline object is ignored. So how the following case must be handle? case 1/ RESET STATE glUseProgram(2) BindProgramPipeline(5) # no effect but is the pipeline bound or not? # My understand is not. case 2/ RESET STATE BindProgramPipeline(5) glUseProgram(2) # higher priority than the pipeline. Do we # unbind the pipeline? glUseProgram(0) # What state is expected NULL or pipeline 5? my understanding is NULL Case 3/ RESET STATE BindProgramPipeline(5) glUseProgram(0) # Don't use a real program. What state is # expected NULL or pipeline 5? My understanding is NULL Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Status of GL_ARB_separate_shader_objects? I would like to help.
On Fri, 22 Mar 2013 15:44:07 -0700 Matt Turner wrote: > On Fri, Mar 22, 2013 at 1:00 PM, gregory hainaut > wrote: > > * GenProgramPipelines doesn't create object! > > ... Spec extract: > > These names are marked as used, for the purposes of GenBuffers only, > > but they acquire buffer state only when they are first bound with > > BindBuffer (see below), just as if they were unused. > > ... > > > > Basically any command (like BindBuffer) that access the pipeline > > will create the pipeline. It seems like vertex array object. From an > > implemention point of view it seems much easier to create the object > > during GenProgramPipelines call. However I don't know if > > IsProgramPipeline must return FALSE if the object was never really > > created (bind) like VAO. > > This is a weird part of the spec. After glGen* (but before glBind*) > the associated glIs* function usually returns false. It's something > that no one but conformance tests seem to care about. See commit > fd93d55141f11069fb76a9b377ad1af88d0ecdd3 (in Mesa) for how to fix this > kind of thing. > > I said "usually" above because there is some inconsistency. The > ARB_sampler_objects spec says that the act of calling glIsSampler() > actually creates the object. > > It looks like for ARB_separate_shader_objects that glGen* followed by > glIs* should return false (like VAOs). Ok. Thanks for the example. I updated my code and create a piglit test. By the way, fglrx doesn't follow this behavior, dunno for nvidia. On the mix UseProgram/BindProgramPipeline subjet. I try to search the spec for additional info and found this example: ## Issue 4: When a non-zero program is passed to UseProgram, any subsequent uniform updates will affect that program, ignoring the active program in any bound pipeline object. For example: glUseProgram(0); glBindProgramPipeline(1); glActiveProgram(1, 2); glUniform1f(0, 3.0); // affects program 2 glUseProgram(3); glUniform1f(0, 3.0); // affects program 3 glUseProgram(0); glUniform1f(0, 3.0); // affects program 2 ### So after glUseProgram(0), the state of the pipeline is restored (or they forgot to update this part of the spec when they clarify the priority rule), at least the ActiveProgram. Anyway, I write an extensive piglit test and check the behavior on fglrx. Here the outcome, glUseProgram(0) destroy current program state, the pipeline need to be rebound again for any shader based rendering. However ActiveProgram is restored as the previous example! Any opinion is welcome, run the test on nvidia? Mimic AMD behavior? Cheers ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Piglit] [RFC] ARB_separate_shader_objects test v2
On Tue, 26 Mar 2013 16:58:17 -0700 Jordan Justen wrote: > Tested with NVidia binary: > fail :: spec/ARB_separate_shader_objects/sso-GetProgramPipelineiv > fail :: spec/ARB_separate_shader_objects/sso-mix_pipeline_useprogram > crash :: spec/ARB_separate_shader_objects/sso-IsProgramPipeline > (results piglit output attached) > > On Tue, Mar 26, 2013 at 12:56 PM, gregory hainaut > wrote: > > Plese find below my (really too big) patch. > > It's not really big at all, but I think the piglit-shader changes need > to be a separate commit/patch. > > -Jordan Thanks very much for your test. V2: * split the patch properly with git :) * sso-GetProgramPipelineiv: fix a VS linker error. Print current test step. * sso-mix_pipeline_useprogram: fix a wrong expected. Now the test must work on nvidia :) * sso-IsProgramPipeline: Print more current test step => my guess is nvidia choke on glDeleteProgramPipelines( -1, id); => Nvidia as AMD doesn't follow the spec (not very serious) Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] piglit util: new functions piglit_program_pipeline_check_status/quiet
Equivalent to piglit_link_check_status/quiet but with program object pipeline
---
tests/util/piglit-shader.c | 50
tests/util/piglit-shader.h |2 ++
2 files changed, 52 insertions(+)
diff --git a/tests/util/piglit-shader.c b/tests/util/piglit-shader.c
index c67e58a..d7a0266 100644
--- a/tests/util/piglit-shader.c
+++ b/tests/util/piglit-shader.c
@@ -213,6 +213,45 @@ link_check_status(GLint prog, FILE *output)
return ok;
}
+/* Same function as above but for program pipeline */
+static GLboolean
+program_pipeline_check_status(GLuint pipeline, FILE *output)
+{
+ GLchar *info = NULL;
+ GLint size;
+ GLint ok;
+
+ piglit_require_extension("GL_ARB_separate_shader_objects");
+
+ glValidateProgramPipeline(pipeline);
+ glGetProgramPipelineiv(pipeline, GL_VALIDATE_STATUS, &ok);
+
+ /* Some drivers return a size of 1 for an empty log. This is the size
+* of a log that contains only a terminating NUL character.
+*/
+ glGetProgramPipelineiv(pipeline, GL_INFO_LOG_LENGTH, &size);
+ if (size > 1) {
+ info = malloc(size);
+ glGetProgramPipelineInfoLog(pipeline, size, NULL, info);
+ }
+
+ if (!ok) {
+ fprintf(output, "Failed to validate the pipeline: %s\n",
+ (info != NULL) ? info : "");
+ }
+ else if (0 && info != NULL) {
+ /* Enable this to get extra linking info.
+* Even if there's no link errors, the info log may
+* have some remarks.
+*/
+ printf("Pipeline validataion warning: %s\n", info);
+ }
+
+ free(info);
+
+ return ok;
+}
+
GLboolean
piglit_link_check_status(GLint prog)
{
@@ -234,6 +273,17 @@ piglit_link_check_status_quiet(GLint prog)
return link_check_status(prog, stdout);
}
+GLboolean
+piglit_program_pipeline_check_status(GLuint pipeline)
+{
+ return program_pipeline_check_status(pipeline, stderr);
+}
+
+GLboolean
+piglit_program_pipeline_check_status_quiet(GLuint pipeline)
+{
+ return program_pipeline_check_status(pipeline, stdout);
+}
GLint piglit_link_simple_program(GLint vs, GLint fs)
{
diff --git a/tests/util/piglit-shader.h b/tests/util/piglit-shader.h
index 12cf731..0bb7792 100644
--- a/tests/util/piglit-shader.h
+++ b/tests/util/piglit-shader.h
@@ -34,6 +34,8 @@ GLuint piglit_compile_shader(GLenum target, const char
*filename);
GLuint piglit_compile_shader_text(GLenum target, const char *text);
GLboolean piglit_link_check_status(GLint prog);
GLboolean piglit_link_check_status_quiet(GLint prog);
+GLboolean piglit_program_pipeline_check_status(GLuint pipeline);
+GLboolean piglit_program_pipeline_check_status_quiet(GLuint pipeline);
GLint piglit_link_simple_program(GLint vs, GLint fs);
#if defined(PIGLIT_USE_OPENGL_ES1)
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/3] add 3 news tests for arb_separate_shader_objects
iv.c)
+piglit_add_executable (arb_separate_shader_object-mix_pipeline_useprogram
mix_pipeline_useprogram.c)
diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.txt
b/tests/spec/arb_separate_shader_objects/CMakeLists.txt
new file mode 100644
index 000..144a306
--- /dev/null
+++ b/tests/spec/arb_separate_shader_objects/CMakeLists.txt
@@ -0,0 +1 @@
+piglit_include_target_api()
diff --git a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
new file mode 100644
index 000..81f6978
--- /dev/null
+++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
@@ -0,0 +1,279 @@
+/*
+ * Copyright © 2013 Gregory Hainaut
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "piglit-util-gl-common.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+ config.supports_gl_compat_version = 10;
+
+ config.window_width = 32;
+ config.window_height = 32;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+static GLboolean pass;
+
+enum piglit_result
+piglit_display(void)
+{
+ /* UNREACHED */
+ return PIGLIT_FAIL;
+}
+
+static GLint
+stage2bitfield(GLint stage) {
+ switch(stage) {
+ case GL_VERTEX_SHADER: return GL_VERTEX_SHADER_BIT;
+ case GL_FRAGMENT_SHADER: return GL_FRAGMENT_SHADER_BIT;
+ case GL_GEOMETRY_SHADER: return GL_GEOMETRY_SHADER_BIT;
+ case GL_TESS_CONTROL_SHADER: return GL_TESS_CONTROL_SHADER_BIT;
+ case GL_TESS_EVALUATION_SHADER:return
GL_TESS_EVALUATION_SHADER_BIT;
+ case GL_COMPUTE_SHADER: return GL_COMPUTE_SHADER_BIT;
+ default:return 0;
+ }
+ return 0;
+}
+
+static void
+check_stage(GLint pipe, GLint expected, GLint stage, GLboolean supported) {
+ GLint param = 0;
+ glGetProgramPipelineiv(pipe, stage, ¶m);
+
+ if (!supported) {
+ pass &= piglit_check_gl_error(GL_INVALID_ENUM);
+ } else if (param != expected) {
+ fprintf(stderr, "Failed to get program of stage %s.\n",
piglit_get_gl_enum_name(stage));
+ pass = GL_FALSE;
+ }
+}
+
+static void
+use_stage_and_check(GLint pipe, GLint program, GLint stage, GLboolean
supported) {
+ printf("Attach program (%d) to stage (%s). Expected to be supported:
%s\n", program,
+ piglit_get_gl_enum_name(stage), supported ? "yes" :
"no");
+ glUseProgramStages(pipe, stage2bitfield(stage), program);
+ if (!supported) {
+ pass &= piglit_check_gl_error(GL_INVALID_VALUE);
+ } else {
+ pass &= piglit_check_gl_error(GL_NO_ERROR);
+ }
+
+ check_stage(pipe, program, stage, supported);
+}
+
+void
+piglit_init(int argc, char **argv)
+{
+ GLint vs, fs, gs, tcs, tes;
+ GLint ver;
+ GLuint pipe = 0;
+ GLint param = 0;
+ char version[100];
+ const char *shader_source[2];
+
+ const char *vs_source_150 =
+ "out gl_PerVertex {\n"
+ "vec4 gl_Position;\n"
+ "};\n"
+ "\n"
+ "in vec4 position;\n"
+ "\n"
+ "void main()\n"
+ "{\n"
+ " gl_Position = position;\n"
+ "}\n";
+ const char *vs_source_140 =
+ "void main()\n"
+ "{\n"
+ " gl_Position = gl_Vertex;\n"
+ "}\n";
+ const char *fs_source =
+ "void main()\n"
+ "{\n"
+ " gl_FragColor = vec4(0.0, 1.0, 0.0, 0.0);\n"
+ "}\n";
+
[Mesa-dev] [PATCH 3/3] update EXT_transform_feedback error detection
program pipeline add new INVALID_OPERATION (spec chapter 13.2.2)
Note: FGLRX don't report any of the expected errors...
---
tests/all.tests|4 +-
tests/spec/ext_transform_feedback/api-errors.c | 84 +++-
2 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/tests/all.tests b/tests/all.tests
index 2cbf3c4..cd759b5 100644
--- a/tests/all.tests
+++ b/tests/all.tests
@@ -1906,7 +1906,9 @@ for mode in ['interleaved_ok_base',
'interleaved_ok_range',
'bind_range_offset_2', 'bind_range_offset_3',
'bind_range_offset_5', 'bind_offset_offset_1',
'bind_offset_offset_2', 'bind_offset_offset_3',
- 'bind_offset_offset_5', 'not_a_program']:
+ 'bind_offset_offset_5', 'not_a_program',
+ 'useprogstage_noactive', 'useprogstage_active',
+ 'bind_pipeline']:
test_name = 'api-errors {0}'.format(mode)
ext_transform_feedback[test_name] = concurrent_test(
'ext_transform_feedback-{0}'.format(test_name))
diff --git a/tests/spec/ext_transform_feedback/api-errors.c
b/tests/spec/ext_transform_feedback/api-errors.c
index 04470b2..ba4fff8 100644
--- a/tests/spec/ext_transform_feedback/api-errors.c
+++ b/tests/spec/ext_transform_feedback/api-errors.c
@@ -78,6 +78,9 @@ enum test_mode {
BIND_BAD_SIZE,
BIND_BAD_OFFSET,
NOT_A_PROGRAM,
+ USEPROGSTAGE_ACTIVE,
+ USEPROGSTAGE_NOACTIVE,
+ BIND_PIPELINE
};
enum bind_mode {
@@ -97,6 +100,32 @@ static const char *vstext =
" gl_Position = vec4(1.0);\n"
"}\n";
+static const char *vstext_sep_150 =
+ "#extension GL_ARB_separate_shader_objects : enable\n"
+ "out gl_PerVertex {\n"
+ "vec4 gl_Position;\n"
+ "};\n"
+ "varying vec4 foo;\n"
+ "varying vec4 bar;\n"
+ "\n"
+ "void main()\n"
+ "{\n"
+ " foo = vec4(1.0);\n"
+ " bar = vec4(1.0);\n"
+ " gl_Position = vec4(1.0);\n"
+ "}\n";
+static const char *vstext_sep_140 =
+ "#extension GL_ARB_separate_shader_objects : enable\n"
+ "varying vec4 foo;\n"
+ "varying vec4 bar;\n"
+ "\n"
+ "void main()\n"
+ "{\n"
+ " foo = vec4(1.0);\n"
+ " bar = vec4(1.0);\n"
+ " gl_Position = vec4(1.0);\n"
+ "}\n";
+
static const char *varyings[] = { "foo", "bar" };
static struct test_desc
@@ -151,6 +180,10 @@ static struct test_desc
{ "bind_offset_offset_3",BIND_BAD_OFFSET, 3, OFFSET,
GL_INTERLEAVED_ATTRIBS, 1 },
{ "bind_offset_offset_5",BIND_BAD_OFFSET, 5, OFFSET,
GL_INTERLEAVED_ATTRIBS, 1 },
{ "not_a_program", NOT_A_PROGRAM,0, BASE,
GL_INTERLEAVED_ATTRIBS, 1 },
+ { "useprogstage_noactive", USEPROGSTAGE_NOACTIVE,0, BASE,
GL_INTERLEAVED_ATTRIBS, 1 },
+ { "useprogstage_active", USEPROGSTAGE_ACTIVE, 0, BASE,
GL_INTERLEAVED_ATTRIBS, 1 },
+ { "bind_pipeline", BIND_PIPELINE,0, BASE,
GL_INTERLEAVED_ATTRIBS, 1 },
+
};
static void
@@ -186,6 +219,7 @@ do_test(const struct test_desc *test)
{
GLuint vs;
GLuint progs[2];
+ GLuint pipes[2];
GLuint bufs[NUM_BUFFERS];
float initial_xfb_buffer_contents[XFB_BUFFER_SIZE];
GLboolean pass = GL_TRUE;
@@ -193,6 +227,10 @@ do_test(const struct test_desc *test)
int num_varyings = test->mode == NO_VARYINGS ? 0 : test->num_buffers;
GLint max_separate_attribs;
+ if (test->mode == USEPROGSTAGE_ACTIVE || test->mode ==
USEPROGSTAGE_NOACTIVE || test->mode == BIND_PIPELINE) {
+ piglit_require_extension("GL_ARB_separate_shader_objects");
+ }
+
glGetIntegerv(GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS,
&max_separate_attribs);
printf("MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTIBS=%i\n",
@@ -200,7 +238,19 @@ do_test(const struct test_desc *test)
printf("Compile vertex shader\n");
vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vstext);
- if (test->mode == NOT_A_PROGRAM) {
+ if (test->mode == USEPROGSTAGE_ACTIVE || test->mode ==
USEPROGSTAGE_NOACTIVE || test->mode == BIND_PIPELINE) {
+ /* Note, we can't use glCreateShaderProgramv because the setup
of transform feedback
+* must be done before linking
+*/
+ if (piglit_get_gl_version() >= 32)
+ vs = piglit_compile_shader_text(GL_VERTEX_SHADER,
vstext_sep_150);
+ else
+ vs = piglit_compile_shader_text(GL_VERTEX_SHADER,
vstext_sep_140);
+ progs[0] = glCreateProgram();
+ glProgramParameteri(progs[0], GL_PROGRAM_SEPARABLE, GL_TRUE);
+ glAttachShader(progs[0], vs);
+
+ } else if (test->mode == NOT_A_PROGRAM) {
printf("Create a program and then delete it\n");
Re: [Mesa-dev] [PATCH 00/12] RFC: add support of ARB_separate_shader_object extensions
On Fri, 05 Apr 2013 14:44:54 -0700 Ian Romanick wrote: > On 04/05/2013 02:25 PM, gregory wrote: > > Hello, > > > > Please find an implementation of the ARB_separate_shader_objects > > extensions. I concentrate mostly on the state part of > > the extensions aka the pipeline object. I think GLSL already compiled > > program separately anyway. > > > > I test my implementation on the test that I send yesterday ago on piglit. > > All tests are ok but I miss a test for new uniform function. > > Besides there are still some parts unimplemented: > > 1/ GLX Protocol: not sure it will be useful, as understand GLX is kinda > > drepecated > > We don't have any other GLX support for GLSL, so I wouldn't worry about it. > > > 2/ Display list: need to be done or maybe enable the extensions on CORE > > profile > > I haven't had any requests for the functionality with compatibility > profiles. As far as I can tell, all of the ISVs that want this feature > are already shifting to core profiles. > > Maybe Aras can give us one humble ISVs opinion. :) We can also enable the extension on compatibility profile but without the display list part. Not ideal but better than nothings. FYI, only glUseProgramStages can be compiled within a display list. All others entry points are executed immediately. > > > Stuff that bug me: > > 1/ I don't get how to use ralloc_strdup. So I set the ralloc's context to > > NULL, not sure it is fully correct > > The ralloc memory context is (usually) some other thing allocated by > ralloc. When the context is freed, all of the things allocated using > that context will also be freed. So, > > a = ralloc_size(NULL, 32); > b = ralloc_size(a, 32); > ... > ralloc_free(a); > > Will free both a and b. > > You can also use ralloc_steal to reparent an allocation. So, > > a = ralloc_size(NULL, 32); > b = ralloc_size(a, 32); > c = ralloc_size(NULL, 32); > > ralloc_steal(c, b); > > ralloc_free(a); > > will only cause a to be freed. b is now in the context of c. > > We use this in the GLSL compiler to, basically, implement a > mark-and-sweep garbage collector. As various parts of the compiler > allocate new objects and orphan others, we'll do a ralloc_steal pass to > reparent all of the objects that are still connected. A ralloc_free of > the old context will destroy all the objects that are no longer connected. Thanks very much for the input. I definitely had a memory leak which is now fixed on my tree :) > > > 2/ I implement the feature as a pure mesa state. I don't know if they're > > any rule to create driver functions. Maybe it > > would be better to add > > CreatePipelineObject/DeletePipelineObject/BindPipeline/UseProgramStages. > > Opinion is welcome > > > > Note: I didn't yet rebase my work on latest mesa. Not sure if it is > > critical for a preliminary review. > > Note2: I create the xml manually, don't know if there is any automatic flow. > > > > Thanks ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] sso: replace Shader binding point with _Shader
On Sat, 06 Apr 2013 08:52:35 -0600
Brian Paul wrote:
> One more comment for now below. I may not get to review the rest for
> a few days.
>
> -Brian
Thanks very much for the review. I redid properly my patch and rebase
my work on a more recent mesa version. I want to do a piglit non regression test
first and double check any typo. Then I will probabaly post the V2 friday
except if you prefer to finish the first review first.
FYI, I choose to keep the shorter "Current" so we got the basic
"Pipeline.Current"
Cheers,
Gregory
>
> On 04/05/2013 03:27 PM, gregory wrote:
> > To avoid NULL pointer check a default pipeline object is installed in
> > _Shader when no
> > program is current
> >
> > The spec say that UseProgram/UseShaderProgramEXT/ActiveProgramEXT got an
> > higher
> > priority over the pipeline object. When default program is uninstall, the
> > pipeline is
> > used if any was bound.
> >
> > Note: A careful rename need to be done now...
> > ---
> > src/mesa/main/mtypes.h |4 ++
> > src/mesa/main/pipelineobj.c |8
> > src/mesa/main/shaderapi.c | 91
> > +--
> > 3 files changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 2445574..dc54f3d 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -2433,6 +2433,9 @@ struct gl_pipeline_shader_state
> > /** Currently bound pipeline object. See _mesa_BindProgramPipeline() */
> > struct gl_shader_state *PipelineObj;
> >
> > + /* Default Object to ensure that _Shader is never NULL */
> > + struct gl_shader_state *Default;
> > +
> > /** Pipeline objects */
> > struct _mesa_HashTable *Objects;
> > };
> > @@ -3541,6 +3544,7 @@ struct gl_context
> >
> > struct gl_pipeline_shader_state Pipeline; /**< GLSL pipeline shader
> > object state */
> > struct gl_shader_state Shader; /**< GLSL shader object state */
> > + struct gl_shader_state *_Shader; /**< Points to ::Shader or
> > ::Pipeline.PipelineObj or ::Pipeline.Default */
>
> If gl_shader_state gets renamed to gl_pipeline_object then this field
> would probably be better name CurrentPipeline, or similar.
>
>
>
> > struct gl_shader_compiler_options
> > ShaderCompilerOptions[MESA_SHADER_TYPES];
> >
> > struct gl_query_state Query; /**< occlusion, timer queries */
> > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> > index 7a56c67..c805cdf 100644
> > --- a/src/mesa/main/pipelineobj.c
> > +++ b/src/mesa/main/pipelineobj.c
> > @@ -97,6 +97,10 @@ _mesa_init_pipeline(struct gl_context *ctx)
> > ctx->Pipeline.Objects = _mesa_NewHashTable();
> >
> > ctx->Pipeline.PipelineObj = NULL;
> > +
> > + /* Install a default Pipeline */
> > + ctx->Pipeline.Default = _mesa_new_pipeline_object(ctx, 0);
> > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader,
> > ctx->Pipeline.Default);
> > }
> >
> >
> > @@ -120,6 +124,10 @@ _mesa_free_pipeline_data(struct gl_context *ctx)
> > {
> > _mesa_HashDeleteAll(ctx->Pipeline.Objects, delete_pipelineobj_cb, ctx);
> > _mesa_DeleteHashTable(ctx->Pipeline.Objects);
> > +
> > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader, NULL);
> > + _mesa_delete_pipeline_object(ctx, ctx->Pipeline.Default);
> > +
> > }
> >
> > /**
> > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> > index a86a429..1092287 100644
> > --- a/src/mesa/main/shaderapi.c
> > +++ b/src/mesa/main/shaderapi.c
> > @@ -43,6 +43,7 @@
> > #include "main/hash.h"
> > #include "main/mfeatures.h"
> > #include "main/mtypes.h"
> > +#include "main/pipelineobj.h"
> > #include "main/shaderapi.h"
> > #include "main/shaderobj.h"
> > #include "main/transformfeedback.h"
> > @@ -138,6 +139,8 @@ _mesa_free_shader_state(struct gl_context *ctx)
> > _mesa_reference_shader_program(ctx,&ctx->Shader.ActiveProgram, NULL);
> >
> > /* Extended for ARB_separate_shader_objects */
> > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader, NULL);
> > +
> > assert(ctx->Shader.RefCount == 1);
> > _glthread_DESTROY_MUTEX(ctx->Shader.Mutex);
> > }
> > @@ -1453,7 +1456,29 @@ _mesa_UseProgram(GLhandleARB program)
> > shProg = NULL;
> > }
> >
> > - _mesa_use_program(ctx, shProg);
> > + /*
> > +* The executable code for an individual shader stage is taken from the
> > +* current program for that stage. If there is a current program object
> > +* for any shader stage or for uniform updates established by
> > UseProgram,
> > +* UseShaderProgramEXT, or ActiveProgramEXT, the current program for
> > that
> > +* stage (if any) is considered current. Otherwise, if there is a bound
> > +* program pipeline object ...
> > +*/
> > + if (program) {
> > + /* Attach shader state to the binding point */
> > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader,&ctx->Shader);
> > +
Re: [Mesa-dev] [PATCH 00/12] RFC: add support of ARB_separate_shader_object extensions V2
On Fri, 12 Apr 2013 09:39:15 -0700 (PDT) Jose Fonseca wrote: > - Original Message - > > From: "gregory" > > To: "gregory hainaut" > > FWIW, your emails BCC'ing the list are a bit of a pain. They manage elude > most of my mail filtering rules, and replying-to-all doesn't include the > list... > > Jose > > > Sent: Friday, April 12, 2013 5:22:02 PM > > Subject: [Mesa-dev] [PATCH 00/12] RFC: add support of > > ARB_separate_shader_object extensions V2 > [...] Thanks for the info I will look. Actually I'm not sure it is a bcc. I setup git to send the email to my draft box, then I used the redirect (to avoid wrapping issue) feature of claws-mail and I manually set "to: mesa" and myself in "cc". The "to :myself" was done by git header but I'm not sure it is used. Maybe it will work better if I send directly git email to my upload queue with the good cc! Sorry for the disturbance ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/12] RFC: add support of ARB_separate_shader_object extensions V2
On Fri, 12 Apr 2013 19:02:47 +0200 gregory hainaut wrote: > On Fri, 12 Apr 2013 09:39:15 -0700 (PDT) > Jose Fonseca wrote: > > > - Original Message - > > > From: "gregory" > > > To: "gregory hainaut" > > > > FWIW, your emails BCC'ing the list are a bit of a pain. They manage elude > > most of my mail filtering rules, and replying-to-all doesn't include the > > list... > > > > Jose > > > > > Sent: Friday, April 12, 2013 5:22:02 PM > > > Subject: [Mesa-dev] [PATCH 00/12] RFC: add support of > > > ARB_separate_shader_object extensions V2 > > [...] > > > Thanks for the info I will look. > > Actually I'm not sure it is a bcc. I setup git to send the email to my draft > box, then I used the redirect (to avoid wrapping issue) > feature of claws-mail and I manually set "to: mesa" and myself in "cc". The > "to :myself" was done by git header but I'm not sure it is used. Maybe it > will work better if I send directly git email to my upload queue with the > good cc! > > Sorry for the disturbance FYI, I changed my flow to bypass the redirect feature. Future patch will be better :) By the way I upload a github tree: https://github.com/gregory38/mesa-sso/tree/sso2 Branch: sso2 Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/12] sso: Create extensions entry points
On Fri, 12 Apr 2013 12:38:19 -0700 Eric Anholt wrote: > Please, patches for Mesa have to actually be addressed to Mesa. What do you mean? The github tree (was requested by Jordan)? Or because the mail header doesn't contains mesa address (mail client setup issue that it is now fixed) ? > I'm really excited to see progress on SSO, so hopefully we can get some > of this landed soon. > > gregory writes: > > > V2: formatting improvement > > patch versioning generally comes at the end of the commit message. > > Also, could you fix up your git author info to include your full name? > > > diff --git a/src/mapi/glapi/gen/ARB_separate_shader_objects.xml > > b/src/mapi/glapi/gen/ARB_separate_shader_objects.xml > > new file mode 100644 > > index 000..29a37f5 > > --- /dev/null > > +++ b/src/mapi/glapi/gen/ARB_separate_shader_objects.xml > > > + > > + > > + > > + > > + > > + > > + > > + > > Missing ProgramParameteri here. Unfortunately it complains about duplication. The function is already defined in ARB_get_program_binary.xml. I could put a dummy name with an alias. I don't see any specific syntax on the python. > > > + > > + > > + > > + > > + > > > +
Re: [Mesa-dev] [PATCH 01/12] sso: Create extensions entry points
On Fri, 12 Apr 2013 13:52:46 -0700 Eric Anholt wrote: > gregory hainaut writes: > > > On Fri, 12 Apr 2013 12:38:19 -0700 > > Eric Anholt wrote: > > > >> Please, patches for Mesa have to actually be addressed to Mesa. > > > > What do you mean? The github tree (was requested by Jordan)? Or > > because the mail header doesn't contains mesa address (mail client > > setup issue that it is now fixed) ? > > The mail header. > Any feedback on the others patches ? Would you prefer a resending of the patch with the proper mail header ? signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 00/12] RFC: add support of ARB_separate_shader_object extensions V3
Please find an implementation of the ARB_separate_shader_objects extensions. I concentrate mostly on the state part of the extensions aka the pipeline object. I think GLSL already compiled program separately anyway. I test my implementation on the test that I send on piglit mailing list. All tests are ok but I miss a test for new uniform function. Besides there are still some parts unimplemented: 1/ GLX Protocol: dropped 2/ Display list: would be done later if someone is interested otherwise dropped Stuff that bug me: 1/ I implement the feature as a pure mesa state. I don't know if they're any rule to create driver functions. Maybe it would be better to add CreatePipelineObject/DeletePipelineObject/BindPipeline/UseProgramStages. Opinion is welcome A github branch can be found here (based on v2 but v3 add only comment): * https://github.com/gregory38/mesa-sso/tree/sso2 V2: * fix ralloc memory leak * follow mesa formating rule * rename Pipeline.PipelineObj to Pipeline.Current * rename gl_shader_state to gl_pipeline_object * rebase on current mesa V3: * resend properly the patches on the mesa mailing list * Add minor comment Note: Rebase done on ac1118d53c0b22db8dcd6fcdcd2d1a245037dbc1 Gregory Hainaut (12): sso: Create extensions entry points sso: Add pipeline container/state sso: add support of GL_PROGRAM_SEPARABLE and CreateShaderProgramv sso: implement ActiveShaderProgram & GetProgramPipelineiv sso: replace Shader binding point with _Shader sso: rename Shader to the pointer _Shader sso: update meta state sso: Implement _mesa_UseProgramStages sso: implement BindProgramPipeline sso: update glGet: GL_PROGRAM_PIPELINE_BINDING sso: implement ValidateProgramPipeline and GetProgramPipelineInfoLog sso: Finally enable the extension on Gallium src/mapi/glapi/gen/ARB_separate_shader_objects.xml | 401 ++ src/mapi/glapi/gen/Makefile.am |1 + src/mapi/glapi/gen/gl_API.xml |6 +- src/mapi/glapi/gen/gl_genexec.py |1 + src/mesa/drivers/common/meta.c | 38 +- src/mesa/drivers/dri/i965/brw_gs.c |2 +- src/mesa/drivers/dri/i965/brw_shader.cpp |4 +- src/mesa/drivers/dri/i965/brw_vs.c |4 +- src/mesa/drivers/dri/i965/brw_vs_surface_state.c |2 +- src/mesa/drivers/dri/i965/brw_wm.c |2 +- src/mesa/drivers/dri/i965/brw_wm_surface_state.c |2 +- src/mesa/drivers/dri/i965/gen6_sol.c |6 +- src/mesa/drivers/dri/i965/gen6_vs_state.c |2 +- src/mesa/drivers/dri/i965/gen6_wm_state.c |2 +- src/mesa/drivers/dri/i965/gen7_sol_state.c |4 +- src/mesa/drivers/dri/i965/gen7_vs_state.c |2 +- src/mesa/drivers/dri/i965/gen7_wm_state.c |2 +- src/mesa/main/api_validate.c |2 +- src/mesa/main/context.c| 44 +- src/mesa/main/extensions.c |1 + src/mesa/main/ff_fragment_shader.cpp |8 +- src/mesa/main/get.c| 19 + src/mesa/main/get_hash_params.py |3 + src/mesa/main/mtypes.h | 43 +- src/mesa/main/pipelineobj.c| 834 src/mesa/main/pipelineobj.h| 102 +++ src/mesa/main/shaderapi.c | 252 -- src/mesa/main/shaderapi.h | 10 +- src/mesa/main/state.c | 14 +- src/mesa/main/texstate.c | 12 +- src/mesa/main/transformfeedback.c |4 +- src/mesa/main/uniform_query.cpp| 75 +- src/mesa/main/uniforms.c | 466 ++- src/mesa/main/uniforms.h | 86 ++ src/mesa/program/ir_to_mesa.cpp| 12 +- src/mesa/sources.mak |1 + src/mesa/state_tracker/st_atom_clip.c |2 +- src/mesa/state_tracker/st_atom_constbuf.c |4 +- src/mesa/state_tracker/st_cb_drawpixels.c |2 +- src/mesa/state_tracker/st_draw.c |6 +- src/mesa/state_tracker/st_extensions.c |1 + src/mesa/state_tracker/st_glsl_to_tgsi.cpp |2 +- src/mesa/state_tracker/st_program.c|6 +- src/mesa/swrast/s_fragprog.c |2 +- 44 files changed, 2319 insertions(+), 175 deletions(-) create mode 100644 src/mapi/glapi/gen/ARB_separate_shader_objects.xml create mode 100644 src/mesa/main/pipelineobj.c create mode 100644 src/mesa/main/pipelineobj.h -- 1.7.10.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 02/12] sso: Add pipeline container/state
V1:
* Extend gl_shader_state as pipeline object state
* Add a new container gl_pipeline_shader_state that contains
binding point of the previous object
* Update mesa init/free shader state due to the extension of
the attibute
* Add an init/free pipeline function for the context
* Implement GenProgramPipeline/DeleteProgramPipeline/IsProgramPipeline.
I based my work on the VAO code.
V2:
* Rename gl_shader_state to gl_pipeline_object
* Rename Pipeline.PipelineObj to Pipeline.Current
* Rename ValidationStatus to Validated
* Formatting improvement
---
src/mesa/main/context.c |3 +
src/mesa/main/mtypes.h | 30 +-
src/mesa/main/pipelineobj.c | 234 ++-
src/mesa/main/pipelineobj.h | 25 +
src/mesa/main/shaderapi.c | 14 ++-
src/mesa/main/shaderapi.h |3 +
6 files changed, 303 insertions(+), 6 deletions(-)
diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 0539934..d089827 100644
--- a/src/mesa/main/context.c
+++ b/src/mesa/main/context.c
@@ -106,6 +106,7 @@
#include "macros.h"
#include "matrix.h"
#include "multisample.h"
+#include "pipelineobj.h"
#include "pixel.h"
#include "pixelstore.h"
#include "points.h"
@@ -762,6 +763,7 @@ init_attrib_groups(struct gl_context *ctx)
_mesa_init_lighting( ctx );
_mesa_init_matrix( ctx );
_mesa_init_multisample( ctx );
+ _mesa_init_pipeline( ctx );
_mesa_init_pixel( ctx );
_mesa_init_pixelstore( ctx );
_mesa_init_point( ctx );
@@ -1167,6 +1169,7 @@ _mesa_free_context_data( struct gl_context *ctx )
_mesa_free_texture_data( ctx );
_mesa_free_matrix_data( ctx );
_mesa_free_viewport_data( ctx );
+ _mesa_free_pipeline_data(ctx);
_mesa_free_program_data(ctx);
_mesa_free_shader_state(ctx);
_mesa_free_queryobj_data(ctx);
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 05d8518..4487068 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2381,9 +2381,19 @@ struct gl_shader_program
/**
* Context state for GLSL vertex/fragment shaders.
+ * Extended to support pipeline object
*/
-struct gl_shader_state
+struct gl_pipeline_object
{
+ /** Name of the pipeline object as received from glGenProgramPipelines.
+* It would be 0 for shaders without separate shader objects.
+*/
+ GLuint Name;
+
+ GLint RefCount;
+
+ _glthread_Mutex Mutex;
+
/**
* Programs used for rendering
*
@@ -2405,8 +2415,23 @@ struct gl_shader_state
struct gl_shader_program *ActiveProgram;
GLbitfield Flags;/**< Mask of GLSL_x flags */
+
+ GLboolean Validated; /**< Pipeline Validation status */
+
+ GLboolean EverBound; /**< Has the pipeline object been
created */
};
+/**
+ * Context state for GLSL pipeline shaders.
+ */
+struct gl_pipeline_shader_state
+{
+ /** Currently bound pipeline object. See _mesa_BindProgramPipeline() */
+ struct gl_pipeline_object *Current;
+
+ /** Pipeline objects */
+ struct _mesa_HashTable *Objects;
+};
/**
* Compiler options for a single GLSL shaders type
@@ -3514,7 +3539,8 @@ struct gl_context
struct gl_geometry_program_state GeometryProgram;
struct gl_ati_fragment_shader_state ATIFragmentShader;
- struct gl_shader_state Shader; /**< GLSL shader object state */
+ struct gl_pipeline_shader_state Pipeline; /**< GLSL pipeline shader object
state */
+ struct gl_pipeline_object Shader; /**< GLSL shader object state */
struct gl_shader_compiler_options ShaderCompilerOptions[MESA_SHADER_TYPES];
struct gl_query_state Query; /**< occlusion, timer queries */
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index e7e628b..d81bd0e 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -30,6 +30,9 @@
* Implementation of pipeline object related API functions. Based on
* GL_ARB_separate_shader_objects extension.
*
+ * XXX things to do:
+ * 1/ Do we need to create 2 new drivers functions: CreatePipelineObject
+ * DeletePipelineObject
*/
#include "main/glheader.h"
@@ -51,6 +54,168 @@
#include "../glsl/glsl_parser_extras.h"
#include "../glsl/ir_uniform.h"
+/**
+ * Delete a pipeline object.
+ */
+void
+_mesa_delete_pipeline_object(struct gl_context *ctx, struct gl_pipeline_object
*obj)
+{
+ _mesa_reference_shader_program(ctx, &obj->_CurrentFragmentProgram, NULL);
+ _mesa_reference_shader_program(ctx, &obj->CurrentFragmentProgram, NULL);
+ _mesa_reference_shader_program(ctx, &obj->CurrentVertexProgram, NULL);
+ _mesa_reference_shader_program(ctx, &obj->CurrentGeometryProgram, NULL);
+ _mesa_reference_shader_program(ctx, &obj->ActiveProgram, NULL);
+ _glthread_DESTROY_MUTEX(obj->Mutex);
+ ralloc_free(obj);
+}
+
+/**
+ * Allocate and initialize a new pipeline object.
+ */
+static struct gl_pipeline_object *
+_mesa_new_pipeline_object(struct gl_context *ctx, GLuint name)
+{
+ struct gl_pipeline_object *obj = rz
[Mesa-dev] [PATCH 03/12] sso: add support of GL_PROGRAM_SEPARABLE and CreateShaderProgramv
V1:
CreateShaderProgramv is similar as CreateShaderProgramEXT. The 2 differences are
1/ it an array of strings
2/ it support the GL_PROGRAM_SEPARABLE (aka SeparateShader) flag
V2: Formatting improvement
---
src/mesa/main/mtypes.h|5 +++
src/mesa/main/shaderapi.c | 94 +++--
src/mesa/main/shaderapi.h |3 +-
3 files changed, 72 insertions(+), 30 deletions(-)
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 4487068..f979cd0 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2255,6 +2255,11 @@ struct gl_shader_program
* glClear()).
*/
GLboolean InternalSeparateShader;
+ /* ARB_separate_shader_objects
+* indicates whether program can be bound for individual pipeline stages
using
+* UseProgramStages after it is next linked.
+*/
+ GLboolean SeparateShader;
GLuint NumShaders; /**< number of attached shaders */
struct gl_shader **Shaders; /**< List of attached the shaders */
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 774163d..46072ba 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -622,6 +622,11 @@ get_programiv(struct gl_context *ctx, GLuint program,
GLenum pname, GLint *param
case GL_PROGRAM_BINARY_LENGTH:
*params = 0;
return;
+ case GL_PROGRAM_SEPARABLE:
+ if (!ctx->Extensions.ARB_separate_shader_objects)
+ break;
+ *params = shProg->SeparateShader;
+ return;
default:
break;
}
@@ -1702,6 +1707,25 @@ _mesa_ProgramParameteri(GLuint program, GLenum pname,
GLint value)
*/
shProg->BinaryRetreivableHint = value;
return;
+
+ case GL_PROGRAM_SEPARABLE:
+ if (!ctx->Extensions.ARB_separate_shader_objects)
+ break;
+
+ /* Spec imply that the behavior is the same as ARB_get_program_binary
+ * Chapter 7.3 Program Objects
+ */
+ if (value != GL_TRUE && value != GL_FALSE) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glProgramParameteri(pname=%s, value=%d): "
+ "value must be 0 or 1.",
+ _mesa_lookup_enum_by_nr(pname),
+ value);
+ return;
+ }
+ shProg->SeparateShader = value;
+ return;
+
default:
break;
}
@@ -1773,59 +1797,71 @@ _mesa_ActiveProgramEXT(GLuint program)
return;
}
-
-/**
- * For GL_EXT_separate_shader_objects
- */
-GLuint GLAPIENTRY
-_mesa_CreateShaderProgramEXT(GLenum type, const GLchar *string)
+static GLuint
+_mesa_create_shader_program(struct gl_context* ctx, GLboolean separate,
+GLenum type, GLsizei count, const GLchar* const
*strings)
{
- GET_CURRENT_CONTEXT(ctx);
const GLuint shader = create_shader(ctx, type);
GLuint program = 0;
if (shader) {
- shader_source(ctx, shader, _mesa_strdup(string));
+ _mesa_ShaderSource(shader, count, strings, NULL);
+
compile_shader(ctx, shader);
program = create_shader_program(ctx);
if (program) {
-struct gl_shader_program *shProg;
-struct gl_shader *sh;
-GLint compiled = GL_FALSE;
+ struct gl_shader_program *shProg;
+ struct gl_shader *sh;
+ GLint compiled = GL_FALSE;
-shProg = _mesa_lookup_shader_program(ctx, program);
-sh = _mesa_lookup_shader(ctx, shader);
+ shProg = _mesa_lookup_shader_program(ctx, program);
+ sh = _mesa_lookup_shader(ctx, shader);
-get_shaderiv(ctx, shader, GL_COMPILE_STATUS, &compiled);
-if (compiled) {
- attach_shader(ctx, program, shader);
- link_program(ctx, program);
- detach_shader(ctx, program, shader);
+ shProg->SeparateShader = separate;
+ get_shaderiv(ctx, shader, GL_COMPILE_STATUS, &compiled);
+ if (compiled) {
+attach_shader(ctx, program, shader);
+link_program(ctx, program);
+detach_shader(ctx, program, shader);
#if 0
- /* Possibly... */
- if (active-user-defined-varyings-in-linked-program) {
- append-error-to-info-log;
- shProg->LinkStatus = GL_FALSE;
- }
+/* Possibly... */
+if (active-user-defined-varyings-in-linked-program) {
+ append-error-to-info-log;
+ shProg->LinkStatus = GL_FALSE;
+}
#endif
-}
+ }
-ralloc_strcat(&shProg->InfoLog, sh->InfoLog);
+ ralloc_strcat(&shProg->InfoLog, sh->InfoLog);
}
-
delete_shader(ctx, shader);
}
-
return program;
}
/**
+ * For GL_EXT_separate_shader_objects
+ */
+GLuint GLAPIENTRY
+_mesa_CreateShaderProgramEXT(GLenum type, const GLchar *string)
+{
+ GET_CURRENT_CONTEXT(ctx);
+
+ return _mesa_create_shader_program(ctx, GL_FALSE, type, 1, &string);
+}
+
+/**
* ARB_separate_shader_objects: Compile & Link Program
+ * Bas
[Mesa-dev] [PATCH 04/12] sso: implement ActiveShaderProgram & GetProgramPipelineiv
V2:
* Rename object
* Formatting improvement
---
src/mesa/main/pipelineobj.c | 77 +++
1 file changed, 77 insertions(+)
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index d81bd0e..ffbeeae 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -231,6 +231,30 @@ _mesa_UseProgramStages(GLuint pipeline, GLbitfield stages,
GLuint program)
void GLAPIENTRY
_mesa_ActiveShaderProgram(GLuint pipeline, GLuint program)
{
+ GET_CURRENT_CONTEXT(ctx);
+ struct gl_shader_program *shProg = (program != 0)
+ ? _mesa_lookup_shader_program_err(ctx, program,
"glActiveShaderProgram(program)")
+ : NULL;
+
+ struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
+
+ if (!pipe) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
"glActiveShaderProgram(pipeline)");
+ return;
+ }
+
+ /* Object is created by any Pipeline call but glGenProgramPipelines,
+* glIsProgramPipeline and GetProgramPipelineInfoLog
+*/
+ pipe->EverBound = GL_TRUE;
+
+ if ((shProg != NULL) && !shProg->LinkStatus) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+"glActiveShaderProgram(program %u not linked)", shProg->Name);
+ return;
+ }
+
+ _mesa_reference_shader_program(ctx, &pipe->ActiveProgram, shProg);
}
/**
@@ -348,6 +372,59 @@ _mesa_IsProgramPipeline(GLuint pipeline)
void GLAPIENTRY
_mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname, GLint *params)
{
+ GET_CURRENT_CONTEXT(ctx);
+ struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
+
+ /* Are geometry shaders available in this context?
+*/
+ const bool has_gs = _mesa_is_desktop_gl(ctx) &&
ctx->Extensions.ARB_geometry_shader4;
+
+ if (!pipe) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
"glGetProgramPipelineiv(pipeline)");
+ return;
+ }
+
+ /* Object is created by any Pipeline call but glGenProgramPipelines,
+* glIsProgramPipeline and GetProgramPipelineInfoLog
+*/
+ pipe->EverBound = GL_TRUE;
+
+ switch (pname) {
+ case GL_ACTIVE_PROGRAM:
+ *params = pipe->ActiveProgram ? pipe->ActiveProgram->Name : 0;
+ return;
+ case GL_INFO_LOG_LENGTH:
+ // TODO
+ *params = 0;
+ return;
+ case GL_VALIDATE_STATUS:
+ *params = pipe->ValidationStatus;
+ return;
+ case GL_VERTEX_SHADER:
+ *params = pipe->CurrentVertexProgram ? pipe->CurrentVertexProgram->Name
: 0;
+ return;
+ case GL_TESS_EVALUATION_SHADER:
+ /* NOT YET SUPPORTED */
+ break;
+ case GL_TESS_CONTROL_SHADER:
+ /* NOT YET SUPPORTED */
+ break;
+ case GL_GEOMETRY_SHADER:
+ if (!has_gs) break;
+ *params = pipe->CurrentGeometryProgram ?
pipe->CurrentGeometryProgram->Name : 0;;
+ return;
+ case GL_FRAGMENT_SHADER:
+ *params = pipe->CurrentFragmentProgram ?
pipe->CurrentFragmentProgram->Name : 0;;
+ return;
+ case GL_COMPUTE_SHADER:
+ /* NOT YET SUPPORTED */
+ break;
+ default:
+ break;
+ }
+
+ _mesa_error(ctx, GL_INVALID_ENUM, "glGetProgramPipelineiv(pname=%s)",
+ _mesa_lookup_enum_by_nr(pname));
}
/**
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 06/12] sso: rename Shader to the pointer _Shader
Basically a sed but shaderapi.c and get.c.
get.c => GL_CURRENT_PROGAM always refer to the "old" UseProgram behavior
shaderapi.c => the old api stil update the Shader object directly
V2: formatting improvement
---
src/mesa/drivers/common/meta.c | 10 ++--
src/mesa/drivers/dri/i965/brw_gs.c |2 +-
src/mesa/drivers/dri/i965/brw_shader.cpp |4 +-
src/mesa/drivers/dri/i965/brw_vs.c |4 +-
src/mesa/drivers/dri/i965/brw_vs_surface_state.c |2 +-
src/mesa/drivers/dri/i965/brw_wm.c |2 +-
src/mesa/drivers/dri/i965/brw_wm_surface_state.c |2 +-
src/mesa/drivers/dri/i965/gen6_sol.c |6 +-
src/mesa/drivers/dri/i965/gen6_vs_state.c|2 +-
src/mesa/drivers/dri/i965/gen6_wm_state.c|2 +-
src/mesa/drivers/dri/i965/gen7_sol_state.c |4 +-
src/mesa/drivers/dri/i965/gen7_vs_state.c|2 +-
src/mesa/drivers/dri/i965/gen7_wm_state.c|2 +-
src/mesa/main/api_validate.c |2 +-
src/mesa/main/context.c | 32 +--
src/mesa/main/ff_fragment_shader.cpp |8 +--
src/mesa/main/get.c | 10
src/mesa/main/shaderapi.c| 26 -
src/mesa/main/state.c| 14 ++---
src/mesa/main/texstate.c | 12 ++--
src/mesa/main/transformfeedback.c|4 +-
src/mesa/main/uniform_query.cpp |4 +-
src/mesa/main/uniforms.c | 66 +++---
src/mesa/program/ir_to_mesa.cpp | 12 ++--
src/mesa/state_tracker/st_atom_clip.c|2 +-
src/mesa/state_tracker/st_atom_constbuf.c|4 +-
src/mesa/state_tracker/st_cb_drawpixels.c|2 +-
src/mesa/state_tracker/st_draw.c |6 +-
src/mesa/state_tracker/st_glsl_to_tgsi.cpp |2 +-
src/mesa/state_tracker/st_program.c |6 +-
src/mesa/swrast/s_fragprog.c |2 +-
31 files changed, 134 insertions(+), 124 deletions(-)
diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
index e3ab82b..927573d 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -617,13 +617,13 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield state)
if (ctx->Extensions.ARB_shader_objects) {
_mesa_reference_shader_program(ctx, &save->VertexShader,
- ctx->Shader.CurrentVertexProgram);
+ ctx->_Shader->CurrentVertexProgram);
_mesa_reference_shader_program(ctx, &save->GeometryShader,
- ctx->Shader.CurrentGeometryProgram);
+ ctx->_Shader->CurrentGeometryProgram);
_mesa_reference_shader_program(ctx, &save->FragmentShader,
- ctx->Shader.CurrentFragmentProgram);
+ ctx->_Shader->CurrentFragmentProgram);
_mesa_reference_shader_program(ctx, &save->ActiveShader,
- ctx->Shader.ActiveProgram);
+ ctx->_Shader->ActiveProgram);
_mesa_UseProgram(0);
}
@@ -965,7 +965,7 @@ _mesa_meta_end(struct gl_context *ctx)
_mesa_use_shader_program(ctx, GL_FRAGMENT_SHADER,
save->FragmentShader);
- _mesa_reference_shader_program(ctx, &ctx->Shader.ActiveProgram,
+ _mesa_reference_shader_program(ctx, &ctx->_Shader->ActiveProgram,
save->ActiveShader);
_mesa_reference_shader_program(ctx, &save->VertexShader, NULL);
diff --git a/src/mesa/drivers/dri/i965/brw_gs.c
b/src/mesa/drivers/dri/i965/brw_gs.c
index caa3b3e..31b01df 100644
--- a/src/mesa/drivers/dri/i965/brw_gs.c
+++ b/src/mesa/drivers/dri/i965/brw_gs.c
@@ -189,7 +189,7 @@ static void populate_key( struct brw_context *brw,
/* _NEW_TRANSFORM_FEEDBACK */
if (_mesa_is_xfb_active_and_unpaused(ctx)) {
const struct gl_shader_program *shaderprog =
-ctx->Shader.CurrentVertexProgram;
+ctx->_Shader->CurrentVertexProgram;
const struct gl_transform_feedback_info *linked_xfb_info =
&shaderprog->LinkedTransformFeedback;
int i;
diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp
b/src/mesa/drivers/dri/i965/brw_shader.cpp
index b3bd1b9..7259fde 100644
--- a/src/mesa/drivers/dri/i965/brw_shader.cpp
+++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
@@ -257,7 +257,7 @@ brw_link_shader(struct gl_context *ctx, struct
gl_shader_program *shProg)
_mesa_reference_program(ctx, &prog, NULL);
- if (ctx->Shader.Flags & GLSL_DUMP) {
+ if (ctx->_Shader->Flags & GLSL_DUMP) {
printf("\n");
pri
[Mesa-dev] [PATCH 07/12] sso: update meta state
save and restore _Shader/Pipeline binding point. Rational we don't want any
conflict
when the program will be unattached.
V2: formatting improvement
---
src/mesa/drivers/common/meta.c | 28 +---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
index 927573d..01a63bd 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -51,6 +51,7 @@
#include "main/macros.h"
#include "main/matrix.h"
#include "main/mipmap.h"
+#include "main/pipelineobj.h"
#include "main/pixel.h"
#include "main/pbo.h"
#include "main/polygon.h"
@@ -142,6 +143,8 @@ struct save_state
struct gl_shader_program *GeometryShader;
struct gl_shader_program *FragmentShader;
struct gl_shader_program *ActiveShader;
+ struct gl_pipeline_object *_Shader;
+ struct gl_pipeline_object *Pipeline;
/** MESA_META_STENCIL_TEST */
struct gl_stencil_attrib Stencil;
@@ -615,6 +618,14 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield state)
_mesa_set_enable(ctx, GL_FRAGMENT_SHADER_ATI, GL_FALSE);
}
+ if (ctx->Extensions.ARB_separate_shader_objects) {
+ /* Warning it must be done before _mesa_UseProgram call */
+ _mesa_reference_pipeline_object(ctx, &save->_Shader, ctx->_Shader);
+ _mesa_reference_pipeline_object(ctx, &save->Pipeline,
+ ctx->Pipeline.Current);
+ _mesa_BindProgramPipeline(0);
+ }
+
if (ctx->Extensions.ARB_shader_objects) {
_mesa_reference_shader_program(ctx, &save->VertexShader,
ctx->_Shader->CurrentVertexProgram);
@@ -954,16 +965,26 @@ _mesa_meta_end(struct gl_context *ctx)
save->ATIFragmentShaderEnabled);
}
+ /* Warning it must be done before _mesa_use_shader_program call */
+ if (ctx->Extensions.ARB_separate_shader_objects) {
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, save->_Shader);
+ _mesa_reference_pipeline_object(ctx, &ctx->Pipeline.Current,
+ save->Pipeline);
+ _mesa_reference_pipeline_object(ctx, &save->Pipeline, NULL);
+ }
+
if (ctx->Extensions.ARB_vertex_shader)
-_mesa_use_shader_program(ctx, GL_VERTEX_SHADER, save->VertexShader);
+_mesa_use_shader_program(ctx, GL_VERTEX_SHADER, save->VertexShader,
+ ctx->_Shader);
if (ctx->Extensions.ARB_geometry_shader4)
_mesa_use_shader_program(ctx, GL_GEOMETRY_SHADER_ARB,
- save->GeometryShader);
+ save->GeometryShader, ctx->_Shader);
if (ctx->Extensions.ARB_fragment_shader)
_mesa_use_shader_program(ctx, GL_FRAGMENT_SHADER,
- save->FragmentShader);
+ save->FragmentShader, ctx->_Shader);
+
_mesa_reference_shader_program(ctx, &ctx->_Shader->ActiveProgram,
save->ActiveShader);
@@ -972,6 +993,7 @@ _mesa_meta_end(struct gl_context *ctx)
_mesa_reference_shader_program(ctx, &save->GeometryShader, NULL);
_mesa_reference_shader_program(ctx, &save->FragmentShader, NULL);
_mesa_reference_shader_program(ctx, &save->ActiveShader, NULL);
+ _mesa_reference_pipeline_object(ctx, &save->_Shader, NULL);
}
if (state & MESA_META_STENCIL_TEST) {
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 05/12] sso: replace Shader binding point with _Shader
To avoid NULL pointer check a default pipeline object is installed in _Shader
when no
program is current
The spec say that UseProgram/UseShaderProgramEXT/ActiveProgramEXT got an higher
priority over the pipeline object. When default program is uninstall, the
pipeline is
used if any was bound.
Note: A careful rename need to be done now...
V2: formating improvement
---
src/mesa/main/mtypes.h |5 +++
src/mesa/main/pipelineobj.c |8
src/mesa/main/shaderapi.c | 87 +--
3 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index f979cd0..adf518b 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2434,6 +2434,9 @@ struct gl_pipeline_shader_state
/** Currently bound pipeline object. See _mesa_BindProgramPipeline() */
struct gl_pipeline_object *Current;
+ /* Default Object to ensure that _Shader is never NULL */
+ struct gl_pipeline_object *Default;
+
/** Pipeline objects */
struct _mesa_HashTable *Objects;
};
@@ -3546,6 +3549,8 @@ struct gl_context
struct gl_pipeline_shader_state Pipeline; /**< GLSL pipeline shader object
state */
struct gl_pipeline_object Shader; /**< GLSL shader object state */
+ struct gl_pipeline_object *_Shader; /**< Points to ::Shader or
::Pipeline.Current
+ or ::Pipeline.Default */
struct gl_shader_compiler_options ShaderCompilerOptions[MESA_SHADER_TYPES];
struct gl_query_state Query; /**< occlusion, timer queries */
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index ffbeeae..5add307 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -95,6 +95,10 @@ _mesa_init_pipeline(struct gl_context *ctx)
ctx->Pipeline.Objects = _mesa_NewHashTable();
ctx->Pipeline.Current = NULL;
+
+ /* Install a default Pipeline */
+ ctx->Pipeline.Default = _mesa_new_pipeline_object(ctx, 0);
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, ctx->Pipeline.Default);
}
@@ -118,6 +122,10 @@ _mesa_free_pipeline_data(struct gl_context *ctx)
{
_mesa_HashDeleteAll(ctx->Pipeline.Objects, delete_pipelineobj_cb, ctx);
_mesa_DeleteHashTable(ctx->Pipeline.Objects);
+
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, NULL);
+ _mesa_delete_pipeline_object(ctx, ctx->Pipeline.Default);
+
}
/**
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 46072ba..c25f49f 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -43,6 +43,7 @@
#include "main/hash.h"
#include "main/mfeatures.h"
#include "main/mtypes.h"
+#include "main/pipelineobj.h"
#include "main/shaderapi.h"
#include "main/shaderobj.h"
#include "main/transformfeedback.h"
@@ -138,6 +139,8 @@ _mesa_free_shader_state(struct gl_context *ctx)
_mesa_reference_shader_program(ctx, &ctx->Shader.ActiveProgram, NULL);
/* Extended for ARB_separate_shader_objects */
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, NULL);
+
assert(ctx->Shader.RefCount == 1);
_glthread_DESTROY_MUTEX(ctx->Shader.Mutex);
}
@@ -1455,7 +1458,29 @@ _mesa_UseProgram(GLhandleARB program)
shProg = NULL;
}
- _mesa_use_program(ctx, shProg);
+ /*
+* The executable code for an individual shader stage is taken from the
+* current program for that stage. If there is a current program object
+* for any shader stage or for uniform updates established by UseProgram,
+* UseShaderProgramEXT, or ActiveProgramEXT, the current program for that
+* stage (if any) is considered current. Otherwise, if there is a bound
+* program pipeline object ...
+*/
+ if (program) {
+ /* Attach shader state to the binding point */
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, &ctx->Shader);
+ /* Update the program */
+ _mesa_use_program(ctx, shProg);
+ } else {
+ /* Must be done first: detach the progam */
+ _mesa_use_program(ctx, shProg);
+ /* Unattach shader_state binding point */
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
ctx->Pipeline.Default);
+ /* If a pipeline was bound, rebind it */
+ if (ctx->Pipeline.Current) {
+ _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
+ }
+ }
}
@@ -1778,7 +1803,35 @@ _mesa_UseShaderProgramEXT(GLenum type, GLuint program)
}
}
- _mesa_use_shader_program(ctx, type, shProg);
+ /*
+* The executable code for an individual shader stage is taken from the
+* current program for that stage. If there is a current program object
+* for any shader stage or for uniform updates established by UseProgram,
+* UseShaderProgramEXT, or ActiveProgramEXT, the current program for that
+* stage (if any) is considered current. Otherwise, if there is a bound
+* program pipeline object ...
+*/
+ if (program) {
+ /* Attach shader state to the binding
[Mesa-dev] [PATCH 08/12] sso: Implement _mesa_UseProgramStages
Implement _mesa_UseProgramStages =>
arb_separate_shader_object-GetProgramPipelineiv is now pass :)
Extend use_shader_program to support a different target. Allow to reuse the
function to update
the pipeline state. Note I bypass the flush when target isn't current. Maybe it
would be better to
create a new UseProgramStages driver function
V2: formatting & rename
---
src/mesa/main/pipelineobj.c | 111 +++
src/mesa/main/shaderapi.c | 32 +++--
src/mesa/main/shaderapi.h |3 +-
3 files changed, 131 insertions(+), 15 deletions(-)
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index 5add307..3cdca63 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -33,6 +33,8 @@
* XXX things to do:
* 1/ Do we need to create 2 new drivers functions: CreatePipelineObject
* DeletePipelineObject
+ * 2/ We probably need a UseProgramStages driver function. It would avoir to
+ * dirty all stages
*/
#include "main/glheader.h"
@@ -231,6 +233,115 @@ _mesa_reference_pipeline_object_(struct gl_context *ctx,
void GLAPIENTRY
_mesa_UseProgramStages(GLuint pipeline, GLbitfield stages, GLuint program)
{
+ GET_CURRENT_CONTEXT(ctx);
+
+ struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
+ struct gl_shader_program *shProg = NULL;
+
+ if (!pipe) {
+ _mesa_error(ctx, GL_INVALID_OPERATION, "glUseProgramStages(pipeline)");
+ return;
+ }
+
+ /* Object is created by any Pipeline call but glGenProgramPipelines,
+* glIsProgramPipeline and GetProgramPipelineInfoLog
+*/
+ pipe->EverBound = GL_TRUE;
+
+ /* NOT YET SUPPORTED:
+* GL_TESS_CONTROL_SHADER_BIT
+* GL_TESS_EVALUATION_SHADER_BIT
+* GL_COMPUTE_SHADER_BIT
+*/
+ GLbitfield any_valid_stages = GL_VERTEX_SHADER_BIT | GL_FRAGMENT_SHADER_BIT;
+ if (_mesa_is_desktop_gl(ctx) && ctx->Extensions.ARB_geometry_shader4)
+ any_valid_stages |= GL_GEOMETRY_SHADER_BIT;
+
+ if (stages != GL_ALL_SHADER_BITS && (stages & ~any_valid_stages) != 0) {
+ _mesa_error(ctx, GL_INVALID_VALUE, "glUseProgramStages(Stages)");
+ return;
+ }
+
+ /*
+* An INVALID_OPERATION error is generated :
+* by UseProgramStages if the program pipeline object it refers to is
current
+* and the current transform feedback object is active and not paused;
+*/
+ /*
+* 6a. Should the fragment shader program object be allowed to changed
+* within transform feedback mode?
+* RESOLVED: No, this should generate an GL_INVALID_OPERATION error.
+*/
+ if (ctx->_Shader == pipe) {
+ if (_mesa_is_xfb_active_and_unpaused(ctx)) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "glUseProgramStages(transform feedback active)");
+ return;
+ }
+ }
+
+ if (program) {
+ /* An INVALID_OPERATION error is generated if program is the name of a
+ * shader object
+ */
+ struct gl_shader *sh = _mesa_lookup_shader(ctx, program);
+ if (sh != NULL) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "glUseProgramStages(progam is a shader object)");
+ return;
+ }
+
+ /* An INVALID_VALUE error is generated if program is not the name of ei-
+ * ther a program or shader object
+ */
+ shProg = _mesa_lookup_shader_program(ctx, program);
+ if (shProg == NULL) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glUseProgramStages(progam is not a program object)");
+ return;
+ }
+
+ /* An INVALID_OPERATION error is generated if the program object named
+ * by program was linked without the PROGRAM_SEPARABLE parameter set, has
+ * not been linked, or was last linked unsuccessfully. The corresponding
shader
+ * stages in pipeline are not modified.
+ */
+ if (!shProg->LinkStatus) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "glUseProgramStages(program not linked)");
+ return;
+ }
+ if (!shProg->SeparateShader) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "glUseProgramStages(program wasn't linked with the
PROGRAM_SEPARABLE flag)");
+ return;
+ }
+ }
+
+ /*
+* 7. What happens if you have a program object current for a shader
stage,
+*but the program object doesn't contain an executable for that stage?
+
+*RESOLVED: This is not an error; instead it is as though there were no
+*program bound to that stage. We have two different notions for
+*programs bound to shader stages. A program is "current" for a stage
+*if it bound to that stage in the active program pipeline object. A
+*program is "active" for a stage if it is current and it has an
+*executable for this stage. In this case, the program would be current
+*but not active.
+
+*When no program is active for a stage, the stage will be replaced
[Mesa-dev] [PATCH 09/12] sso: implement BindProgramPipeline
Test become green in piglit:
The updated ext_transform_feedback-api-errors:useprogstage_noactive
useprogstage_active bind_pipeline
arb_separate_shader_object-GetProgramPipelineiv
arb_separate_shader_object-IsProgramPipeline
For the moment I reuse Driver.UseProgram but I guess it will be better to create
a UseProgramStages functions. Opinion is welcome
V2: formatting & rename
---
src/mesa/main/pipelineobj.c | 60 +++
1 file changed, 60 insertions(+)
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index 3cdca63..2d18192 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -35,6 +35,10 @@
* DeletePipelineObject
* 2/ We probably need a UseProgramStages driver function. It would avoir to
* dirty all stages
+ * 3/ Maybe create a bind pipeline driver function to notify that all
+ * program was updated. Currently I call UseProgram with NULL because there
isn't
+ * a single program
+ * 4/ When vertices need to be flushed (FLUSH_VERTICES)
*/
#include "main/glheader.h"
@@ -382,6 +386,62 @@ _mesa_ActiveShaderProgram(GLuint pipeline, GLuint program)
void GLAPIENTRY
_mesa_BindProgramPipeline(GLuint pipeline)
{
+ GET_CURRENT_CONTEXT(ctx);
+ struct gl_pipeline_object *newObj = NULL;
+
+ if (ctx->_Shader->Name == pipeline)
+ return; /* rebinding the same pipeline object- no change */
+
+ /*
+* An INVALID_OPERATION error is generated :
+* by BindProgramPipeline if the current transform feedback object is
active
+* and not paused;
+*/
+ if (_mesa_is_xfb_active_and_unpaused(ctx)) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+"glBindProgramPipeline(transform feedback active)");
+ return;
+ }
+
+ /*
+* Get pointer to new pipeline object (newObj)
+*/
+ if (pipeline) {
+ /* non-default pipeline object */
+ newObj = lookup_pipeline_object(ctx, pipeline);
+ if (!newObj) {
+ _mesa_error(ctx, GL_INVALID_OPERATION, "glBindProgramPipeline(non-gen
name)");
+ return;
+ }
+
+ /* Object is created by any Pipeline call but glGenProgramPipelines,
+ * glIsProgramPipeline and GetProgramPipelineInfoLog
+ */
+ newObj->EverBound = GL_TRUE;
+ }
+
+ /* First bind the Pipeline to pipeline binding point */
+ _mesa_reference_pipeline_object(ctx, &ctx->Pipeline.Current, newObj);
+
+ /* Spec say:
+* if any program is bound to the context, the current pipeline object is
+* ignored.
+*/
+ if (&ctx->Shader != ctx->_Shader) {
+ if (pipeline) {
+ /* Bound the pipeline to the current program and
+ * restore the pipeline state
+ */
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader, newObj);
+ } else {
+ /* Unbind the pipeline */
+ _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
ctx->Pipeline.Default);
+ }
+ FLUSH_VERTICES(ctx, _NEW_PROGRAM | _NEW_PROGRAM_CONSTANTS);
+ /* FIXME */
+ if (ctx->Driver.UseProgram)
+ ctx->Driver.UseProgram(ctx, NULL);
+ }
}
/**
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 10/12] sso: update glGet: GL_PROGRAM_PIPELINE_BINDING
---
src/mesa/main/get.c |9 +
src/mesa/main/get_hash_params.py |3 +++
2 files changed, 12 insertions(+)
diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
index 54159c0..6cbb7db 100644
--- a/src/mesa/main/get.c
+++ b/src/mesa/main/get.c
@@ -369,6 +369,7 @@ EXTRA_EXT(ARB_map_buffer_alignment);
EXTRA_EXT(ARB_texture_cube_map_array);
EXTRA_EXT(ARB_texture_buffer_range);
EXTRA_EXT(ARB_texture_multisample);
+EXTRA_EXT(ARB_separate_shader_objects);
static const int
extra_ARB_color_buffer_float_or_glcore[] = {
@@ -889,6 +890,14 @@ find_custom_value(struct gl_context *ctx, const struct
value_desc *d, union valu
_mesa_problem(ctx, "driver doesn't implement GetTimestamp");
}
break;
+ /* GL_ARB_separate_shader_objects */
+ case GL_PROGRAM_PIPELINE_BINDING:
+ if (ctx->Pipeline.Current) {
+ v->value_int = ctx->Pipeline.Current->Name;
+ } else {
+ v->value_int = 0;
+ }
+ break;
}
}
diff --git a/src/mesa/main/get_hash_params.py b/src/mesa/main/get_hash_params.py
index 2b97da6..43a11cf 100644
--- a/src/mesa/main/get_hash_params.py
+++ b/src/mesa/main/get_hash_params.py
@@ -709,6 +709,9 @@ descriptor=[
# GL_ARB_texture_cube_map_array
[ "TEXTURE_BINDING_CUBE_MAP_ARRAY_ARB", "LOC_CUSTOM, TYPE_INT,
TEXTURE_CUBE_ARRAY_INDEX, extra_ARB_texture_cube_map_array" ],
+
+# GL_ARB_separate_shader_objects
+ [ "PROGRAM_PIPELINE_BINDING", "LOC_CUSTOM, TYPE_INT,
GL_PROGRAM_PIPELINE_BINDING, extra_ARB_separate_shader_objects" ],
]},
# Enums restricted to OpenGL Core profile
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 11/12] sso: implement ValidateProgramPipeline and GetProgramPipelineInfoLog
Implementation note:
I don't use context for ralloc (don't know how).
The check on PROGRAM_SEPARABLE flags is also done when the pipeline isn't bound.
It doesn't make any sense in a DSA style API.
Maybe we could replace _mesa_validate_program_pipeline by
_mesa_validate_program_pipeline.
For example we could recreate a dummy pipeline object. However the new function
checks
also the TEXTURE_IMAGE_UNIT number not sure of the impact.
V2:
Fix memory leak with ralloc_strdup
Formatting improvement
---
src/mesa/main/context.c |9 ++
src/mesa/main/mtypes.h |2 +
src/mesa/main/pipelineobj.c | 221 ++-
src/mesa/main/pipelineobj.h |3 +
src/mesa/main/uniform_query.cpp | 71 +
src/mesa/main/uniforms.h|3 +
6 files changed, 305 insertions(+), 4 deletions(-)
diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 6a0619a..559e21f 100644
--- a/src/mesa/main/context.c
+++ b/src/mesa/main/context.c
@@ -1767,6 +1767,7 @@ _mesa_check_blend_func_error(struct gl_context *ctx)
* Prior to drawing anything with glBegin, glDrawArrays, etc. this function
* is called to see if it's valid to render. This involves checking that
* the current shader is valid and the framebuffer is complete.
+ * It also check the current pipeline object is valid if any.
* If an error is detected it'll be recorded here.
* \return GL_TRUE if OK to render, GL_FALSE if not
*/
@@ -1876,6 +1877,14 @@ _mesa_valid_to_render(struct gl_context *ctx, const char
*where)
}
}
+ /* A pipeline object is bound */
+ if (ctx->_Shader->Name && !ctx->_Shader->Validated) {
+ /* Error message will be printed inside _mesa_validate_program_pipeline
*/
+ if (!_mesa_validate_program_pipeline(ctx, ctx->_Shader, GL_TRUE)) {
+ return GL_FALSE;
+ }
+ }
+
if (ctx->DrawBuffer->_Status != GL_FRAMEBUFFER_COMPLETE_EXT) {
_mesa_error(ctx, GL_INVALID_FRAMEBUFFER_OPERATION_EXT,
"%s(incomplete framebuffer)", where);
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index adf518b..ce88860 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2424,6 +2424,8 @@ struct gl_pipeline_object
GLboolean Validated; /**< Pipeline Validation status */
GLboolean EverBound; /**< Has the pipeline object been
created */
+
+ GLchar *InfoLog;
};
/**
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index 2d18192..d7948ff 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -87,6 +87,7 @@ _mesa_new_pipeline_object(struct gl_context *ctx, GLuint name)
_glthread_INIT_MUTEX(obj->Mutex);
obj->RefCount = 1;
obj->Flags = _mesa_get_shader_flags();
+ obj->InfoLog = ralloc_strdup(obj, "");
}
return obj;
@@ -339,13 +340,15 @@ _mesa_UseProgramStages(GLuint pipeline, GLbitfield
stages, GLuint program)
*disabled (tessellation control and evaluation, geometry), or have
*undefined results (core profile vertex and fragment).
*/
-
if (stages & GL_VERTEX_SHADER_BIT)
_mesa_use_shader_program(ctx, GL_VERTEX_SHADER, shProg, pipe);
if (stages & GL_FRAGMENT_SHADER_BIT)
_mesa_use_shader_program(ctx, GL_FRAGMENT_SHADER, shProg, pipe);
if (stages & GL_GEOMETRY_SHADER_BIT)
_mesa_use_shader_program(ctx, GL_GEOMETRY_SHADER_ARB, shProg, pipe);
+
+ /* Validation would need to be redone */
+ pipe->Validated = GL_FALSE;
}
/**
@@ -573,11 +576,10 @@ _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname,
GLint *params)
*params = pipe->ActiveProgram ? pipe->ActiveProgram->Name : 0;
return;
case GL_INFO_LOG_LENGTH:
- // TODO
- *params = 0;
+ *params = pipe->Validated;
return;
case GL_VALIDATE_STATUS:
- *params = pipe->ValidationStatus;
+ *params = pipe->Validated;
return;
case GL_VERTEX_SHADER:
*params = pipe->CurrentVertexProgram ? pipe->CurrentVertexProgram->Name
: 0;
@@ -606,16 +608,227 @@ _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum
pname, GLint *params)
_mesa_lookup_enum_by_nr(pname));
}
+static GLboolean
+ProgramEnabledEverywhere(struct gl_pipeline_object *pipe,
+ struct gl_shader_program *prog,
+ char *errMsg, size_t errMsgLength)
+{
+ if (!prog) return GL_TRUE;
+
+ GLboolean status = GL_TRUE;
+
+ if (prog->_LinkedShaders[MESA_SHADER_VERTEX]) {
+ if (pipe->CurrentVertexProgram) {
+ if (prog->Name != pipe->CurrentVertexProgram->Name) {
+status = GL_FALSE;
+ }
+ } else {
+ status = GL_FALSE;
+ }
+ }
+
+ if (prog->_LinkedShaders[MESA_SHADER_FRAGMENT]) {
+ if (pipe->CurrentFragmentProgram) {
+ if (prog->Name != pipe->CurrentFragmentProgram->Name) {
+status = GL_FALSE;
+ }
+ } else {
+
[Mesa-dev] [PATCH 12/12] sso: Finally enable the extension on Gallium
Note: it probably work on others drivers. --- src/mesa/state_tracker/st_extensions.c |1 + 1 file changed, 1 insertion(+) diff --git a/src/mesa/state_tracker/st_extensions.c b/src/mesa/state_tracker/st_extensions.c index f986480..4ce74f2 100644 --- a/src/mesa/state_tracker/st_extensions.c +++ b/src/mesa/state_tracker/st_extensions.c @@ -519,6 +519,7 @@ void st_init_extensions(struct st_context *st) ctx->Extensions.ARB_half_float_vertex = GL_TRUE; ctx->Extensions.ARB_internalformat_query = GL_TRUE; ctx->Extensions.ARB_map_buffer_range = GL_TRUE; + ctx->Extensions.ARB_separate_shader_objects = GL_TRUE; ctx->Extensions.ARB_shader_objects = GL_TRUE; ctx->Extensions.ARB_shading_language_100 = GL_TRUE; ctx->Extensions.ARB_texture_border_clamp = GL_TRUE; /* XXX temp */ -- 1.7.10.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/12] sso: update glGet: GL_PROGRAM_PIPELINE_BINDING
On Fri, 3 May 2013 12:04:48 -0700
Matt Turner wrote:
> On Fri, May 3, 2013 at 10:44 AM, Gregory Hainaut
> wrote:
> > ---
> > src/mesa/main/get.c |9 +
> > src/mesa/main/get_hash_params.py |3 +++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > index 54159c0..6cbb7db 100644
> > --- a/src/mesa/main/get.c
> > +++ b/src/mesa/main/get.c
> > @@ -369,6 +369,7 @@ EXTRA_EXT(ARB_map_buffer_alignment);
> > EXTRA_EXT(ARB_texture_cube_map_array);
> > EXTRA_EXT(ARB_texture_buffer_range);
> > EXTRA_EXT(ARB_texture_multisample);
> > +EXTRA_EXT(ARB_separate_shader_objects);
> >
> > static const int
> > extra_ARB_color_buffer_float_or_glcore[] = {
> > @@ -889,6 +890,14 @@ find_custom_value(struct gl_context *ctx, const struct
> > value_desc *d, union valu
> > _mesa_problem(ctx, "driver doesn't implement GetTimestamp");
> >}
> >break;
> > + /* GL_ARB_separate_shader_objects */
> > + case GL_PROGRAM_PIPELINE_BINDING:
> > + if (ctx->Pipeline.Current) {
> > + v->value_int = ctx->Pipeline.Current->Name;
> > + } else {
> > + v->value_int = 0;
> > + }
> > + break;
> > }
> > }
>
> This looks believable, but I can't find a description in the extension
> spec or GL 4.1+ specs that say precisely what this query is supposed
> to do. Looks like it's just mentioned in the extension spec, and not
> at all in GL 4.1+ specs.
Yes you're right that strange. There is also a couple of line in glGet man page.
GL_PROGRAM_PIPELINE_BINDING
params a single value, the name of the currently bound
program pipeline
object, or zero if no program pipeline object is bound.
See glBindProgramPipeline.
Both Nvidia and AMD support this query. I did a quick update on my piglit test,
on the AMD side:
* UseProgram(2)
* BindPipeline(5) (the pipeline isn't really bound because UseProgram got an
higher priority)
* Get GL_PROGRAM_PIPELINE_BINDING => 5
I will try to check the behavior on Nvidia implementation.
> > diff --git a/src/mesa/main/get_hash_params.py
> > b/src/mesa/main/get_hash_params.py
> > index 2b97da6..43a11cf 100644
> > --- a/src/mesa/main/get_hash_params.py
> > +++ b/src/mesa/main/get_hash_params.py
> > @@ -709,6 +709,9 @@ descriptor=[
> >
> > # GL_ARB_texture_cube_map_array
> >[ "TEXTURE_BINDING_CUBE_MAP_ARRAY_ARB", "LOC_CUSTOM, TYPE_INT,
> > TEXTURE_CUBE_ARRAY_INDEX, extra_ARB_texture_cube_map_array" ],
> > +
> > +# GL_ARB_separate_shader_objects
> > + [ "PROGRAM_PIPELINE_BINDING", "LOC_CUSTOM, TYPE_INT,
> > GL_PROGRAM_PIPELINE_BINDING, extra_ARB_separate_shader_objects" ],
> > ]},
> >
> > # Enums restricted to OpenGL Core profile
> > --
> > 1.7.10.4
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/12] sso: implement ActiveShaderProgram & GetProgramPipelineiv
On Fri, 3 May 2013 11:56:39 -0700
Matt Turner wrote:
> On Fri, May 3, 2013 at 10:44 AM, Gregory Hainaut
> wrote:
> > V2:
> > * Rename object
> > * Formatting improvement
> > ---
> > src/mesa/main/pipelineobj.c | 77
> > +++
> > 1 file changed, 77 insertions(+)
> >
> > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> > index d81bd0e..ffbeeae 100644
> > --- a/src/mesa/main/pipelineobj.c
> > +++ b/src/mesa/main/pipelineobj.c
> > @@ -231,6 +231,30 @@ _mesa_UseProgramStages(GLuint pipeline, GLbitfield
> > stages, GLuint program)
> > void GLAPIENTRY
> > _mesa_ActiveShaderProgram(GLuint pipeline, GLuint program)
> > {
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_shader_program *shProg = (program != 0)
> > + ? _mesa_lookup_shader_program_err(ctx, program,
> > "glActiveShaderProgram(program)")
> > + : NULL;
> > +
> > + struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
> > +
> > + if (!pipe) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > "glActiveShaderProgram(pipeline)");
> > + return;
> > + }
> > +
> > + /* Object is created by any Pipeline call but glGenProgramPipelines,
> > +* glIsProgramPipeline and GetProgramPipelineInfoLog
> > +*/
> > + pipe->EverBound = GL_TRUE;
> > +
> > + if ((shProg != NULL) && !shProg->LinkStatus) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > +"glActiveShaderProgram(program %u not linked)", shProg->Name);
> > + return;
> > + }
> > +
> > + _mesa_reference_shader_program(ctx, &pipe->ActiveProgram, shProg);
> > }
> >
> > /**
> > @@ -348,6 +372,59 @@ _mesa_IsProgramPipeline(GLuint pipeline)
> > void GLAPIENTRY
> > _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname, GLint *params)
> > {
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
> > +
> > + /* Are geometry shaders available in this context?
> > +*/
> > + const bool has_gs = _mesa_is_desktop_gl(ctx) &&
> > ctx->Extensions.ARB_geometry_shader4;
>
> The geometry shaders in GL 3.2 aren't exactly the same as in
> ARB_geometry_shader4, so it's conceivable that we could support 3.2
> without ARB_geometry_shader4. Probably should be changed to
>
> const bool has_gs = _mesa_is_desktop_gl(ctx) && (ctx->Version >= 32 ||
> ctx->Extensions.ARB_geometry_shader4);
>
> > +
> > + if (!pipe) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > "glGetProgramPipelineiv(pipeline)");
> > + return;
> > + }
> > +
> > + /* Object is created by any Pipeline call but glGenProgramPipelines,
> > +* glIsProgramPipeline and GetProgramPipelineInfoLog
> > +*/
> > + pipe->EverBound = GL_TRUE;
> > +
> > + switch (pname) {
> > + case GL_ACTIVE_PROGRAM:
> > + *params = pipe->ActiveProgram ? pipe->ActiveProgram->Name : 0;
> > + return;
> > + case GL_INFO_LOG_LENGTH:
> > + // TODO
> > + *params = 0;
> > + return;
> > + case GL_VALIDATE_STATUS:
> > + *params = pipe->ValidationStatus;
> > + return;
> > + case GL_VERTEX_SHADER:
> > + *params = pipe->CurrentVertexProgram ?
> > pipe->CurrentVertexProgram->Name : 0;
> > + return;
> > + case GL_TESS_EVALUATION_SHADER:
> > + /* NOT YET SUPPORTED */
> > + break;
> > + case GL_TESS_CONTROL_SHADER:
> > + /* NOT YET SUPPORTED */
> > + break;
> > + case GL_GEOMETRY_SHADER:
> > + if (!has_gs) break;
> > + *params = pipe->CurrentGeometryProgram ?
> > pipe->CurrentGeometryProgram->Name : 0;;
> > + return;
> > + case GL_FRAGMENT_SHADER:
> > + *params = pipe->CurrentFragmentProgram ?
> > pipe->CurrentFragmentProgram->Name : 0;;
>
> Double ; at the ends of these two lines.
>
> > + return;
> > + case GL_COMPUTE_SHADER:
> > + /* NOT YET SUPPORTED */
> > + break;
>
> GL_COMPUTE_SHADER isn't valid, even in GL 4.3 where compute shaders
> are part of core.
>
Feel like they forget to update the spec when they add this new stage. I will
remove it. Thanks.
> > + default:
> > + break;
> > + }
>
> The spec
Re: [Mesa-dev] [PATCH 04/12] sso: implement ActiveShaderProgram & GetProgramPipelineiv
On Sat, 04 May 2013 12:37:05 -0700
Kenneth Graunke wrote:
> On 05/03/2013 10:44 AM, Gregory Hainaut wrote:
> > V2:
> > * Rename object
> > * Formatting improvement
> > ---
> > src/mesa/main/pipelineobj.c | 77
> > +++
> > 1 file changed, 77 insertions(+)
> >
> > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> > index d81bd0e..ffbeeae 100644
> > --- a/src/mesa/main/pipelineobj.c
> > +++ b/src/mesa/main/pipelineobj.c
> > @@ -231,6 +231,30 @@ _mesa_UseProgramStages(GLuint pipeline, GLbitfield
> > stages, GLuint program)
> > void GLAPIENTRY
> > _mesa_ActiveShaderProgram(GLuint pipeline, GLuint program)
> > {
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_shader_program *shProg = (program != 0)
> > + ? _mesa_lookup_shader_program_err(ctx, program,
> > "glActiveShaderProgram(program)")
> > + : NULL;
> > +
> > + struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
> > +
> > + if (!pipe) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > "glActiveShaderProgram(pipeline)");
> > + return;
> > + }
> > +
> > + /* Object is created by any Pipeline call but glGenProgramPipelines,
> > +* glIsProgramPipeline and GetProgramPipelineInfoLog
> > +*/
> > + pipe->EverBound = GL_TRUE;
>
> This doesn't seem right to me. I found this section of the spec:
>
> """
> A program pipeline object is created by binding a name returned by
> GenProgramPipelines with the command
>
> void BindProgramPipeline(uint pipeline);
>
> is the program pipeline object name. The resulting program
> pipeline object is a new state vector, comprising ACTIVE_PROGRAM,
> VERTEX_SHADER, GEOMETRY_SHADER, FRAGMENT_SHADER, TESS_CONTROL_SHADER,
> and TESS_EVALUATION_SHADER.
> """
>
> It sure sounds to me like BindProgramPipeline is the only thing that
> should set EverBound. Is there another part of the spec that I missed
> which suggests otherwise?
>
You mean http://www.opengl.org/registry/specs/ARB/separate_shader_objects.txt ?
Or an older glspec?
Don't know which one is the reference but here an extract (duplicated 4 times)
of glspec43.core.20130214.pdf:
Page 104-105 (void UseProgramStages)
Page 105-106 (void ActiveShaderProgram)
Page 144 (void GetProgramPipelineiv)
Page 339 (void ValidateProgramPipeline)
"""
If pipeline is a name that has been generated (without subsequent deletion) by
GenProgramPipelines, but refers to a program pipeline object that has not been
previously bound, the GL first creates a new state vector in the same manner as
when BindProgramPipeline creates a new program pipeline object.
"""
> > +
> > + if ((shProg != NULL) && !shProg->LinkStatus) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > +"glActiveShaderProgram(program %u not linked)", shProg->Name);
> > + return;
> > + }
> > +
> > + _mesa_reference_shader_program(ctx, &pipe->ActiveProgram, shProg);
> > }
> >
> > /**
> > @@ -348,6 +372,59 @@ _mesa_IsProgramPipeline(GLuint pipeline)
> > void GLAPIENTRY
> > _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname, GLint *params)
> > {
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_pipeline_object *pipe = lookup_pipeline_object(ctx, pipeline);
> > +
> > + /* Are geometry shaders available in this context?
> > +*/
> > + const bool has_gs = _mesa_is_desktop_gl(ctx) &&
> > ctx->Extensions.ARB_geometry_shader4;
> > +
> > + if (!pipe) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > "glGetProgramPipelineiv(pipeline)");
> > + return;
> > + }
> > +
> > + /* Object is created by any Pipeline call but glGenProgramPipelines,
> > +* glIsProgramPipeline and GetProgramPipelineInfoLog
> > +*/
> > + pipe->EverBound = GL_TRUE;
> > +
> > + switch (pname) {
> > + case GL_ACTIVE_PROGRAM:
> > + *params = pipe->ActiveProgram ? pipe->ActiveProgram->Name : 0;
> > + return;
> > + case GL_INFO_LOG_LENGTH:
> > + // TODO
> > + *params = 0;
> > + return;
> > + case GL_VALIDATE_STATUS:
> > + *params = pipe->ValidationStatus;
> > + return;
> > + case GL_VERTEX_SHADER:
> > + *params = pipe->CurrentVertexProgram ?
> > pipe->
Re: [Mesa-dev] [PATCH 10/12] sso: update glGet: GL_PROGRAM_PIPELINE_BINDING
On Sat, 4 May 2013 11:35:22 +0200
gregory hainaut wrote:
> On Fri, 3 May 2013 12:04:48 -0700
> Matt Turner wrote:
>
> > On Fri, May 3, 2013 at 10:44 AM, Gregory Hainaut
> > wrote:
> > > ---
> > > src/mesa/main/get.c |9 +
> > > src/mesa/main/get_hash_params.py |3 +++
> > > 2 files changed, 12 insertions(+)
> > >
> > > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > > index 54159c0..6cbb7db 100644
> > > --- a/src/mesa/main/get.c
> > > +++ b/src/mesa/main/get.c
> > > @@ -369,6 +369,7 @@ EXTRA_EXT(ARB_map_buffer_alignment);
> > > EXTRA_EXT(ARB_texture_cube_map_array);
> > > EXTRA_EXT(ARB_texture_buffer_range);
> > > EXTRA_EXT(ARB_texture_multisample);
> > > +EXTRA_EXT(ARB_separate_shader_objects);
> > >
> > > static const int
> > > extra_ARB_color_buffer_float_or_glcore[] = {
> > > @@ -889,6 +890,14 @@ find_custom_value(struct gl_context *ctx, const
> > > struct value_desc *d, union valu
> > > _mesa_problem(ctx, "driver doesn't implement GetTimestamp");
> > >}
> > >break;
> > > + /* GL_ARB_separate_shader_objects */
> > > + case GL_PROGRAM_PIPELINE_BINDING:
> > > + if (ctx->Pipeline.Current) {
> > > + v->value_int = ctx->Pipeline.Current->Name;
> > > + } else {
> > > + v->value_int = 0;
> > > + }
> > > + break;
> > > }
> > > }
> >
> > This looks believable, but I can't find a description in the extension
> > spec or GL 4.1+ specs that say precisely what this query is supposed
> > to do. Looks like it's just mentioned in the extension spec, and not
> > at all in GL 4.1+ specs.
>
>
> Yes you're right that strange. There is also a couple of line in glGet man
> page.
>
> GL_PROGRAM_PIPELINE_BINDING
>
> params a single value, the name of the currently
> bound program pipeline
> object, or zero if no program pipeline object is
> bound.
> See glBindProgramPipeline.
>
> Both Nvidia and AMD support this query. I did a quick update on my piglit
> test, on the AMD side:
> * UseProgram(2)
> * BindPipeline(5) (the pipeline isn't really bound because UseProgram got an
> higher priority)
> * Get GL_PROGRAM_PIPELINE_BINDING => 5
>
> I will try to check the behavior on Nvidia implementation.
Nvidia implementation "is" this one:
if (ctx->_Shader) {
v->value_int = ctx->_Shader->Name;
} else {
v->value_int = 0;
}
So on my previous example
* UseProgram(2)
* BindPipeline(5)
* Get GL_PROGRAM_PIPELINE_BINDING => 0
There is no spec but the SSO spec was written by Nvidia so I would say that
Nvidia is correct.
> > > diff --git a/src/mesa/main/get_hash_params.py
> > > b/src/mesa/main/get_hash_params.py
> > > index 2b97da6..43a11cf 100644
> > > --- a/src/mesa/main/get_hash_params.py
> > > +++ b/src/mesa/main/get_hash_params.py
> > > @@ -709,6 +709,9 @@ descriptor=[
> > >
> > > # GL_ARB_texture_cube_map_array
> > >[ "TEXTURE_BINDING_CUBE_MAP_ARRAY_ARB", "LOC_CUSTOM, TYPE_INT,
> > > TEXTURE_CUBE_ARRAY_INDEX, extra_ARB_texture_cube_map_array" ],
> > > +
> > > +# GL_ARB_separate_shader_objects
> > > + [ "PROGRAM_PIPELINE_BINDING", "LOC_CUSTOM, TYPE_INT,
> > > GL_PROGRAM_PIPELINE_BINDING, extra_ARB_separate_shader_objects" ],
> > > ]},
> > >
> > > # Enums restricted to OpenGL Core profile
> > > --
> > > 1.7.10.4
> > >
> > > ___
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/12] sso: Add pipeline container/state
On Tue, 28 May 2013 12:30:56 -0700
Ian Romanick wrote:
> On 05/03/2013 10:44 AM, Gregory Hainaut wrote:
> > V1:
> > * Extend gl_shader_state as pipeline object state
> > * Add a new container gl_pipeline_shader_state that contains
> >binding point of the previous object
> > * Update mesa init/free shader state due to the extension of
> >the attibute
> > * Add an init/free pipeline function for the context
> > * Implement GenProgramPipeline/DeleteProgramPipeline/IsProgramPipeline.
> > I based my work on the VAO code.
> >
> > V2:
> > * Rename gl_shader_state to gl_pipeline_object
> > * Rename Pipeline.PipelineObj to Pipeline.Current
> > * Rename ValidationStatus to Validated
> > * Formatting improvement
> > ---
> > src/mesa/main/context.c |3 +
> > src/mesa/main/mtypes.h | 30 +-
> > src/mesa/main/pipelineobj.c | 234
> > ++-
> > src/mesa/main/pipelineobj.h | 25 +
> > src/mesa/main/shaderapi.c | 14 ++-
> > src/mesa/main/shaderapi.h |3 +
> > 6 files changed, 303 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> > index 0539934..d089827 100644
> > --- a/src/mesa/main/context.c
> > +++ b/src/mesa/main/context.c
> > @@ -106,6 +106,7 @@
> > #include "macros.h"
> > #include "matrix.h"
> > #include "multisample.h"
> > +#include "pipelineobj.h"
> > #include "pixel.h"
> > #include "pixelstore.h"
> > #include "points.h"
> > @@ -762,6 +763,7 @@ init_attrib_groups(struct gl_context *ctx)
> > _mesa_init_lighting( ctx );
> > _mesa_init_matrix( ctx );
> > _mesa_init_multisample( ctx );
> > + _mesa_init_pipeline( ctx );
> > _mesa_init_pixel( ctx );
> > _mesa_init_pixelstore( ctx );
> > _mesa_init_point( ctx );
> > @@ -1167,6 +1169,7 @@ _mesa_free_context_data( struct gl_context *ctx )
> > _mesa_free_texture_data( ctx );
> > _mesa_free_matrix_data( ctx );
> > _mesa_free_viewport_data( ctx );
> > + _mesa_free_pipeline_data(ctx);
> > _mesa_free_program_data(ctx);
> > _mesa_free_shader_state(ctx);
> > _mesa_free_queryobj_data(ctx);
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 05d8518..4487068 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -2381,9 +2381,19 @@ struct gl_shader_program
> >
> > /**
> >* Context state for GLSL vertex/fragment shaders.
> > + * Extended to support pipeline object
> >*/
> > -struct gl_shader_state
> > +struct gl_pipeline_object
> > {
> > + /** Name of the pipeline object as received from glGenProgramPipelines.
> > +* It would be 0 for shaders without separate shader objects.
> > +*/
> > + GLuint Name;
> > +
> > + GLint RefCount;
> > +
> > + _glthread_Mutex Mutex;
> > +
> > /**
> > * Programs used for rendering
> > *
>
> I think this comment needs to be updated. The rest of it says
>
> *
> * There is a separate program set for each shader stage. If
> * GL_EXT_separate_shader_objects is not supported, each of these
> must point
> * to \c NULL or to the same program.
> */
>
> However, I think the shader stages can only point to different programs
> for the default pipeline object (gl_pipeline_object::Name == 0). Right?
Hum no it is more complicated.
When gl_pipeline_object::Name != 0, you can put any program on shader stage.
When gl_pipeline_object::Name == 0, there is the 2 possibilities
0/ GL_EXT_separate_shader_objects is not supported. All program must be the
same.
1/ GL_EXT_separate_shader_objects is supported. You can set different program.
> > @@ -2405,8 +2415,23 @@ struct gl_shader_state
> > struct gl_shader_program *ActiveProgram;
> >
> > GLbitfield Flags;/**< Mask of GLSL_x flags */
> > +
> > + GLboolean Validated; /**< Pipeline Validation status */
> > +
> > + GLboolean EverBound; /**< Has the pipeline object been
> > created */
> > };
> >
> > +/**
> > + * Context state for GLSL pipeline shaders.
> > + */
> > +struct gl_pipeline_shader_state
> > +{
> > + /** Currently bound pipeline object. See _mesa_BindProgramPipeline() */
> > + struct gl_pipeline_object *Current;
>
[Mesa-dev] [PATCH] gallium egl: mark KHR_create_context as supported
Don't know why it was not already activated! Code seem to be common between
dri2 and gallium.
Piglit tested on r600 gallium.
---
src/gallium/state_trackers/egl/common/egl_g3d.c |1 +
1 file changed, 1 insertion(+)
diff --git a/src/gallium/state_trackers/egl/common/egl_g3d.c
b/src/gallium/state_trackers/egl/common/egl_g3d.c
index 7cc4e8f..12b767a 100644
--- a/src/gallium/state_trackers/egl/common/egl_g3d.c
+++ b/src/gallium/state_trackers/egl/common/egl_g3d.c
@@ -579,6 +579,7 @@ egl_g3d_initialize(_EGLDriver *drv, _EGLDisplay *dpy)
dpy->Extensions.KHR_reusable_sync = EGL_TRUE;
dpy->Extensions.KHR_fence_sync = EGL_TRUE;
+ dpy->Extensions.KHR_create_context = EGL_TRUE;
dpy->Extensions.KHR_surfaceless_context = EGL_TRUE;
if (dpy->Platform == _EGL_PLATFORM_DRM) {
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] glsl/opt: disable optimization of varying input
GL_ARB_separate_shader_objects allow to match by name variable or block interface. Input varying can't be removed as it is will impact the location assignment. The patch only disable deadcode optimization for inputs that don't have an explicit location. It allows to remove various builtin variables. In theory, I think we must keep them as it could impact the location assignment. However I'm not sure anyone use a mix of match by names/by locations. Interface example: layout (location = 0) vec4 zero; vec4 one; // assignment might be 0 if zero isn't used. It fixes the bug 79783 and likely any application that uses GL_ARB_separate_shader_objects extension. Signed-off-by: Gregory Hainaut --- src/glsl/opt_dead_code.cpp | 16 1 file changed, 16 insertions(+) diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp index e4bf874..9852747 100644 --- a/src/glsl/opt_dead_code.cpp +++ b/src/glsl/opt_dead_code.cpp @@ -131,6 +131,22 @@ do_dead_code(exec_list *instructions, bool uniform_locations_assigned) continue; } + /* Section 7.4.1 (Shader Interface Matching) of the OpenGL 4.5 + * (Core Profile) spec says: + * + *"With separable program objects, interfaces between shader + *stages may involve the outputs from one program object and the + *inputs from a second program object. For such interfaces, it is + *not possible to detect mismatches at link time, because the + *programs are linked separately. When each such program is + *linked, all inputs or outputs interfacing with another program + *stage are treated as active." + */ +if (entry->var->data.mode == ir_var_shader_in && + !entry->var->data.attribute_input && + !entry->var->data.explicit_location) + continue; + entry->var->remove(); progress = true; -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] glsl/link: don't consider unused variable as read variable.
Current checks rely on the optimization phase to remove deadcode
varying. Therefore remaining varying are used.
Deadcode Optimizations won't be possible anymore with the support of
GL_ARB_separate_shader_objects. So it requires to check the used flag of
the varying.
Signed-off-by: Gregory Hainaut
---
src/glsl/link_varyings.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index f7a7b8c..72d6632 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -1579,7 +1579,7 @@ assign_varying_locations(struct gl_context *ctx,
if (var && var->data.mode == ir_var_shader_in &&
var->data.is_unmatched_generic_inout) {
-if (prog->IsES) {
+if (var->data.used && prog->IsES) {
/*
* On Page 91 (Page 97 of the PDF) of the GLSL ES 1.0 spec:
*
@@ -1594,7 +1594,7 @@ assign_varying_locations(struct gl_context *ctx,
_mesa_shader_stage_to_string(consumer->Stage),
var->name,
_mesa_shader_stage_to_string(producer->Stage));
-} else if (prog->Version <= 120) {
+} else if (var->data.used && prog->Version <= 120) {
/* On page 25 (page 31 of the PDF) of the GLSL 1.20 spec:
*
* Only those varying variables used (i.e. read) in
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/3] glsl/ast2hir: add a new flag attribute_input for VS input
GL_ARB_separate_shader_objects requires that all inputs varying remains active.
This flag allow to distinguish input attribute from the input varying.
Flag will be used on next commit.
Signed-off-by: Gregory Hainaut
---
src/glsl/ast_to_hir.cpp | 4
src/glsl/ir.h | 9 +
2 files changed, 13 insertions(+)
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 72c6459..16bf849 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2590,6 +2590,10 @@ apply_type_qualifier_to_variable(const struct
ast_type_qualifier *qual,
_mesa_shader_stage_to_string(state->stage));
}
+ if (state->stage == MESA_SHADER_VERTEX &&
+ (qual->flags.q.attribute || qual->flags.q.in))
+ var->data.attribute_input = 1;
+
/* Disallow layout qualifiers which may only appear on layout declarations.
*/
if (qual->flags.q.prim_type) {
_mesa_glsl_error(loc, state,
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index f9ddf74..a1f5fa0 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -627,6 +627,15 @@ public:
unsigned precise:1;
/**
+ * Is the variable an input from the Vertex Shader?
+ *
+ * GL_ARB_separate_shader_objects doesn't allow the optimization of
+ * unused varying variable. However unused-attribute must still be
+ * optimized.
+ */
+ unsigned attribute_input:1;
+
+ /**
* Has this variable been used for reading or writing?
*
* Several GLSL semantic checks require knowledge of whether or not a
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3][RFC] Improve GLSL support of GL_ARB_separate_shader_objects
Current GLSL badly optimizes the code making it incompatible with the
GL_ARB_separate_shader_objects extension.
Typical example of the current behavior:
VS:
out SHADER
{
vec4 var_zero; // location will always be 0
vec2 var_one; // location will always be 1
} VSout;
FS:
in SHADER
{
vec4 var_zero;
vec2 var_one; // ERROR: location would be wrongly set to 0 if var_zero is
inactive
} PSin;
In short, SSO allow to match by name but you can't make any hypothesis on the
previous/next stage. Therefore you must consider all inputs as actives.
Commit decription:
patch 1/ Tighten up some linker error checks to avoid dependency with the
deadcode optimization phase. Phase that will be partially removed in
patch 3.
patch 2/ Add a new variable flags in the AST to distinguish attribute from
varying. It is still legal to removed unused attribute.
patch 3/ Disable the optimization of varying inputs that don't have an explicit
location. The idea was to keep the optimization for all builtins
varyings. Besides it won't trigger the bug if explicit location are
used for all variables. I'm not sure what it is the best solutions.
Piglit test done on older Mesa/Nouveau (8276ba260) due to libdrm newer
requirement.
[30454/30454] crash: 29, fail: 221, pass: 20749, skip: 9451, warn: 4 -
The fix was validated on the PCSX2 application. I will try to create a
piglit tests of the above situation.
Gregory Hainaut (3):
glsl/link: don't consider unused variable as read variable.
glsl/ast2hir: add a new flag attribute_input for VS input
glsl/opt: disable optimization of varying input
src/glsl/ast_to_hir.cpp| 4
src/glsl/ir.h | 9 +
src/glsl/link_varyings.cpp | 4 ++--
src/glsl/opt_dead_code.cpp | 16
4 files changed, 31 insertions(+), 2 deletions(-)
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/3][RFC] Improve GLSL support of GL_ARB_separate_shader_objects
On Fri, 25 Sep 2015 16:40:31 -0700
Ian Romanick wrote:
> On 09/20/2015 01:15 PM, Gregory Hainaut wrote:
> > Current GLSL badly optimizes the code making it incompatible with the
> > GL_ARB_separate_shader_objects extension.
> >
> > Typical example of the current behavior:
> >
> > VS:
> > out SHADER
> > {
> > vec4 var_zero; // location will always be 0
> > vec2 var_one; // location will always be 1
> > } VSout;
> >
> > FS:
> > in SHADER
> > {
> > vec4 var_zero;
> > vec2 var_one; // ERROR: location would be wrongly set to 0 if var_zero
> > is inactive
> > } PSin;
> >
> > In short, SSO allow to match by name but you can't make any hypothesis on
> > the
> > previous/next stage. Therefore you must consider all inputs as actives.
>
> Welcome back. :)
Oh, thanks you :)
> I think I understand what is happening, but I think the approach is not
> quite right. My understanding, with the aid of the spec quote in patch
> 3, is that any interstage I/O (i.e., not vertex inputs and not fragment
> outputs) that are explicitly declared in a stage, even if unused in that
> stage, cannot be eliminated. This is roughly like layout(shared) on UBOs.
Yes
> This set of changes only prevents unused interstage inputs from being
> eliminated. It does nothing for vertex, geometry, or tessellation
> outputs. It also prevents the elimination of user-declared inputs that
> could previously be eliminated. It seems that if the VS and FS
> mentioned above were linked together, var_zero would (incorrectly) not
> be eliminated. Right?
Yes, you're right. I extended my current piglit test to check
both input and output wrong that are wrongly eliminated. I also made a small
test to check interstage optimization.
> I think patches 1 and 2 should just be dropped. A new patch 1 should
> add a method that determines whether or not a ir_var_shader_in or
> ir_var_shader_out is interstage. I thought such a function existed, but
> I could not find it. The replacement for patch 3 would change the logic
> to "if it's interstage I/O, and there isn't an explicit location, and
> this stage isn't linked with a following stage, do not eliminate the
> variable."
>
> The "this stage isn't linked with a following stage" part is important
> because we still want to eliminate vertex shader outputs in separable
> shaders if the vertex shader is linked directly with (only) a geometry
> shader, for example.
>
So far, I manage to annotate the ir with an interstage flag. However some issues
are remaining with explicit location.
Here the bad FS program:
layout(location = 0) in vec4 blue;
in vec4 green;
in vec4 red;
void main()
{
out_color = vec4(green.xyz, 1);
}
// IR for the FS
(declare (location=26 shader_in ) vec4 green)
(declare (location=27 shader_in ) vec4 red)
(declare (location=4 shader_out ) vec4 out_color)
// IR for the VS (same interface but declared as out)
(declare (location=0 shader_out ) vec4 gl_Position)
(declare (temporary ) vec4 gl_Position)
(declare (location=26 shader_out ) vec4 blue)
(declare (temporary ) vec4 blue)
(declare (location=26 shader_out flat) vec4 green)
(declare (temporary ) vec4 green)
(declare (location=27 shader_out flat) vec4 red)
(declare (temporary ) vec4 red)
(declare (location=17 shader_in ) vec4 piglit_vertex)
As you can see there are 2 problems.
1/ location in the VS overlap, 26 appears 2 times.
I'm not sure that Mesa support it actually (or if it isn't allowed).
2/ FS doesn't reserve slot 26 (VARYING_SLOT_VAR0) for blue and start directly
to the slot 26
Technically I could just disable the optimization of those variables too. But
sadly
I hit issue number 1 for FS too.
Any idea of where it could be wrong?
Thanks.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/3][RFC] Improve GLSL support of GL_ARB_separate_shader_objects
On Wed, 30 Sep 2015 21:18:43 +0200
gregory hainaut wrote:
> On Fri, 25 Sep 2015 16:40:31 -0700
> Ian Romanick wrote:
>
> > On 09/20/2015 01:15 PM, Gregory Hainaut wrote:
> > > Current GLSL badly optimizes the code making it incompatible with the
> > > GL_ARB_separate_shader_objects extension.
> > >
> > > Typical example of the current behavior:
> > >
> > > VS:
> > > out SHADER
> > > {
> > > vec4 var_zero; // location will always be 0
> > > vec2 var_one; // location will always be 1
> > > } VSout;
> > >
> > > FS:
> > > in SHADER
> > > {
> > > vec4 var_zero;
> > > vec2 var_one; // ERROR: location would be wrongly set to 0 if
> > > var_zero is inactive
> > > } PSin;
> > >
> > > In short, SSO allow to match by name but you can't make any hypothesis on
> > > the
> > > previous/next stage. Therefore you must consider all inputs as actives.
> >
> > Welcome back. :)
>
> Oh, thanks you :)
>
> > I think I understand what is happening, but I think the approach is not
> > quite right. My understanding, with the aid of the spec quote in patch
> > 3, is that any interstage I/O (i.e., not vertex inputs and not fragment
> > outputs) that are explicitly declared in a stage, even if unused in that
> > stage, cannot be eliminated. This is roughly like layout(shared) on UBOs.
>
> Yes
>
> > This set of changes only prevents unused interstage inputs from being
> > eliminated. It does nothing for vertex, geometry, or tessellation
> > outputs. It also prevents the elimination of user-declared inputs that
> > could previously be eliminated. It seems that if the VS and FS
> > mentioned above were linked together, var_zero would (incorrectly) not
> > be eliminated. Right?
>
> Yes, you're right. I extended my current piglit test to check
> both input and output wrong that are wrongly eliminated. I also made a small
> test to check interstage optimization.
>
> > I think patches 1 and 2 should just be dropped. A new patch 1 should
> > add a method that determines whether or not a ir_var_shader_in or
> > ir_var_shader_out is interstage. I thought such a function existed, but
> > I could not find it. The replacement for patch 3 would change the logic
> > to "if it's interstage I/O, and there isn't an explicit location, and
> > this stage isn't linked with a following stage, do not eliminate the
> > variable."
> >
> > The "this stage isn't linked with a following stage" part is important
> > because we still want to eliminate vertex shader outputs in separable
> > shaders if the vertex shader is linked directly with (only) a geometry
> > shader, for example.
> >
> So far, I manage to annotate the ir with an interstage flag. However some
> issues
> are remaining with explicit location.
>
> Here the bad FS program:
>
> layout(location = 0) in vec4 blue;
> in vec4 green;
> in vec4 red;
>
> void main()
> {
> out_color = vec4(green.xyz, 1);
> }
>
> // IR for the FS
> (declare (location=26 shader_in ) vec4 green)
> (declare (location=27 shader_in ) vec4 red)
> (declare (location=4 shader_out ) vec4 out_color)
>
> // IR for the VS (same interface but declared as out)
> (declare (location=0 shader_out ) vec4 gl_Position)
> (declare (temporary ) vec4 gl_Position)
> (declare (location=26 shader_out ) vec4 blue)
> (declare (temporary ) vec4 blue)
> (declare (location=26 shader_out flat) vec4 green)
> (declare (temporary ) vec4 green)
> (declare (location=27 shader_out flat) vec4 red)
> (declare (temporary ) vec4 red)
> (declare (location=17 shader_in ) vec4 piglit_vertex)
>
> As you can see there are 2 problems.
> 1/ location in the VS overlap, 26 appears 2 times.
> I'm not sure that Mesa support it actually (or if it isn't allowed).
>
> 2/ FS doesn't reserve slot 26 (VARYING_SLOT_VAR0) for blue and start directly
> to the slot 26
> Technically I could just disable the optimization of those variables too. But
> sadly
> I hit issue number 1 for FS too.
>
> Any idea of where it could be wrong?
>
> Thanks.
>
Hum, just further reading of the code, I found a comment in
link_invalidate_variable_locations that code must be updated for
GL_ARB_separate_shader_objects. In the above test, the explicit variable
has is_unmatched_generic_layout = 0 so varying_matches::record doesn't
record it. Not sure if the comment is still valid.
Otherwise, it seems that varying_matches::assign_locations doesn't take into
account
already reserved locations.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/3] glsl IR: add not_interstage attribute to ir_variable
The negative form was used to keep interstage as the default value
Note: I put the ir_set_not_interstage_io function in opt_dead_code
because it will be used to disable deadcode optimization. Feel free
to point me a better place if it exists.
Signed-off-by: Gregory Hainaut
---
src/glsl/ir.h | 6 ++
src/glsl/ir_optimization.h | 2 ++
src/glsl/opt_dead_code.cpp | 19 +++
3 files changed, 27 insertions(+)
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index f9ddf74..b4c4ace 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -649,6 +649,12 @@ public:
unsigned assigned:1;
/**
+ * When separate shader programs are enabled, only interstage
+ * variables can be safely removed of the shader interface.
+ */
+ unsigned not_interstage:1;
+
+ /**
* Enum indicating how the variable was declared. See
* ir_var_declaration_type.
*
diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
index 265b223..bdd6ccc 100644
--- a/src/glsl/ir_optimization.h
+++ b/src/glsl/ir_optimization.h
@@ -72,6 +72,8 @@ enum lower_packing_builtins_op {
LOWER_PACK_USE_BFE = 0x2000,
};
+void ir_set_not_interstage_io(exec_list *instructions, ir_variable_mode mode);
+
bool do_common_optimization(exec_list *ir, bool linked,
bool uniform_locations_assigned,
const struct gl_shader_compiler_options *options,
diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
index e4bf874..d70c855 100644
--- a/src/glsl/opt_dead_code.cpp
+++ b/src/glsl/opt_dead_code.cpp
@@ -35,6 +35,25 @@
static bool debug = false;
+void
+ir_set_not_interstage_io(exec_list *instructions, ir_variable_mode mode)
+{
+ ir_variable_refcount_visitor v;
+
+ assert(mode == ir_var_shader_in || mode == ir_var_shader_out);
+
+ v.run(instructions);
+
+ struct hash_entry *e;
+ hash_table_foreach(v.ht, e) {
+ ir_variable_refcount_entry *entry = (ir_variable_refcount_entry
*)e->data;
+
+ if (entry->var->data.mode == mode)
+ entry->var->data.not_interstage = 1;
+ }
+}
+
+
/**
* Do a dead code pass over instructions and everything that instructions
* references.
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 2/3] glsl link: annotate not_interstage attribute of the ir_var after link phase
Signed-off-by: Gregory Hainaut
---
src/glsl/linker.cpp | 47 +++
1 file changed, 47 insertions(+)
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index fd69dbc..a4a8ab4 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -3378,6 +3378,51 @@ link_assign_subroutine_types(struct gl_shader_program
*prog)
}
}
+static void
+set_interstage_io(struct gl_shader_program *prog)
+{
+ unsigned first, last;
+
+ /* In monolithic build, IO can be freely optimized */
+ if (!prog->SeparateShader)
+ return;
+
+ first = MESA_SHADER_STAGES;
+ last = 0;
+
+ /* Determine first and last stage. Excluding the compute stage */
+ for (unsigned i = 0; i < MESA_SHADER_COMPUTE; i++) {
+ if (!prog->_LinkedShaders[i])
+ continue;
+ if (first == MESA_SHADER_STAGES)
+ first = i;
+ last = i;
+ }
+
+ if (first == MESA_SHADER_STAGES)
+ return;
+
+ for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
+ gl_shader *sh = prog->_LinkedShaders[stage];
+ if (!sh)
+ continue;
+
+ if (first == last) {
+ /* Single shader program */
+ if (first != MESA_SHADER_VERTEX)
+ir_set_not_interstage_io(sh->ir, ir_var_shader_in);
+ if (first != MESA_SHADER_FRAGMENT)
+ir_set_not_interstage_io(sh->ir, ir_var_shader_out);
+ } else {
+ /* Multiple shaders program */
+ if (stage == first && stage != MESA_SHADER_VERTEX)
+ir_set_not_interstage_io(sh->ir, ir_var_shader_in);
+ else if (stage == last && stage != MESA_SHADER_FRAGMENT)
+ir_set_not_interstage_io(sh->ir, ir_var_shader_out);
+ }
+ }
+}
+
void
link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
{
@@ -3637,6 +3682,8 @@ link_shaders(struct gl_context *ctx, struct
gl_shader_program *prog)
}
}
+ set_interstage_io(prog);
+
if (!interstage_cross_validate_uniform_blocks(prog))
goto done;
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 3/3] glsl IR: only allow optimization of interstage variable
GL_ARB_separate_shader_objects allow to match by name variable or block
interface. Input varying can't be removed as it is will impact the location
assignment.
It fixes the bug 79783 and likely any application that uses
GL_ARB_separate_shader_objects extension.
Signed-off-by: Gregory Hainaut
---
src/glsl/opt_dead_code.cpp | 18 ++
1 file changed, 18 insertions(+)
diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
index d70c855..9ddee4d 100644
--- a/src/glsl/opt_dead_code.cpp
+++ b/src/glsl/opt_dead_code.cpp
@@ -94,6 +94,24 @@ do_dead_code(exec_list *instructions, bool
uniform_locations_assigned)
|| !entry->declaration)
continue;
+ /* Section 7.4.1 (Shader Interface Matching) of the OpenGL 4.5
+ * (Core Profile) spec says:
+ *
+ *"With separable program objects, interfaces between shader
+ *stages may involve the outputs from one program object and the
+ *inputs from a second program object. For such interfaces, it is
+ *not possible to detect mismatches at link time, because the
+ *programs are linked separately. When each such program is
+ *linked, all inputs or outputs interfacing with another program
+ *stage are treated as active."
+ */
+ if (entry->var->data.not_interstage &&
+(!entry->var->data.explicit_location ||
+ entry->var->data.location >= VARYING_SLOT_VAR0) &&
+(entry->var->data.mode == ir_var_shader_in ||
+ entry->var->data.mode == ir_var_shader_out))
+ continue;
+
if (entry->assign) {
/* Remove a single dead assignment to the variable we found.
* Don't do so if it's a shader or function output or a shader
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 0/3] V2: Improve GLSL support of GL_ARB_separate_shader_objects
> In short, SSO allow to match by name but you can't make any hypothesis on the > previous/next stage. Therefore you must consider all inputs and output as > actives. New version based on Ian's feedbacks. * Real interstage variables of the program are still optimized * Both output and input of the program remains active * An old bug still remains if name and explicit location rendezvous are mixed Commits decription: 1/ add a not_interstage parameter that will be 1 when variable is either an input of output of the program. I didn't know where to put the ir_set_not_interstage_io function. Maybe it can be moved in linker.cpp. 2/ Set the interstage parameter based on the shader position in the pipeline program. Potentially can be squased with the previous one. 3/ Don't do deadcode removal of not_interstage variable with the exception of the built-in varyings because they don't impact location of others variables Gregory Hainaut (4): glsl IR: add not_interstage attribute to ir_variable glsl link: annotate not_interstage attribute of the ir_var after link phase glsl IR: only allow optimization of interstage variable allow compilation on my system configure.ac | 6 -- src/glsl/ir.h | 6 ++ src/glsl/ir_optimization.h | 2 ++ src/glsl/linker.cpp| 47 ++ src/glsl/opt_dead_code.cpp | 37 5 files changed, 96 insertions(+), 2 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/3] V2: Improve GLSL support of GL_ARB_separate_shader_objects
On Sat, 03 Oct 2015 09:35:49 + Mike Lothian wrote: > Would it be better to have is_interstage=0 rather than a double negative? > Yes. I think it just need to set 1 in the constructor (forget to update it by the way...) as default value. Otherwise it can be renamed to is_unlinked_io (or something similar). > > > > In short, SSO allow to match by name but you can't make any hypothesis > > on the > > > previous/next stage. Therefore you must consider all inputs and output as > > > actives. > > > > New version based on Ian's feedbacks. > > * Real interstage variables of the program are still optimized > > * Both output and input of the program remains active > > * An old bug still remains if name and explicit location rendezvous are > > mixed > > > > Commits decription: > > 1/ add a not_interstage parameter that will be 1 when variable is either an > >input of output of the program. I didn't know where to put the > > ir_set_not_interstage_io > >function. Maybe it can be moved in linker.cpp. > > 2/ Set the interstage parameter based on the shader position in the > > pipeline > >program. Potentially can be squased with the previous one. > > 3/ Don't do deadcode removal of not_interstage variable with the exception > >of the built-in varyings because they don't impact location of others > > variables > > > > Gregory Hainaut (4): > > glsl IR: add not_interstage attribute to ir_variable > > glsl link: annotate not_interstage attribute of the ir_var after link > > phase > > glsl IR: only allow optimization of interstage variable > > allow compilation on my system > > > > configure.ac | 6 -- > > src/glsl/ir.h | 6 ++ > > src/glsl/ir_optimization.h | 2 ++ > > src/glsl/linker.cpp| 47 > > ++ > > src/glsl/opt_dead_code.cpp | 37 > > 5 files changed, 96 insertions(+), 2 deletions(-) > > > > -- > > 2.1.4 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/3] V2: Improve GLSL support of GL_ARB_separate_shader_objects
On Sat, 3 Oct 2015 11:59:49 +0200
gregory hainaut wrote:
> On Sat, 03 Oct 2015 09:35:49 +
> Mike Lothian wrote:
>
> > Would it be better to have is_interstage=0 rather than a double negative?
> >
> Yes. I think it just need to set 1 in the constructor (forget to
> update it by the way...) as default value. Otherwise it can be
> renamed to is_unlinked_io (or something similar).
>
> >
> > > > In short, SSO allow to match by name but you can't make any hypothesis
> > > on the
> > > > previous/next stage. Therefore you must consider all inputs and output
> > > > as
> > > > actives.
> > >
> > > New version based on Ian's feedbacks.
> > > * Real interstage variables of the program are still optimized
> > > * Both output and input of the program remains active
> > > * An old bug still remains if name and explicit location rendezvous are
> > > mixed
> > >
> > > Commits decription:
> > > 1/ add a not_interstage parameter that will be 1 when variable is either
> > > an
> > >input of output of the program. I didn't know where to put the
> > > ir_set_not_interstage_io
> > >function. Maybe it can be moved in linker.cpp.
> > > 2/ Set the interstage parameter based on the shader position in the
> > > pipeline
> > > program. Potentially can be squased with the previous one.
> > > 3/ Don't do deadcode removal of not_interstage variable with the exception
> > >of the built-in varyings because they don't impact location of others
> > > variables
> > >
> > > Gregory Hainaut (4):
> > > glsl IR: add not_interstage attribute to ir_variable
> > > glsl link: annotate not_interstage attribute of the ir_var after link
> > > phase
> > > glsl IR: only allow optimization of interstage variable
> > > allow compilation on my system
> > >
> > > configure.ac | 6 --
> > > src/glsl/ir.h | 6 ++
> > > src/glsl/ir_optimization.h | 2 ++
> > > src/glsl/linker.cpp| 47
> > > ++
> > > src/glsl/opt_dead_code.cpp | 37
> > > 5 files changed, 96 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.1.4
> > >
> > > ___
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
I found another issue with my application (lucky me).
out SHADER
{
vec4 t;
vec4 c;
flat vec4 fc;
} VSout;
in SHADER
{
vec4 t;
vec4 c;
flat vec4 fc;
} PSin;
will be transformed into the following interface in SSO (in
varying_matches::record) to
out SHADER
{
flat vec4 t;
flat vec4 c;
flat vec4 fc;
} VSout;
in SHADER
{
vec4 t;
vec4 c;
flat vec4 fc;
} PSin;
It doesn't change the interpolation/rendering as it will use the one
from the FS. However it will affect the
match_comparator function. So the sorting in
varying_matches::assign_locations will
behave differently for some parameters. I don't know how to handle
this. Either we could adapt the sort function
to mask the flat value. Or we mustn't add the flat qualifier. Any opinions or
other solutions?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa: faster validation of sampler unit mapping for SSO
Code was inspired from _mesa_update_shader_textures_used
However unlike _mesa_update_shader_textures_used that only check for a single
stage, it will check all stages.
It avoids to loop on all uniforms, only active samplers are checked.
Signed-off-by: Gregory Hainaut
---
src/mesa/main/uniform_query.cpp | 69 ++---
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index 127f097..4fa4e6e 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -1068,58 +1068,51 @@ _mesa_sampler_uniforms_pipeline_are_valid(struct
gl_pipeline_object *pipeline)
* - The number of active samplers in the program exceeds the
* maximum number of texture image units allowed."
*/
+
+ GLbitfield mask;
+ GLbitfield TexturesUsed[MAX_COMBINED_TEXTURE_IMAGE_UNITS];
+ struct gl_shader *shader;
unsigned active_samplers = 0;
const struct gl_shader_program **shProg =
(const struct gl_shader_program **) pipeline->CurrentProgram;
- const glsl_type *unit_types[MAX_COMBINED_TEXTURE_IMAGE_UNITS];
- memset(unit_types, 0, sizeof(unit_types));
+
+ memset(TexturesUsed, 0, sizeof(TexturesUsed));
for (unsigned idx = 0; idx < ARRAY_SIZE(pipeline->CurrentProgram); idx++) {
if (!shProg[idx])
continue;
- for (unsigned i = 0; i < shProg[idx]->NumUniformStorage; i++) {
- const struct gl_uniform_storage *const storage =
-&shProg[idx]->UniformStorage[i];
+ shader = shProg[idx]->_LinkedShaders[idx];
+ if (!shader || !shader->Program)
+ continue;
- if (!storage->type->is_sampler())
+ mask = shader->Program->SamplersUsed;
+ while (mask) {
+ const int s = u_bit_scan(&mask);
+ GLuint unit = shader->SamplerUnits[s];
+ GLuint tgt = shader->SamplerTargets[s];
+
+ /* FIXME: Samplers are initialized to 0 and Mesa doesn't do a
+ * great job of eliminating unused uniforms currently so for now
+ * don't throw an error if two sampler types both point to 0.
+ */
+ if (unit == 0)
continue;
- active_samplers++;
-
- const unsigned count = MAX2(1, storage->array_elements);
- for (unsigned j = 0; j < count; j++) {
-const unsigned unit = storage->storage[j].i;
-
-/* FIXME: Samplers are initialized to 0 and Mesa doesn't do a
- * great job of eliminating unused uniforms currently so for now
- * don't throw an error if two sampler types both point to 0.
- */
-if (unit == 0)
- continue;
-
-/* The types of the samplers associated with a particular texture
- * unit must be an exact match. Page 74 (page 89 of the PDF) of
- * the OpenGL 3.3 core spec says:
- *
- * "It is not allowed to have variables of different sampler
- * types pointing to the same texture image unit within a
- * program object."
- */
-if (unit_types[unit] == NULL) {
- unit_types[unit] = storage->type;
-} else if (unit_types[unit] != storage->type) {
- pipeline->InfoLog =
- ralloc_asprintf(pipeline,
- "Texture unit %d is accessed both as %s "
- "and %s",
- unit, unit_types[unit]->name,
- storage->type->name);
- return false;
-}
+ if (TexturesUsed[unit] & ~(1 << tgt)) {
+pipeline->InfoLog =
+ ralloc_asprintf(pipeline,
+ "Program %d: "
+ "Texture unit %d is accessed with 2 different types",
+ shProg[idx]->Name, unit);
+return false;
}
+
+ TexturesUsed[unit] |= (1 << tgt);
}
+
+ active_samplers += shader->num_samplers;
}
if (active_samplers > MAX_COMBINED_TEXTURE_IMAGE_UNITS) {
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: faster validation of sampler unit mapping for SSO
Hello Timothy,
On my use case: high FS frequency switches. Few sampler.
Perf event (relative to nouveau_dri.so) goes from 5.01% to 1.68% for
the _mesa_sampler_uniforms_pipeline_are_valid function.
Unfortunately it didn't translate into measurable speed change. I have
too much FPS variation (cache? temperature, GPU turbo?).
Honestly I was reluctant to send the patch, but the code isn't more
complex. I let's you judge.
Gregory
On 6/25/16, Timothy Arceri wrote:
> On Fri, 2016-06-24 at 10:07 +0200, Gregory Hainaut wrote:
>> Code was inspired from _mesa_update_shader_textures_used
>>
>> However unlike _mesa_update_shader_textures_used that only check for
>> a single
>> stage, it will check all stages.
>>
>> It avoids to loop on all uniforms, only active samplers are checked.
>
> Hi Gregory,
>
> Is there any measurable change as a result of this? Normally
> performance changes should come with some results in the commit
> message.
>
> Tim
>
>>
>> Signed-off-by: Gregory Hainaut
>> ---
>> src/mesa/main/uniform_query.cpp | 69 ++-
>> --
>> 1 file changed, 31 insertions(+), 38 deletions(-)
>>
>> diff --git a/src/mesa/main/uniform_query.cpp
>> b/src/mesa/main/uniform_query.cpp
>> index 127f097..4fa4e6e 100644
>> --- a/src/mesa/main/uniform_query.cpp
>> +++ b/src/mesa/main/uniform_query.cpp
>> @@ -1068,58 +1068,51 @@
>> _mesa_sampler_uniforms_pipeline_are_valid(struct gl_pipeline_object
>> *pipeline)
>> * - The number of active samplers in the program exceeds
>> the
>> * maximum number of texture image units allowed."
>> */
>> +
>> + GLbitfield mask;
>> + GLbitfield TexturesUsed[MAX_COMBINED_TEXTURE_IMAGE_UNITS];
>> + struct gl_shader *shader;
>> unsigned active_samplers = 0;
>> const struct gl_shader_program **shProg =
>> (const struct gl_shader_program **) pipeline->CurrentProgram;
>>
>> - const glsl_type *unit_types[MAX_COMBINED_TEXTURE_IMAGE_UNITS];
>> - memset(unit_types, 0, sizeof(unit_types));
>> +
>> + memset(TexturesUsed, 0, sizeof(TexturesUsed));
>>
>> for (unsigned idx = 0; idx < ARRAY_SIZE(pipeline-
>> >CurrentProgram); idx++) {
>> if (!shProg[idx])
>> continue;
>>
>> - for (unsigned i = 0; i < shProg[idx]->NumUniformStorage; i++)
>> {
>> - const struct gl_uniform_storage *const storage =
>> -&shProg[idx]->UniformStorage[i];
>> + shader = shProg[idx]->_LinkedShaders[idx];
>> + if (!shader || !shader->Program)
>> + continue;
>>
>> - if (!storage->type->is_sampler())
>> + mask = shader->Program->SamplersUsed;
>> + while (mask) {
>> + const int s = u_bit_scan(&mask);
>> + GLuint unit = shader->SamplerUnits[s];
>> + GLuint tgt = shader->SamplerTargets[s];
>> +
>> + /* FIXME: Samplers are initialized to 0 and Mesa doesn't do
>> a
>> + * great job of eliminating unused uniforms currently so
>> for now
>> + * don't throw an error if two sampler types both point to
>> 0.
>> + */
>> + if (unit == 0)
>> continue;
>>
>> - active_samplers++;
>> -
>> - const unsigned count = MAX2(1, storage->array_elements);
>> - for (unsigned j = 0; j < count; j++) {
>> -const unsigned unit = storage->storage[j].i;
>> -
>> -/* FIXME: Samplers are initialized to 0 and Mesa doesn't
>> do a
>> - * great job of eliminating unused uniforms currently so
>> for now
>> - * don't throw an error if two sampler types both point
>> to 0.
>> - */
>> -if (unit == 0)
>> - continue;
>> -
>> -/* The types of the samplers associated with a
>> particular texture
>> - * unit must be an exact match. Page 74 (page 89 of the
>> PDF) of
>> - * the OpenGL 3.3 core spec says:
>> - *
>> - * "It is not allowed to have variables of different
>> sampler
>> - * types pointing to the same texture image unit
>> within a
>> - * program object."
>> - */
>> -if (unit_types[unit] == NULL) {
>> - unit_types[unit] = storage->type;
>> -
Re: [Mesa-dev] [PATCH 3/7] glsl: optimise inputs/outputs with explicit locations
Hello Timoty,
The patch introduces a regression on deqp test. (I'm nearly sure there are
hidden piglit tests that trigger the same error)
DEQP test: deqp-gles31
--deqp-case=dEQP-GLES31.functional.separate_shader.interface.same_location
Be awware that you need to patch Mesa (temporary hack) to start the test. See
below
The relevant log:
// vertex interface
layout(location=0) out highp float vtxVar0;
// fragment interface
layout(location=0) out highp float frgVar0;
// mesa warning
warning: fragment shader varying frgVar0 not written by vertex shader
Cheers,
Gregory
diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index 90dff13..6e333c5 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -646,7 +646,8 @@ _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname,
GLint *params)
return;
case GL_VALIDATE_STATUS:
/* If pipeline is not bound, return initial value 0. */
- *params = (ctx->_Shader->Name != pipe->Name) ? 0 : pipe->Validated;
+ //*params = (ctx->_Shader->Name != pipe->Name) ? 0 : pipe->Validated;
+ *params = pipe->Validated;
return;
case GL_VERTEX_SHADER:
*params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
On Sat, 21 Nov 2015 19:02:02 +1100
Timothy Arceri wrote:
> From: Timothy Arceri
>
> This change allows used defined inputs/outputs with explicit locations
> to be removed if they are detected to not be used between shaders
> at link time.
>
> To enable this we change the is_unmatched_generic_inout field to be
> flagged when we have a user defined varying. Previously
> explicit_location was assumed to be set only in builtins however SSO
> allows the user to set an explicit location.
>
> We then add a function to match explicit locations between shaders.
>
> Cc: Gregory Hainaut
> ---
> src/glsl/link_varyings.cpp | 6 ++--
> src/glsl/linker.cpp| 82
> +++---
> 2 files changed, 74 insertions(+), 14 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index c0b4b3e..ac2755f 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -896,8 +896,10 @@ varying_matches::record(ir_variable *producer_var,
> ir_variable *consumer_var)
> {
> assert(producer_var != NULL || consumer_var != NULL);
>
> - if ((producer_var && !producer_var->data.is_unmatched_generic_inout)
> - || (consumer_var && !consumer_var->data.is_unmatched_generic_inout)) {
> + if ((producer_var && (!producer_var->data.is_unmatched_generic_inout ||
> + producer_var->data.explicit_location)) ||
> + (consumer_var && (!consumer_var->data.is_unmatched_generic_inout ||
> + consumer_var->data.explicit_location))) {
>/* Either a location already exists for this variable (since it is part
> * of fixed functionality), or it has already been recorded as part of
> a
> * previous match.
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 5ff433c..1db7b7a 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -631,20 +631,12 @@ link_invalidate_variable_locations(exec_list *ir)
>
>/* ir_variable::is_unmatched_generic_inout is used by the linker while
> * connecting outputs from one stage to inputs of the next stage.
> - *
> - * There are two implicit assumptions here. First, we assume that any
> - * built-in variable (i.e., non-generic in or out) will have
> - * explicit_location set. Second, we assume that any generic in or out
> - * will not have explicit_location set.
> - *
> - * This second assumption will only be valid until
> - * GL_ARB_separate_shader_objects is supported. When that extension is
> - * implemented, this function will need some modifications.
> */
> - if (!var->data.explicit_location) {
> - var->data.is_unmatched_generic_inout = 1;
> - } else {
> + if (var->data.explicit_location &&
> + var->data.location < VARYING_SLOT_VAR0) {
> var->data.is_unmatched_generic_inout = 0;
> + } else {
> + var->data.is_unmatched_generic_inout = 1;
>}
> }
> }
> @@ -2421,6 +2413,7 @@ assign_attribute_or_color_locations(gl_shader_program
> *prog,
>continue;
>
>if (var->data.explicit_location) {
> + var->data.is_unmatched_generic_inout = 0;
>if ((var->data.location >= (int)(max_index + generic_base))
>|| (var->data.location < 0)) {
> linker_error(prog,
> @@ -2690,6 +2683,61 @@ ass
Re: [Mesa-dev] [PATCH 3/3] mesa: return initial value for VALIDATE_STATUS if pipe not bound
On Sun, 22 Nov 2015 14:25:31 +1100
Timothy Arceri wrote:
> On Tue, 2015-09-01 at 13:53 +0300, Tapani Pälli wrote:
> > From OpenGL 4.5 Core spec (7.13):
> >
> > "If pipeline is a name that has been generated (without
> > subsequent
> > deletion) by GenProgramPipelines, but refers to a program
> > pipeline
> > object that has not been previously bound, the GL first creates
> > a new state vector in the same manner as when BindProgramPipeline
> > creates a new program pipeline object."
> >
> > I interpret this as "If GetProgramPipelineiv gets called without a
> > bound (but valid) pipeline object, the state should reflect initial
> > state of a new pipeline object." This is also expected behaviour by
> > ES31-CTS.sepshaderobjs.PipelineApi conformance test.
> >
> > Signed-off-by: Tapani Pälli
> > ---
> > src/mesa/main/pipelineobj.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/main/pipelineobj.c
> > b/src/mesa/main/pipelineobj.c
> > index 07acbf1..c2e1d29 100644
> > --- a/src/mesa/main/pipelineobj.c
> > +++ b/src/mesa/main/pipelineobj.c
> > @@ -614,7 +614,8 @@ _mesa_GetProgramPipelineiv(GLuint pipeline,
> > GLenum pname, GLint *params)
> >*params = pipe->InfoLog ? strlen(pipe->InfoLog) + 1 : 0;
> >return;
> > case GL_VALIDATE_STATUS:
> > - *params = pipe->Validated;
> > + /* If pipeline is not bound, return initial value 0. */
> > + *params = (ctx->_Shader->Name != pipe->Name)
> > ? 0 : pipe->Validated;
>
> Hi Tapani,
>
> As Gregory has pointed out this change causes a large number of the
> SSO deqp tests to fail.
>
> I'm not sure what the solution is yet but one thing I have noticed is
> that with this change we will always return a value of 0 as nothing
> ever sets the value of ctx->_Shader->Name.
>
> I did try setting it when the bind is done but the deqp tests still
> fail as the bind is done after the validate in those tests. Are you
> sure that this must return 0 if the pipe is not bound?
>
> Tim
>
> >return;
> > case GL_VERTEX_SHADER:
> >*params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
Hello,
Here my understanding of the specification (with my point of view of GL
users).
On various objects, gen* cmds reserve a key in a hash table but objects
aren't created. Generally the object are created when they're bound (often call
bind to
create). However SSO API is (closer of the/a) direct state access API. It is
really
awkward to bind the object to modify it. Besides there is now full DSA.
So for me, the following pipeline command must created the object if it
wasn't created (read if it wasn't bound)
1/ BindProgramPipeline
2/ UseProgramStages
3/ CreateProgramPipelines (???: not sure, but likely)
4/ ActiveShaderProgram
5/ GetProgramPipelineiv
6/ ValidateProgramPipeline
Note: can be read as all pipeline commands, so it is nearly equivalent to
create the object in GenProgramPipelines. The only exception is
IsProgramPipeline.
So GetProgramPipelineiv (and others gl cmd) must start with:
if (! pipe->IsCreated ) {
pipe->Init();
pipe->IsCreated = true;
}
For example:
GenProgramPipelines(1, &pipe);
GetProgramPipelineiv(pipe, X, X); // Must create the object and therefore will
return the initial value (0)
GenProgramPipelines(1, &pipe2);
ValidateProgramPipeline(pipe2); // Will create the object
GetProgramPipelineiv(pipe2, X, X); // object was created by
ValidateProgramPipeline, we can return the property
As a side note, if Mesa is updated. Piglit test will need some updates too.
Best regards,
Gregory
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: implement recent spec update to SSO validation
Thanks. Yes I think it must be this way. I said the same to Tapani 1
hour ago (but stupid gmail didn't reply all...).
On 11/23/15, Timothy Arceri wrote:
> On Mon, 2015-11-23 at 23:24 +1100, Timothy Arceri wrote:
>> From: Timothy Arceri
>>
>> Enables 200+ dEQP SSO tests to proceed passed validation,
>> while not regressing ES31-CTS.sepshaderobjs.PipelineApi.
>>
>> Cc: Tapani Pälli
>> Cc: Gregory Hainaut
>> ---
>> src/mesa/main/pipelineobj.c | 25 -
>> 1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/pipelineobj.c
>> b/src/mesa/main/pipelineobj.c
>> index 90dff13..99e1491 100644
>> --- a/src/mesa/main/pipelineobj.c
>> +++ b/src/mesa/main/pipelineobj.c
>> @@ -646,7 +646,7 @@ _mesa_GetProgramPipelineiv(GLuint pipeline,
>> GLenum pname, GLint *params)
>>return;
>> case GL_VALIDATE_STATUS:
>>/* If pipeline is not bound, return initial value 0. */
>
> Whoops, I've removed this comment locally also.
>
>> - *params = (ctx->_Shader->Name != pipe->Name) ? 0 : pipe
>> ->Validated;
>> + *params = pipe->Validated;
>>return;
>> case GL_VERTEX_SHADER:
>>*params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
>> @@ -858,6 +858,29 @@ _mesa_validate_program_pipeline(struct
>> gl_context* ctx,
>>}
>> }
>>
>> + /* Section 11.1.3.11 (Validation) of the OpenGL 4.5 spec says:
>> +*
>> +*"An INVALID_OPERATION error is generated by any command
>> that trans-
>> +*fers vertices to the GL or launches compute work if the
>> current set
>> +*of active program objects cannot be executed, for reasons
>> including:
>> +*
>> +* ...
>> +*
>> +* - There is no current program object specified by
>> UseProgram,
>> +* there is a current program pipeline object, and that
>> object is
>> +* empty (no executable code is installed for any stage).
>> +*/
>> + bool program_empty = true;
>> + for (i = 0; i < MESA_SHADER_STAGES; i++) {
>> + if (pipe->CurrentProgram[i]) {
>> + program_empty = false;
>> + break;
>> + }
>> + }
>> + if(program_empty) {
>> + goto err;
>> + }
>> +
>> /* Section 2.11.11 (Shader Execution), subheading "Validation,"
>> of the
>> * OpenGL 4.1 spec says:
>> *
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: implement recent spec update to SSO validation
It is a different issue and it seems I already did it (or someone else
patched it).
You can see those lines in the code.
/* Object is created by any Pipeline call but glGenProgramPipelines,
* glIsProgramPipeline and GetProgramPipelineInfoLog
*/
pipe->EverBound = GL_TRUE;
The trick is that object is created by GenProgramPipelines (unlike the
spec said). Only a flag is updated to mark the object as created. And
normally validated must be 0 after the init (rzalloc?).
On 11/23/15, Tapani Pälli wrote:
> Hi;
>
> On 11/23/2015 02:24 PM, Timothy Arceri wrote:
>> From: Timothy Arceri
>>
>> Enables 200+ dEQP SSO tests to proceed passed validation,
>> while not regressing ES31-CTS.sepshaderobjs.PipelineApi.
>>
>> Cc: Tapani Pälli
>> Cc: Gregory Hainaut
>> ---
>> src/mesa/main/pipelineobj.c | 25 -
>> 1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
>> index 90dff13..99e1491 100644
>> --- a/src/mesa/main/pipelineobj.c
>> +++ b/src/mesa/main/pipelineobj.c
>> @@ -646,7 +646,7 @@ _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum
>> pname, GLint *params)
>> return;
>> case GL_VALIDATE_STATUS:
>> /* If pipeline is not bound, return initial value 0. */
>> - *params = (ctx->_Shader->Name != pipe->Name) ? 0 :
>> pipe->Validated;
>> + *params = pipe->Validated;
>
> I agree with the added check below but I feel worried with this change
> as the test is testing explicitly if pipeline was bound before the call,
> here it just happens to be empty as well.
>
> I guess my main worry is that we don't seem to be implementing following:
>
> "If pipeline is a name that has been generated (without subsequent
> deletion) by GenProgramPipelines, but refers to a program pipeline
> object that has not been previously bound, the GL first creates a new
> state vector in the same manner as when BindProgramPipeline creates a
> new program pipeline object."
>
> So we don't realy create 'new state vector' for a new program pipeline
> object but try to return same values as one would have?
>
>
>> return;
>> case GL_VERTEX_SHADER:
>> *params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
>> @@ -858,6 +858,29 @@ _mesa_validate_program_pipeline(struct gl_context*
>> ctx,
>> }
>> }
>>
>> + /* Section 11.1.3.11 (Validation) of the OpenGL 4.5 spec says:
>> +*
>> +*"An INVALID_OPERATION error is generated by any command that
>> trans-
>> +*fers vertices to the GL or launches compute work if the current
>> set
>> +*of active program objects cannot be executed, for reasons
>> including:
>> +*
>> +* ...
>> +*
>> +* - There is no current program object specified by
>> UseProgram,
>> +* there is a current program pipeline object, and that object
>> is
>> +* empty (no executable code is installed for any stage).
>> +*/
>> + bool program_empty = true;
>> + for (i = 0; i < MESA_SHADER_STAGES; i++) {
>> + if (pipe->CurrentProgram[i]) {
>> + program_empty = false;
>> + break;
>> + }
>> + }
>> + if(program_empty) {
>> + goto err;
>> + }
>> +
>> /* Section 2.11.11 (Shader Execution), subheading "Validation," of
>> the
>> * OpenGL 4.1 spec says:
>> *
>>
>
> // Tapani
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 1/3] mesa/glthread: track buffer creation/destruction
It would be used in next commit to allow asynchronous PBO transfer.
The tracking saves the buffer name into a hash. Saving pointer
will be more complex as the buffer is created in BindBuffer due to IsBuffer
insanity.
Perf wise DeleteBuffers is now synchronous for robustness.
v5: properly delete hash element with the help of _mesa_HashDeleteAll
v6: rebase
Signed-off-by: Gregory Hainaut
---
src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +-
src/mapi/glapi/gen/gl_API.xml | 4 +-
src/mesa/main/glthread.h | 10 +++
src/mesa/main/marshal.c| 113 +
src/mesa/main/marshal.h| 24 ++
src/mesa/main/mtypes.h | 5 ++
src/mesa/main/shared.c | 14 +++
7 files changed, 169 insertions(+), 3 deletions(-)
diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml
b/src/mapi/glapi/gen/ARB_direct_state_access.xml
index b8780f75b3..8761920c91 100644
--- a/src/mapi/glapi/gen/ARB_direct_state_access.xml
+++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml
@@ -42,21 +42,21 @@
-
+
diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml
index 762fb5a676..829f99b0f0 100644
--- a/src/mapi/glapi/gen/gl_API.xml
+++ b/src/mapi/glapi/gen/gl_API.xml
@@ -5053,27 +5053,27 @@
-
+
-
+
diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h
index 50c1db2548..494e94270a 100644
--- a/src/mesa/main/glthread.h
+++ b/src/mesa/main/glthread.h
@@ -85,20 +85,30 @@ struct glthread_state
* Tracks on the main thread side whether the current vertex array binding
* is in a VBO.
*/
bool vertex_array_is_vbo;
/**
* Tracks on the main thread side whether the current element array (index
* buffer) binding is in a VBO.
*/
bool element_array_is_vbo;
+
+ /**
+* Tracks on the main thread side the bound unpack pixel buffer
+*/
+ GLint pixel_unpack_buffer_bound;
+
+ /**
+* Tracks on the main thread side the bound pack pixel buffer
+*/
+ GLint pixel_pack_buffer_bound;
};
/**
* A single batch of commands queued up for later execution by a thread pool
* task.
*/
struct glthread_batch
{
/**
* Next batch of commands to execute after this batch, or NULL if this is
diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c
index ae4efb5ecb..4037b793ee 100644
--- a/src/mesa/main/marshal.c
+++ b/src/mesa/main/marshal.c
@@ -25,20 +25,21 @@
*
* Custom functions for marshalling GL calls from the main thread to a worker
* thread when automatic code generation isn't appropriate.
*/
#include "main/enums.h"
#include "main/macros.h"
#include "marshal.h"
#include "dispatch.h"
#include "marshal_generated.h"
+#include "hash.h"
#ifdef HAVE_PTHREAD
struct marshal_cmd_Flush
{
struct marshal_cmd_base cmd_base;
};
void
@@ -189,20 +190,132 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count,
}
_mesa_post_marshal_hook(ctx);
} else {
_mesa_glthread_finish(ctx);
CALL_ShaderSource(ctx->CurrentServerDispatch,
(shader, count, string, length_tmp));
}
free(length_tmp);
}
+/**
+ * Used as a placeholder for track_buffers_creation/track_buffers_destruction
+ * so we know if buffer really exists in track_buffers_binding.
+ */
+static struct gl_buffer_object DummyBufferObject;
+
+static void track_buffers_creation(GLsizei n, GLuint * buffers)
+{
+ GET_CURRENT_CONTEXT(ctx);
+ GLsizei i;
+
+ if (n < 0 || !buffers)
+ return;
+
+ _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects);
+
+ for (i = 0; i < n ; i++) {
+ _mesa_HashInsertLocked(ctx->Shared->ShadowBufferObjects, buffers[i],
+&DummyBufferObject);
+ }
+
+ _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects);
+}
+
+static void track_buffers_destruction(GLsizei n, const GLuint * buffers)
+{
+ GET_CURRENT_CONTEXT(ctx);
+ GLsizei i;
+ struct glthread_state *glthread = ctx->GLThread;
+
+ if (n < 0 || !buffers)
+ return;
+
+ _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects);
+
+ for (i = 0; i < n ; i++) {
+ if (buffers[i] == glthread->pixel_pack_buffer_bound)
+ glthread->pixel_pack_buffer_bound = 0;
+
+ if (buffers[i] == glthread->pixel_unpack_buffer_bound)
+ glthread->pixel_unpack_buffer_bound = 0;
+
+ /* Technically the buffer can still exist if it is bound to another
+ * context. It isn't important as nex
[Mesa-dev] [PATCH v6 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 23 - src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 8761920c91..6ae1a6c249 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0a74..6e1ac09ce0 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250777..447b03a41d 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single" commands vendorpriv -
[Mesa-dev] [PATCH v4 3/4] egl: implement __DRIbackgroundCallableExtension.isThreadSafe
v2:
bump version
v3:
Add code comment
s/IsGlThread/IsThread/ (and variation)
Include X11/Xlibint.h protected by ifdef
Signed-off-by: Gregory Hainaut
---
src/egl/drivers/dri2/egl_dri2.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 0be7132ac5..3295d472f4 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -48,20 +48,24 @@
#include
#include "GL/mesa_glinterop.h"
#include
#include
#ifdef HAVE_WAYLAND_PLATFORM
#include "wayland-drm.h"
#include "wayland-drm-client-protocol.h"
#endif
+#ifdef HAVE_X11_PLATFORM
+#include "X11/Xlibint.h"
+#endif
+
#include "egl_dri2.h"
#include "loader/loader.h"
#include "util/u_atomic.h"
/* The kernel header drm_fourcc.h defines the DRM formats below. We duplicate
* some of the definitions here so that building Mesa won't bleeding-edge
* kernel headers.
*/
#ifndef DRM_FORMAT_R8
#define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
@@ -85,24 +89,46 @@
static void
dri_set_background_context(void *loaderPrivate)
{
_EGLContext *ctx = _eglGetCurrentContext();
_EGLThreadInfo *t = _eglGetCurrentThread();
_eglBindContextToThread(ctx, t);
}
+static GLboolean
+dri_is_thread_safe(void *loaderPrivate)
+{
+#ifdef HAVE_X11_PLATFORM
+ struct dri2_egl_surface *dri2_surf = loaderPrivate;
+ _EGLDisplay *display = dri2_surf->base.Resource.Display;
+ Display *xdpy = (Display*)display->PlatformDisplay;
+
+ /* Check Xlib is running in thread safe mode when running on EGL/X11-xlib
+* platform
+*
+* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'.
+* It wll be NULL if XInitThreads wasn't called.
+*/
+ if (display->Platform == _EGL_PLATFORM_X11 && xdpy && !xdpy->lock_fns)
+ return false;
+#endif
+
+ return true;
+}
+
const __DRIbackgroundCallableExtension background_callable_extension = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext = dri_set_background_context,
+ .isThreadSafe = dri_is_thread_safe,
};
const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const EGLint dri2_to_egl_attribute_map[__DRI_ATTRIB_MAX] = {
[__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE,
[__DRI_ATTRIB_LEVEL] = EGL_LEVEL,
[__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE,
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 0/4] Disable glthread if libX11 isn't thread-safe
Hello Mesa developers, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) v3: based on Emil reviews Split the initial first patch into 3 sub patches dri extension / glx / egl Improve error message Improve code readability Just include libX11 on EGL protected by ifdef v4: based on Eric feedback, I marked DRI3 as always thread safe Thanks you for all the review comments. Best regards, Gregory Hainaut (4): dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety glx: implement __DRIbackgroundCallableExtension.isThreadSafe egl: implement __DRIbackgroundCallableExtension.isThreadSafe glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 11 +++ src/egl/drivers/dri2/egl_dri2.c | 28 +++- src/gallium/state_trackers/dri/dri_context.c | 21 + src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 12 +++- 5 files changed, 80 insertions(+), 7 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 0/3] asynchronous pbo transfer with glthread
Hello Mesa developers, Please find a new version to handle invalid buffer handles. Allow to handle this kind of case: genBuffer(&pbo); BindBuffer(pbo) DeleteBuffer(pbo); BindBuffer(rand_pbo) TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous There are various subtely to handle multi threaded shared context. In order to keep the code sane, I've considered a buffer invalid when it is deleted by a context even it is still bound to others contexts. It will force a synchronous transfer which is always safe. An example could be Ctx A: glGenBuffers(1, &pbo); Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); Ctx B: glDeleteBuffers(1, &pbo); Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ be asynchronous (because the PBO that was generated first is still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) v6: Rebase on latest master Note: crash related to unsafe X call will be handled by "Disable glthread if libX11 isn't thread-safe" series Best regards, Gregory Hainaut (3): mesa/glthread: track buffer creation/destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 +- src/mapi/glapi/gen/gl_API.xml | 32 +++--- src/mapi/glapi/gen/gl_marshal.py | 23 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +++- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c| 149 - src/mesa/main/marshal.h| 24 src/mesa/main/mtypes.h | 5 + src/mesa/main/shared.c | 14 +++ 11 files changed, 269 insertions(+), 39 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers
to be valid.
v4: update comments based on Nicolai review
Signed-off-by: Gregory Hainaut
---
src/mesa/main/marshal.c | 36 +---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c
index 4037b793ee..1167271607 100644
--- a/src/mesa/main/marshal.c
+++ b/src/mesa/main/marshal.c
@@ -312,21 +312,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx,
/* BindBufferBase: marshalled asynchronously */
struct marshal_cmd_BindBufferBase
{
struct marshal_cmd_base cmd_base;
GLenum target;
GLuint index;
GLuint buffer;
};
-/** Tracks the current bindings for the vertex array and index array buffers.
+/**
+ * Check that buffer is a valid buffer handle
+ * Always return false for ID 0.
+ */
+static bool
+is_bufferobj(struct gl_context *ctx, GLuint buffer)
+{
+ if (buffer == 0)
+ return false;
+ else
+ return _mesa_HashLookup(ctx->Shared->ShadowBufferObjects, buffer) !=
NULL;
+}
+
+/** Tracks the current bindings of GL buffer targets
*
* This is part of what we need to enable glthread on compat-GL contexts that
* happen to use VBOs, without also supporting the full tracking of VBO vs
* user vertex array bindings per attribute on each vertex array for
* determining what to upload at draw call time.
*
* Note that GL core makes it so that a buffer binding with an invalid handle
* in the "buffer" parameter will throw an error, and then a
* glVertexAttribPointer() that followsmight not end up pointing at a VBO.
* However, in GL core the draw call would throw an error as well, so we don't
@@ -334,37 +347,54 @@ struct marshal_cmd_BindBufferBase
* marshal user data for draw calls, and the unmarshal will just generate an
* error or not as appropriate.
*
* For compatibility GL, we do need to accurately know whether the draw call
* on the unmarshal side will dereference a user pointer or load data from a
* VBO per vertex. That would make it seem like we need to track whether a
* "buffer" is valid, so that we can know when an error will be generated
* instead of updating the binding. However, compat GL has the ridiculous
* feature that if you pass a bad name, it just gens a buffer object for you,
* so we escape without having to know if things are valid or not.
+ *
+ * Pixel buffers are tracked to decide whether pixel transfer goes to a user
+ * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike
+ * for VBOs, we do need accurate tracking, since user pointers can be used in
+ * GL core contexts.
*/
static void
-track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer)
+track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer)
{
struct glthread_state *glthread = ctx->GLThread;
switch (target) {
case GL_ARRAY_BUFFER:
glthread->vertex_array_is_vbo = (buffer != 0);
break;
case GL_ELEMENT_ARRAY_BUFFER:
/* The current element array buffer binding is actually tracked in the
* vertex array object instead of the context, so this would need to
* change on vertex array object updates.
*/
glthread->element_array_is_vbo = (buffer != 0);
break;
+ case GL_PIXEL_UNPACK_BUFFER:
+ if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer))
+ glthread->pixel_unpack_buffer_bound = buffer;
+ else
+ glthread->pixel_unpack_buffer_bound = 0;
+ break;
+ case GL_PIXEL_PACK_BUFFER:
+ if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer))
+ glthread->pixel_pack_buffer_bound = buffer;
+ else
+ glthread->pixel_pack_buffer_bound = 0;
+ break;
}
}
struct marshal_cmd_BindBuffer
{
struct marshal_cmd_base cmd_base;
GLenum target;
GLuint buffer;
};
@@ -382,21 +412,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx,
CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer));
}
void GLAPIENTRY
_mesa_marshal_BindBuffer(GLenum target, GLuint buffer)
{
GET_CURRENT_CONTEXT(ctx);
size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer);
struct marshal_cmd_BindBuffer *cmd;
debug_print_marshal("BindBuffer");
- track_vbo_binding(ctx, target, buffer);
+ track_buffers_binding(ctx, target, buffer);
if (cmd_size <= MARSHAL_MAX_CMD_SIZE) {
cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer,
cmd_size);
cmd->target = target;
cmd->buffer = buffer;
_mesa_post_marshal_hook(ctx);
} else {
_mesa_glthread_finish(ctx);
CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer));
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 4/4] glthread/gallium: require safe_glthread to start glthread
Print an error message for the user if the requirement isn't met, or
we're not thread safe.
v2: based on Nicolai feedbacks
Check the DRI extension version
v3: based on Emil feedbacks
improve commit and error messages.
use backgroundCallable variable to improve readability
Signed-off-by: Gregory Hainaut
---
src/gallium/state_trackers/dri/dri_context.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/gallium/state_trackers/dri/dri_context.c
b/src/gallium/state_trackers/dri/dri_context.c
index 92d79849c4..8cbab5359f 100644
--- a/src/gallium/state_trackers/dri/dri_context.c
+++ b/src/gallium/state_trackers/dri/dri_context.c
@@ -51,20 +51,22 @@ dri_create_context(gl_api api, const struct gl_config *
visual,
{
__DRIscreen *sPriv = cPriv->driScreenPriv;
struct dri_screen *screen = dri_screen(sPriv);
struct st_api *stapi = screen->st_api;
struct dri_context *ctx = NULL;
struct st_context_iface *st_share = NULL;
struct st_context_attribs attribs;
enum st_context_error ctx_err = 0;
unsigned allowed_flags = __DRI_CTX_FLAG_DEBUG |
__DRI_CTX_FLAG_FORWARD_COMPATIBLE;
+ const __DRIbackgroundCallableExtension *backgroundCallable =
+ screen->sPriv->dri2.backgroundCallable;
if (screen->has_reset_status_query)
allowed_flags |= __DRI_CTX_FLAG_ROBUST_BUFFER_ACCESS;
if (flags & ~allowed_flags) {
*error = __DRI_CTX_ERROR_UNKNOWN_FLAG;
goto fail;
}
if (!screen->has_reset_status_query && notify_reset) {
@@ -151,24 +153,35 @@ dri_create_context(gl_api api, const struct gl_config *
visual,
ctx->st->st_manager_private = (void *) ctx;
ctx->stapi = stapi;
if (ctx->st->cso_context) {
ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled,
ctx->st->cso_context);
ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context);
}
/* Do this last. */
if (ctx->st->start_thread &&
- /* the driver loader must implement this */
- screen->sPriv->dri2.backgroundCallable &&
- driQueryOptionb(&screen->optionCache, "mesa_glthread"))
- ctx->st->start_thread(ctx->st);
+ driQueryOptionb(&screen->optionCache, "mesa_glthread")) {
+
+ if (backgroundCallable && backgroundCallable->base.version >= 2 &&
+driQueryOptionb(&screen->optionCache, "mesa_glthread")) {
+
+ if (backgroundCallable->isThreadSafe(cPriv->loaderPrivate))
+ctx->st->start_thread(ctx->st);
+ else
+fprintf(stderr, "dri_create_context: glthread isn't thread safe "
+ "- missing call XInitThreads\n");
+ } else {
+ fprintf(stderr, "dri_create_context: requested glthread but driver "
+ "is missing backgroundCallable V2 extension\n");
+ }
+ }
*error = __DRI_CTX_ERROR_SUCCESS;
return GL_TRUE;
fail:
if (ctx && ctx->st)
ctx->st->destroy(ctx->st);
free(ctx);
return GL_FALSE;
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 2/4] glx: implement __DRIbackgroundCallableExtension.isThreadSafe
v2:
bump version
v3:
Add code comment
s/IsGlThread/IsThread/ (and variation)
v4:
DRI3 doesn't hit X through GL call so it is always safe
Signed-off-by: Gregory Hainaut
---
src/glx/dri2_glx.c | 15 ++-
src/glx/dri3_glx.c | 12 +++-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..4f163688f2 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -946,20 +946,32 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw)
return priv->swap_interval;
}
static void
driSetBackgroundContext(void *loaderPrivate)
{
struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
__glXSetCurrentContext(&pcp->base);
}
+static GLboolean
+driIsThreadSafe(void *loaderPrivate)
+{
+ struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
+ /* Check Xlib is running in thread safe mode
+*
+* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'.
+* It wll be NULL if XInitThreads wasn't called.
+*/
+ return pcp->base.psc->dpy->lock_fns != NULL;
+}
+
static const __DRIdri2LoaderExtension dri2LoaderExtension = {
.base = { __DRI_DRI2_LOADER, 3 },
.getBuffers = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= dri2GetBuffersWithFormat,
};
static const __DRIdri2LoaderExtension dri2LoaderExtension_old = {
.base = { __DRI_DRI2_LOADER, 3 },
@@ -967,23 +979,24 @@ static const __DRIdri2LoaderExtension
dri2LoaderExtension_old = {
.getBuffers = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= NULL,
};
static const __DRIuseInvalidateExtension dri2UseInvalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const __DRIbackgroundCallableExtension driBackgroundCallable = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext= driSetBackgroundContext,
+ .isThreadSafe= driIsThreadSafe,
};
_X_HIDDEN void
dri2InvalidateBuffers(Display *dpy, XID drawable)
{
__GLXDRIdrawable *pdraw =
dri2GetGlxDrawableFromXDrawableId(dpy, drawable);
struct dri2_screen *psc;
struct dri2_drawable *pdp = (struct dri2_drawable *) pdraw;
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index e1dc5aa4a8..d07968e3c5 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -496,37 +496,47 @@ dri3_flush_front_buffer(__DRIdrawable *driDrawable, void
*loaderPrivate)
loader_dri3_wait_gl(draw);
}
static void
dri_set_background_context(void *loaderPrivate)
{
struct dri3_context *pcp = (struct dri3_context *)loaderPrivate;
__glXSetCurrentContext(&pcp->base);
}
+static GLboolean
+dri_is_thread_safe(void *loaderPrivate)
+{
+ /* Unlike DRI2, DRI3 doesn't call GetBuffers/GetBuffersWithFormat
+* during draw so we're safe here.
+*/
+ return true;
+}
+
/* The image loader extension record for DRI3
*/
static const __DRIimageLoaderExtension imageLoaderExtension = {
.base = { __DRI_IMAGE_LOADER, 1 },
.getBuffers = loader_dri3_get_buffers,
.flushFrontBuffer= dri3_flush_front_buffer,
};
const __DRIuseInvalidateExtension dri3UseInvalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const __DRIbackgroundCallableExtension driBackgroundCallable = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext = dri_set_background_context,
+ .isThreadSafe = dri_is_thread_safe,
};
static const __DRIextension *loader_extensions[] = {
&imageLoaderExtension.base,
&dri3UseInvalidate.base,
&driBackgroundCallable.base,
NULL
};
/** dri3_swap_buffers
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 1/4] dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety
DRI-drivers could call Xlib functions, for example to allocate a new back
buffer.
When glthread is enabled, the driver runs mostly on a separate thread.
Therefore we need to guarantee the thread safety between libX11 calls
from the applications (not aware of the extra thread) and the ones from
the driver.
See discussion thread:
https://lists.freedesktop.org/archives/mesa-dev/2017-April/152547.html
Fortunately, Xlib allows to lock display to ensure thread safety but
XInitThreads must be called first by the application to initialize the lock
function pointer. This patch will allow to check XInitThreads was called
to allow glthread on GLX or EGL platform.
Note: a tentative was done to port libX11 code to XCB but it didn't solve fully
thread safety.
See discussion thread:
https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html
Note: Nvidia forces the driver to call XInitThreads. Quoting their manpage:
"The NVIDIA OpenGL driver will automatically attempt to enable Xlib
thread-safe mode if needed. However, it might not be possible in some
situations, such as when the NVIDIA OpenGL driver library is dynamically
loaded after Xlib has been loaded and initialized. If that is the case,
threaded optimizations will stay disabled unless the application is
modified to call XInitThreads() before initializing Xlib or to link
directly against the NVIDIA OpenGL driver library. Alternatively, using
the LD_PRELOAD environment variable to include the NVIDIA OpenGL driver
library should also achieve the desired result."
v2: based on Nicolai and Matt feedback
Use C style comment
v3: based on Emil feedback
split the patch in 3
s/isGlThreadSafe/isThreadSafe/
Signed-off-by: Gregory Hainaut
---
include/GL/internal/dri_interface.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/include/GL/internal/dri_interface.h
b/include/GL/internal/dri_interface.h
index c83056aa70..8381f704e4 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1714,13 +1714,24 @@ struct __DRIbackgroundCallableExtensionRec {
* non-background thread (i.e. a thread that has already been bound to a
* context using __DRIcoreExtensionRec::bindContext()); when this happens,
* the \c loaderPrivate pointer must be equal to the pointer that was
* passed to the driver when the currently bound context was created.
*
* This call should execute quickly enough that the driver can call it with
* impunity whenever a background thread starts performing drawing
* operations (e.g. it should just set a thread-local variable).
*/
void (*setBackgroundContext)(void *loaderPrivate);
+
+ /**
+* Indicate that it is multithread safe to use glthread. For GLX/EGL
+* platforms using Xlib, that involves calling XInitThreads, before
+* opening an X display.
+*
+* \param loaderPrivate is the value that was passed to to the driver when
+* the context was created. This can be used by the loader to identify
+* which context any callbacks are associated with.
+*/
+ GLboolean (*isThreadSafe)(void *loaderPrivate);
};
#endif
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 1/4] dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety
DRI-drivers could call Xlib functions, for example to allocate a new back
buffer.
When glthread is enabled, the driver runs mostly on a separate thread.
Therefore we need to guarantee the thread safety between libX11 calls
from the applications (not aware of the extra thread) and the ones from
the driver.
See discussion thread:
https://lists.freedesktop.org/archives/mesa-dev/2017-April/152547.html
Fortunately, Xlib allows to lock display to ensure thread safety but
XInitThreads must be called first by the application to initialize the lock
function pointer. This patch will allow to check XInitThreads was called
to allow glthread on GLX or EGL platform.
Note: a tentative was done to port libX11 code to XCB but it didn't solve fully
thread safety.
See discussion thread:
https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html
Note: Nvidia forces the driver to call XInitThreads. Quoting their manpage:
"The NVIDIA OpenGL driver will automatically attempt to enable Xlib
thread-safe mode if needed. However, it might not be possible in some
situations, such as when the NVIDIA OpenGL driver library is dynamically
loaded after Xlib has been loaded and initialized. If that is the case,
threaded optimizations will stay disabled unless the application is
modified to call XInitThreads() before initializing Xlib or to link
directly against the NVIDIA OpenGL driver library. Alternatively, using
the LD_PRELOAD environment variable to include the NVIDIA OpenGL driver
library should also achieve the desired result."
v2: based on Nicolai and Matt feedback
Use C style comment
v3: based on Emil feedback
split the patch in 3
s/isGlThreadSafe/isThreadSafe/
v5: based on Marek comment
Add a comment that isThreadSafe is supported by extension v2
Signed-off-by: Gregory Hainaut
---
include/GL/internal/dri_interface.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/GL/internal/dri_interface.h
b/include/GL/internal/dri_interface.h
index c83056aa70..ffe99499fc 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1714,13 +1714,26 @@ struct __DRIbackgroundCallableExtensionRec {
* non-background thread (i.e. a thread that has already been bound to a
* context using __DRIcoreExtensionRec::bindContext()); when this happens,
* the \c loaderPrivate pointer must be equal to the pointer that was
* passed to the driver when the currently bound context was created.
*
* This call should execute quickly enough that the driver can call it with
* impunity whenever a background thread starts performing drawing
* operations (e.g. it should just set a thread-local variable).
*/
void (*setBackgroundContext)(void *loaderPrivate);
+
+ /**
+* Indicate that it is multithread safe to use glthread. For GLX/EGL
+* platforms using Xlib, that involves calling XInitThreads, before
+* opening an X display.
+*
+* Note: only supported if extension version is at least 2.
+*
+* \param loaderPrivate is the value that was passed to to the driver when
+* the context was created. This can be used by the loader to identify
+* which context any callbacks are associated with.
+*/
+ GLboolean (*isThreadSafe)(void *loaderPrivate);
};
#endif
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 2/4] glx: implement __DRIbackgroundCallableExtension.isThreadSafe
v2:
bump version
v3:
Add code comment
s/IsGlThread/IsThread/ (and variation)
v4:
DRI3 doesn't hit X through GL call so it is always safe
Signed-off-by: Gregory Hainaut
---
src/glx/dri2_glx.c | 15 ++-
src/glx/dri3_glx.c | 12 +++-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..4f163688f2 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -946,20 +946,32 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw)
return priv->swap_interval;
}
static void
driSetBackgroundContext(void *loaderPrivate)
{
struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
__glXSetCurrentContext(&pcp->base);
}
+static GLboolean
+driIsThreadSafe(void *loaderPrivate)
+{
+ struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
+ /* Check Xlib is running in thread safe mode
+*
+* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'.
+* It wll be NULL if XInitThreads wasn't called.
+*/
+ return pcp->base.psc->dpy->lock_fns != NULL;
+}
+
static const __DRIdri2LoaderExtension dri2LoaderExtension = {
.base = { __DRI_DRI2_LOADER, 3 },
.getBuffers = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= dri2GetBuffersWithFormat,
};
static const __DRIdri2LoaderExtension dri2LoaderExtension_old = {
.base = { __DRI_DRI2_LOADER, 3 },
@@ -967,23 +979,24 @@ static const __DRIdri2LoaderExtension
dri2LoaderExtension_old = {
.getBuffers = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= NULL,
};
static const __DRIuseInvalidateExtension dri2UseInvalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const __DRIbackgroundCallableExtension driBackgroundCallable = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext= driSetBackgroundContext,
+ .isThreadSafe= driIsThreadSafe,
};
_X_HIDDEN void
dri2InvalidateBuffers(Display *dpy, XID drawable)
{
__GLXDRIdrawable *pdraw =
dri2GetGlxDrawableFromXDrawableId(dpy, drawable);
struct dri2_screen *psc;
struct dri2_drawable *pdp = (struct dri2_drawable *) pdraw;
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index e1dc5aa4a8..d07968e3c5 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -496,37 +496,47 @@ dri3_flush_front_buffer(__DRIdrawable *driDrawable, void
*loaderPrivate)
loader_dri3_wait_gl(draw);
}
static void
dri_set_background_context(void *loaderPrivate)
{
struct dri3_context *pcp = (struct dri3_context *)loaderPrivate;
__glXSetCurrentContext(&pcp->base);
}
+static GLboolean
+dri_is_thread_safe(void *loaderPrivate)
+{
+ /* Unlike DRI2, DRI3 doesn't call GetBuffers/GetBuffersWithFormat
+* during draw so we're safe here.
+*/
+ return true;
+}
+
/* The image loader extension record for DRI3
*/
static const __DRIimageLoaderExtension imageLoaderExtension = {
.base = { __DRI_IMAGE_LOADER, 1 },
.getBuffers = loader_dri3_get_buffers,
.flushFrontBuffer= dri3_flush_front_buffer,
};
const __DRIuseInvalidateExtension dri3UseInvalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const __DRIbackgroundCallableExtension driBackgroundCallable = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext = dri_set_background_context,
+ .isThreadSafe = dri_is_thread_safe,
};
static const __DRIextension *loader_extensions[] = {
&imageLoaderExtension.base,
&dri3UseInvalidate.base,
&driBackgroundCallable.base,
NULL
};
/** dri3_swap_buffers
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 4/4] glthread/gallium: require safe_glthread to start glthread
Print an error message for the user if the requirement isn't met, or
we're not thread safe.
v2: based on Nicolai feedbacks
Check the DRI extension version
v3: based on Emil feedbacks
improve commit and error messages.
use backgroundCallable variable to improve readability
v5: based on Emil feedbacks
Properly check the function pointer
Signed-off-by: Gregory Hainaut
---
src/gallium/state_trackers/dri/dri_context.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/gallium/state_trackers/dri/dri_context.c
b/src/gallium/state_trackers/dri/dri_context.c
index 92d79849c4..ec555e44d7 100644
--- a/src/gallium/state_trackers/dri/dri_context.c
+++ b/src/gallium/state_trackers/dri/dri_context.c
@@ -51,20 +51,22 @@ dri_create_context(gl_api api, const struct gl_config *
visual,
{
__DRIscreen *sPriv = cPriv->driScreenPriv;
struct dri_screen *screen = dri_screen(sPriv);
struct st_api *stapi = screen->st_api;
struct dri_context *ctx = NULL;
struct st_context_iface *st_share = NULL;
struct st_context_attribs attribs;
enum st_context_error ctx_err = 0;
unsigned allowed_flags = __DRI_CTX_FLAG_DEBUG |
__DRI_CTX_FLAG_FORWARD_COMPATIBLE;
+ const __DRIbackgroundCallableExtension *backgroundCallable =
+ screen->sPriv->dri2.backgroundCallable;
if (screen->has_reset_status_query)
allowed_flags |= __DRI_CTX_FLAG_ROBUST_BUFFER_ACCESS;
if (flags & ~allowed_flags) {
*error = __DRI_CTX_ERROR_UNKNOWN_FLAG;
goto fail;
}
if (!screen->has_reset_status_query && notify_reset) {
@@ -151,24 +153,35 @@ dri_create_context(gl_api api, const struct gl_config *
visual,
ctx->st->st_manager_private = (void *) ctx;
ctx->stapi = stapi;
if (ctx->st->cso_context) {
ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled,
ctx->st->cso_context);
ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context);
}
/* Do this last. */
if (ctx->st->start_thread &&
- /* the driver loader must implement this */
- screen->sPriv->dri2.backgroundCallable &&
- driQueryOptionb(&screen->optionCache, "mesa_glthread"))
- ctx->st->start_thread(ctx->st);
+ driQueryOptionb(&screen->optionCache, "mesa_glthread")) {
+
+ if (backgroundCallable && backgroundCallable->base.version >= 2 &&
+backgroundCallable->isThreadSafe) {
+
+ if (backgroundCallable->isThreadSafe(cPriv->loaderPrivate))
+ctx->st->start_thread(ctx->st);
+ else
+fprintf(stderr, "dri_create_context: glthread isn't thread safe "
+ "- missing call XInitThreads\n");
+ } else {
+ fprintf(stderr, "dri_create_context: requested glthread but driver "
+ "is missing backgroundCallable V2 extension\n");
+ }
+ }
*error = __DRI_CTX_ERROR_SUCCESS;
return GL_TRUE;
fail:
if (ctx && ctx->st)
ctx->st->destroy(ctx->st);
free(ctx);
return GL_FALSE;
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 3/4] egl: implement __DRIbackgroundCallableExtension.isThreadSafe
v2:
bump version
v3:
Add code comment
s/IsGlThread/IsThread/ (and variation)
Include X11/Xlibint.h protected by ifdef
v5: based on Daniel feedback
Move non X11 code outside of X11 define
Always return true for Wayland
Signed-off-by: Gregory Hainaut
---
src/egl/drivers/dri2/egl_dri2.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 0be7132ac5..8891771e3f 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -48,20 +48,24 @@
#include
#include "GL/mesa_glinterop.h"
#include
#include
#ifdef HAVE_WAYLAND_PLATFORM
#include "wayland-drm.h"
#include "wayland-drm-client-protocol.h"
#endif
+#ifdef HAVE_X11_PLATFORM
+#include "X11/Xlibint.h"
+#endif
+
#include "egl_dri2.h"
#include "loader/loader.h"
#include "util/u_atomic.h"
/* The kernel header drm_fourcc.h defines the DRM formats below. We duplicate
* some of the definitions here so that building Mesa won't bleeding-edge
* kernel headers.
*/
#ifndef DRM_FORMAT_R8
#define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
@@ -85,24 +89,52 @@
static void
dri_set_background_context(void *loaderPrivate)
{
_EGLContext *ctx = _eglGetCurrentContext();
_EGLThreadInfo *t = _eglGetCurrentThread();
_eglBindContextToThread(ctx, t);
}
+static GLboolean
+dri_is_thread_safe(void *loaderPrivate)
+{
+ struct dri2_egl_surface *dri2_surf = loaderPrivate;
+ _EGLDisplay *display = dri2_surf->base.Resource.Display;
+
+#ifdef HAVE_X11_PLATFORM
+ Display *xdpy = (Display*)display->PlatformDisplay;
+
+ /* Check Xlib is running in thread safe mode when running on EGL/X11-xlib
+* platform
+*
+* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'.
+* It wll be NULL if XInitThreads wasn't called.
+*/
+ if (display->Platform == _EGL_PLATFORM_X11 && xdpy && !xdpy->lock_fns)
+ return false;
+#endif
+
+#ifdef HAVE_WAYLAND_PLATFORM
+ if (display->Platform == _EGL_PLATFORM_WAYLAND)
+ return true;
+#endif
+
+ return true;
+}
+
const __DRIbackgroundCallableExtension background_callable_extension = {
- .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+ .base = { __DRI_BACKGROUND_CALLABLE, 2 },
.setBackgroundContext = dri_set_background_context,
+ .isThreadSafe = dri_is_thread_safe,
};
const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
static const EGLint dri2_to_egl_attribute_map[__DRI_ATTRIB_MAX] = {
[__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE,
[__DRI_ATTRIB_LEVEL] = EGL_LEVEL,
[__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE,
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 0/4] Disable glthread if libX11 isn't thread-safe
Hello Mesa developers, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) v3: based on Emil reviews Split the initial first patch into 3 sub patches dri extension / glx / egl Improve error message Improve code readability Just include libX11 on EGL protected by ifdef v4: based on Eric feedback, I marked DRI3 as always thread safe v5: Fix the null pointer check on patch 4. I added Daniel comment on patch 3 but I'm not sure I got it right. Thanks you for all the review comments. Best regards, Gregory Hainaut (4): dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety glx: implement __DRIbackgroundCallableExtension.isThreadSafe egl: implement __DRIbackgroundCallableExtension.isThreadSafe glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 13 +++ src/egl/drivers/dri2/egl_dri2.c | 34 +++- src/gallium/state_trackers/dri/dri_context.c | 21 + src/glx/dri2_glx.c | 15 +++- src/glx/dri3_glx.c | 12 +- 5 files changed, 88 insertions(+), 7 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 0/4] Disable glthread if libX11 isn't thread-safe
On Mon, 29 May 2017 17:12:05 +0100 Emil Velikov wrote: > On 29 May 2017 at 15:45, Dieter Nützel wrote: > > Hi Gregory, > > > > there isn't currently a copy of this on Mesa-Patchwork. > > Can you please send one over there? > > > > And maybe an updated version of: > > [PATCH v5 0/3] asynchronous pbo transfer with glthread > > > > Would be awesome. > > Isn't Mesa-Patchwork a bot that parse the mail from the mailing list ? Reading Mesa doc, there is only a note that "patches must be marked superseeded manually" > The series is in master now, so it should be a bit easier ;-) > > Gregory, thanks again for the work. Please keep an eye open for > reports. it's highly unlikely but still. > > -Emil Thanks you for the review and the push :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v8 0/3] asynchronous pbo transfer with glthread
Hello Mesa developers, > Please find a new version to handle invalid buffer handles. > > Allow to handle this kind of case: >genBuffer(&pbo); >BindBuffer(pbo) >DeleteBuffer(pbo); >BindBuffer(rand_pbo) >TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous > > There are various subtely to handle multi threaded shared context. In order to > keep the code sane, I've considered a buffer invalid when it is deleted by a > context even it is still bound to others contexts. It will force a synchronous > transfer which is always safe. > > An example could be >Ctx A: glGenBuffers(1, &pbo); >Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); >Ctx B: glDeleteBuffers(1, &pbo); >Ctx A: glTexSubImage2D(...); // will be synchronous, even though it >_could_ be asynchronous (because the PBO that was generated first is >still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) v6: Rebase on latest master v7: Fredrik's suggestion (remove shadow hash table and rename pixel_*pack_buffer_bound variables) I rebased the code on latest master, it got extra conflict (gl_marshal.py) since the resent from Timothy v8: rebase on latest master Best regards, Gregory Hainaut (3): mesa/glthread: track buffer destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++--- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 ++-- src/mapi/glapi/gen/gl_API.xml | 30 +- src/mapi/glapi/gen/gl_marshal.py | 24 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +-- src/mesa/main/glthread.h | 10 src/mesa/main/marshal.c| 76 +- src/mesa/main/marshal.h| 8 +++ 9 files changed, 159 insertions(+), 38 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v8 1/3] mesa/glthread: track buffer destruction
It would be used in following commits to allow asynchronous PBO transfer.
The tracking saves the buffer name into a hash. Saving pointer
will be more complex as the buffer is created in BindBuffer due to IsBuffer
insanity.
Perf wise DeleteBuffers is now synchronous for robustness.
v5: properly delete hash element with the help of _mesa_HashDeleteAll
v6: rebase
v7: rebase
s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/
Drop the ShadowBufferObjects hash. Synchronous creation/destruction
gives us enough guarantee to lookup the BufferObjects hash directly.
Drop custom code for GenBuffers/CreateBuffers
v8: rebase
Signed-off-by: Gregory Hainaut
---
src/mapi/glapi/gen/gl_API.xml | 2 +-
src/mesa/main/glthread.h | 10 ++
src/mesa/main/marshal.c | 40
src/mesa/main/marshal.h | 8
4 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml
index 18839ec70c..4b01ca552f 100644
--- a/src/mapi/glapi/gen/gl_API.xml
+++ b/src/mapi/glapi/gen/gl_API.xml
@@ -5055,21 +5055,21 @@
-
+
diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h
index 306246ca1c..e5c8b79a97 100644
--- a/src/mesa/main/glthread.h
+++ b/src/mesa/main/glthread.h
@@ -88,20 +88,30 @@ struct glthread_state
* Tracks on the main thread side whether the current vertex array binding
* is in a VBO.
*/
bool vertex_array_is_vbo;
/**
* Tracks on the main thread side whether the current element array (index
* buffer) binding is in a VBO.
*/
bool element_array_is_vbo;
+
+ /**
+* Tracks on the main thread side the bound unpack pixel buffer
+*/
+ GLuint bound_pixel_unpack_buffer;
+
+ /**
+* Tracks on the main thread side the bound pack pixel buffer
+*/
+ GLuint bound_pixel_pack_buffer;
};
void _mesa_glthread_init(struct gl_context *ctx);
void _mesa_glthread_destroy(struct gl_context *ctx);
void _mesa_glthread_restore_dispatch(struct gl_context *ctx);
void _mesa_glthread_flush_batch(struct gl_context *ctx);
void _mesa_glthread_finish(struct gl_context *ctx);
#endif /* _GLTHREAD_H*/
diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c
index 8f8e8c78ed..1914b5 100644
--- a/src/mesa/main/marshal.c
+++ b/src/mesa/main/marshal.c
@@ -25,20 +25,21 @@
*
* Custom functions for marshalling GL calls from the main thread to a worker
* thread when automatic code generation isn't appropriate.
*/
#include "main/enums.h"
#include "main/macros.h"
#include "marshal.h"
#include "dispatch.h"
#include "marshal_generated.h"
+#include "hash.h"
struct marshal_cmd_Flush
{
struct marshal_cmd_base cmd_base;
};
void
_mesa_unmarshal_Flush(struct gl_context *ctx,
const struct marshal_cmd_Flush *cmd)
@@ -188,20 +189,59 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count,
_mesa_post_marshal_hook(ctx);
} else {
_mesa_glthread_finish(ctx);
CALL_ShaderSource(ctx->CurrentServerDispatch,
(shader, count, string, length_tmp));
}
free(length_tmp);
}
+static void track_buffers_destruction(struct gl_context *ctx,
+ GLsizei n, const GLuint * buffers)
+{
+ GLsizei i;
+ struct glthread_state *glthread = ctx->GLThread;
+
+ if (n < 0 || !buffers)
+ return;
+
+ for (i = 0; i < n ; i++) {
+ if (buffers[i] == glthread->bound_pixel_pack_buffer)
+ glthread->bound_pixel_pack_buffer = 0;
+
+ if (buffers[i] == glthread->bound_pixel_unpack_buffer)
+ glthread->bound_pixel_unpack_buffer = 0;
+ }
+}
+
+/* DeleteBuffers: custom marshal to track buffers destruction */
+void GLAPIENTRY
+_mesa_marshal_DeleteBuffers(GLsizei n, const GLuint * buffer)
+{
+ GET_CURRENT_CONTEXT(ctx);
+ _mesa_glthread_finish(ctx);
+ debug_print_sync("DeleteBuffers");
+
+ // It is done before CALL_DeleteBuffers to avoid any ABA multithread issue.
+ track_buffers_destruction(ctx, n, buffer);
+
+ CALL_DeleteBuffers(ctx->CurrentServerDispatch, (n, buffer));
+}
+
+void
+_mesa_unmarshal_DeleteBuffers(struct gl_context *ctx,
+ const struct marshal_cmd_DeleteBuffers *cmd)
+{
+ assert(0);
+}
+
/* BindBufferBase: marshalled asynchronously */
struct marshal_cmd_BindBufferBase
{
struct marshal_cmd_base cmd_base;
GLenum target;
GLuint index;
GLuint buffer;
};
/** Tracks the current bindings for the vertex array and index array buffers.
diff --git a/src/mesa/main/marshal.h b/src/mesa/main/marshal.h
index 63e0295576..e7f681213c 100644
--- a/src/mesa/main/marshal.h
+++ b/src/mesa/main/marshal.h
@@ -180,20 +180,2
[Mesa-dev] [PATCH v8 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue v7: rebase (update const handling) s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 24 -- src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 34 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 0c34b63854..c555727682 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0a74..6e1ac09ce0 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250777..447b03a41d 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single&qu
[Mesa-dev] [PATCH v8 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers
to be valid.
v4: update comments based on Nicolai review
v7:
s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/
Drop the ShadowBufferObjects hash. Synchronous creation/destruction
gives us enough guarantee to lookup the BufferObjects hash directly.
Signed-off-by: Gregory Hainaut
---
src/mesa/main/marshal.c | 36 +---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c
index 1914b5..285a46457d 100644
--- a/src/mesa/main/marshal.c
+++ b/src/mesa/main/marshal.c
@@ -237,21 +237,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx,
/* BindBufferBase: marshalled asynchronously */
struct marshal_cmd_BindBufferBase
{
struct marshal_cmd_base cmd_base;
GLenum target;
GLuint index;
GLuint buffer;
};
-/** Tracks the current bindings for the vertex array and index array buffers.
+/**
+ * Check that buffer is a valid buffer handle
+ * Always return false for ID 0.
+ */
+static bool
+is_bufferobj(struct gl_context *ctx, GLuint buffer)
+{
+ if (buffer == 0)
+ return false;
+ else
+ return _mesa_HashLookup(ctx->Shared->BufferObjects, buffer) != NULL;
+}
+
+/** Tracks the current bindings of GL buffer targets
*
* This is part of what we need to enable glthread on compat-GL contexts that
* happen to use VBOs, without also supporting the full tracking of VBO vs
* user vertex array bindings per attribute on each vertex array for
* determining what to upload at draw call time.
*
* Note that GL core makes it so that a buffer binding with an invalid handle
* in the "buffer" parameter will throw an error, and then a
* glVertexAttribPointer() that followsmight not end up pointing at a VBO.
* However, in GL core the draw call would throw an error as well, so we don't
@@ -259,37 +272,54 @@ struct marshal_cmd_BindBufferBase
* marshal user data for draw calls, and the unmarshal will just generate an
* error or not as appropriate.
*
* For compatibility GL, we do need to accurately know whether the draw call
* on the unmarshal side will dereference a user pointer or load data from a
* VBO per vertex. That would make it seem like we need to track whether a
* "buffer" is valid, so that we can know when an error will be generated
* instead of updating the binding. However, compat GL has the ridiculous
* feature that if you pass a bad name, it just gens a buffer object for you,
* so we escape without having to know if things are valid or not.
+ *
+ * Pixel buffers are tracked to decide whether pixel transfer goes to a user
+ * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike
+ * for VBOs, we do need accurate tracking, since user pointers can be used in
+ * GL core contexts.
*/
static void
-track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer)
+track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer)
{
struct glthread_state *glthread = ctx->GLThread;
switch (target) {
case GL_ARRAY_BUFFER:
glthread->vertex_array_is_vbo = (buffer != 0);
break;
case GL_ELEMENT_ARRAY_BUFFER:
/* The current element array buffer binding is actually tracked in the
* vertex array object instead of the context, so this would need to
* change on vertex array object updates.
*/
glthread->element_array_is_vbo = (buffer != 0);
break;
+ case GL_PIXEL_UNPACK_BUFFER:
+ if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer))
+ glthread->bound_pixel_unpack_buffer = buffer;
+ else
+ glthread->bound_pixel_unpack_buffer = 0;
+ break;
+ case GL_PIXEL_PACK_BUFFER:
+ if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer))
+ glthread->bound_pixel_pack_buffer = buffer;
+ else
+ glthread->bound_pixel_pack_buffer = 0;
+ break;
}
}
struct marshal_cmd_BindBuffer
{
struct marshal_cmd_base cmd_base;
GLenum target;
GLuint buffer;
};
@@ -307,21 +337,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx,
CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer));
}
void GLAPIENTRY
_mesa_marshal_BindBuffer(GLenum target, GLuint buffer)
{
GET_CURRENT_CONTEXT(ctx);
size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer);
struct marshal_cmd_BindBuffer *cmd;
debug_print_marshal("BindBuffer");
- track_vbo_binding(ctx, target, buffer);
+ track_buffers_binding(ctx, target, buffer);
if (cmd_size <= MARSHAL_MAX_CMD_SIZE) {
cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer,
cmd_size);
cmd->target = target;
cmd->buffer = buffer;
_mesa_post_marshal_hook(ctx);
} else {
_mesa_
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
Hello, I did more tests on my side. DRI3 + recent stack is fine. Older (Debian Jessie, ~2y old) XCB hangs/deadlock. So all DRI3 drivers should be fine (typically AMD). And all applications that called XInitThread soon enough. So yeah I think we can merge it. Note: I don't have commit access. Nouveau doesn't use DRI3 by default. So I'm looking to use XCB for dri2GetBuffer* operations. I copied the code from EGL/X11. So far it isn't a success. The dri2_drawable pdraw object is correctly populated with good value but I have tons of piglit failure. Debug is on-going but I'm afraid that it might not be possible to use XCB. Best Regads, Gregory On 4/25/17, Dieter Nützel wrote: > Am 21.04.2017 12:11, schrieb Marek Olšák: >> FWIW, I think this series can land, because glthread is not enabled by >> default, and the libX11 issue is unrelated. >> >> Marek > > > Gregory? > > For the series: > > Tested-by: Dieter Nützel > > On Turks XT (6670). > > Dieter > >> On Apr 21, 2017 4:22 AM, "Michel Dänzer" wrote: >> >>> On 21/04/17 09:01 AM, Marek Olšák wrote: >>>> On Thu, Apr 20, 2017 at 9:44 PM, gregory hainaut >>>> wrote: >>>>> On Thu, 20 Apr 2017 20:01:00 +0200 >>>>> Marek Olšák wrote: >>>>> >>>>>> On Thu, Apr 20, 2017 at 6:53 PM, gregory hainaut >>>>>> wrote: >>>>>>> On Thu, 20 Apr 2017 11:57:08 +0200 >>>>>>> Marek Olšák wrote: >>>>>>> >>>>>>>> On Thu, Apr 20, 2017 at 10:28 AM, gregory hainaut >>>>>>>> wrote: >>>>>>>>> On Thu, 20 Apr 2017 12:29:11 +0900 >>>>>>>>> Michel Dänzer wrote: >>>>>>>>> >>>>>>>>>> On 20/04/17 01:54 AM, Gregory Hainaut wrote: >>>>>>>>>>> Hello, >>>>>>>>>>> >>>>>>>>>>> Please find the latest version that include a small fix for >>> hash deletion. I >>>>>>>>>>> think the series is good now. Please review/ack it. >>>>>>>>>> >>>>>>>>>> I'm afraid I have to NACK it. As discussed in the v4 cover >>> letter >>>>>>>>>> thread, Mesa's glthread cannot make any libX11 API calls. >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> Hello Michel, >>>>>>>>> >>>>>>>>> Just to be sure we are on the same line, let's me do a >>> summary. >>>>>>>>> >>>>>>>>> PCSX2 does the following bad pattern >>>>>>>>> 1/ no call to XInitThread >>>>>>>>> 2/ XGetGeometry to get the size of the surface >>>>>>>>> 3/ glDrawArray into the default framebuffer 0 (the window, >>> I'm not sure how to call it) >>>>>>>>> => which seem to call DRI2GetBuffersWithFormat under the >>> hood. I guess to get the >>>>>>>>> associated buffer of the window. >>>>>>>>> >>>>>>>>> >>>>>>>>> So far it was (kind of) working fine because PCSX2 does tons >>> of PBO transfer. So glthread >>>>>>>>> was mostly synchronous. >>>>>>>>> >>>>>>>>> This series removes the (useless) PBO transfer >>> synchronization. So now glthread is really >>>>>>>>> asynchronous and the above bad pattern crash as expected. >>>>>>>>> >>>>>>>>> I didn't add any libX11 API call on the patches. And I don't >>> think we can remvove the DRI stuff. >>>>>>>>> >>>>>>>>> Hum, I don't know what will be the impact on the perf but >>> potentially we can force a synchronization >>>>>>>>> when there is a draw to framebuffer 0. >>>>>>>> >>>>>>>> Can you send us the backtrace when DRI2GetBuffersWithFormat is >>> called >>>>>>>> by glDrawArrays? >>>>>>>> >>>>>>>> Marek >>>>>>> >>>>>>> Hello Marek, Michel, >>>>>>> >>>>>>> Here the full backtra
[Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
Following the discussion in "[PATCH v4 0/3] asynchronous pbo transfer with glthread" It will help apps that are ported to XCB. But Xlib (without XInitThread) apps will still crash when glthread is enabled on DRI2. I only tested the patch on Nouveau There are 3 remaining possibilities * Won't fix. DRI3/XCB is the future * Enable thread safe Xlib by default (which would make sense in multicore CPU era) * Track gl call that will use X11 API to force a sync Note: I don't know if xcb_dri2_get_buffers_buffers can return a NULL so I added a check in doubt. Best Regards, Gregory Hainaut (1): glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB src/glx/dri2.c | 118 - src/glx/dri2.h | 25 src/glx/dri2_glx.c | 64 + 3 files changed, 47 insertions(+), 160 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/1] glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB
By default Xlib isn't thread safe so we better avoid it when gl thread
is enabled.
It will help applications that use XCB. But it will still crash if
applications are still relying on Xlib (without XInitThread).
Note: those dri2* functions are typically called by gallium/mesa state
tracker to handle new backbuffer allocation. When the old backbuffer was
previously invalidated due to vsync.
Signed-off-by: Gregory Hainaut
---
src/glx/dri2.c | 118 -
src/glx/dri2.h | 25
src/glx/dri2_glx.c | 64 +
3 files changed, 47 insertions(+), 160 deletions(-)
diff --git a/src/glx/dri2.c b/src/glx/dri2.c
index f00b96525a..eed899e237 100644
--- a/src/glx/dri2.c
+++ b/src/glx/dri2.c
@@ -391,138 +391,20 @@ DRI2DestroyDrawable(Display * dpy, XID drawable)
LockDisplay(dpy);
GetReq(DRI2DestroyDrawable, req);
req->reqType = info->codes->major_opcode;
req->dri2ReqType = X_DRI2DestroyDrawable;
req->drawable = drawable;
UnlockDisplay(dpy);
SyncHandle();
}
-DRI2Buffer *
-DRI2GetBuffers(Display * dpy, XID drawable,
- int *width, int *height,
- unsigned int *attachments, int count, int *outCount)
-{
- XExtDisplayInfo *info = DRI2FindDisplay(dpy);
- xDRI2GetBuffersReply rep;
- xDRI2GetBuffersReq *req;
- DRI2Buffer *buffers;
- xDRI2Buffer repBuffer;
- CARD32 *p;
- int i;
-
- XextCheckExtension(dpy, info, dri2ExtensionName, False);
-
- LockDisplay(dpy);
- GetReqExtra(DRI2GetBuffers, count * 4, req);
- req->reqType = info->codes->major_opcode;
- req->dri2ReqType = X_DRI2GetBuffers;
- req->drawable = drawable;
- req->count = count;
- p = (CARD32 *) & req[1];
- for (i = 0; i < count; i++)
- p[i] = attachments[i];
-
- if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) {
- UnlockDisplay(dpy);
- SyncHandle();
- return NULL;
- }
-
- *width = rep.width;
- *height = rep.height;
- *outCount = rep.count;
-
- buffers = malloc(rep.count * sizeof buffers[0]);
- if (buffers == NULL) {
- _XEatData(dpy, rep.count * sizeof repBuffer);
- UnlockDisplay(dpy);
- SyncHandle();
- return NULL;
- }
-
- for (i = 0; i < rep.count; i++) {
- _XReadPad(dpy, (char *) &repBuffer, sizeof repBuffer);
- buffers[i].attachment = repBuffer.attachment;
- buffers[i].name = repBuffer.name;
- buffers[i].pitch = repBuffer.pitch;
- buffers[i].cpp = repBuffer.cpp;
- buffers[i].flags = repBuffer.flags;
- }
-
- UnlockDisplay(dpy);
- SyncHandle();
-
- return buffers;
-}
-
-
-DRI2Buffer *
-DRI2GetBuffersWithFormat(Display * dpy, XID drawable,
- int *width, int *height,
- unsigned int *attachments, int count, int *outCount)
-{
- XExtDisplayInfo *info = DRI2FindDisplay(dpy);
- xDRI2GetBuffersReply rep;
- xDRI2GetBuffersReq *req;
- DRI2Buffer *buffers;
- xDRI2Buffer repBuffer;
- CARD32 *p;
- int i;
-
- XextCheckExtension(dpy, info, dri2ExtensionName, False);
-
- LockDisplay(dpy);
- GetReqExtra(DRI2GetBuffers, count * (4 * 2), req);
- req->reqType = info->codes->major_opcode;
- req->dri2ReqType = X_DRI2GetBuffersWithFormat;
- req->drawable = drawable;
- req->count = count;
- p = (CARD32 *) & req[1];
- for (i = 0; i < (count * 2); i++)
- p[i] = attachments[i];
-
- if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) {
- UnlockDisplay(dpy);
- SyncHandle();
- return NULL;
- }
-
- *width = rep.width;
- *height = rep.height;
- *outCount = rep.count;
-
- buffers = malloc(rep.count * sizeof buffers[0]);
- if (buffers == NULL) {
- _XEatData(dpy, rep.count * sizeof repBuffer);
- UnlockDisplay(dpy);
- SyncHandle();
- return NULL;
- }
-
- for (i = 0; i < rep.count; i++) {
- _XReadPad(dpy, (char *) &repBuffer, sizeof repBuffer);
- buffers[i].attachment = repBuffer.attachment;
- buffers[i].name = repBuffer.name;
- buffers[i].pitch = repBuffer.pitch;
- buffers[i].cpp = repBuffer.cpp;
- buffers[i].flags = repBuffer.flags;
- }
-
- UnlockDisplay(dpy);
- SyncHandle();
-
- return buffers;
-}
-
-
void
DRI2CopyRegion(Display * dpy, XID drawable, XserverRegion region,
CARD32 dest, CARD32 src)
{
XExtDisplayInfo *info = DRI2FindDisplay(dpy);
xDRI2CopyRegionReq *req;
xDRI2CopyRegionReply rep;
XextSimpleCheckExtension(dpy, info, dri2ExtensionName);
diff --git a/src/glx/dri2.h b/src/glx/dri2.h
index 4be5bf8eb8..c383e27123 100644
--- a/src/glx/dri2.h
+++ b/src/glx/dri2.h
@@ -30,29 +30,20 @@
* Kristian Høgsberg (k...@redhat.com)
*/
#ifndef _DRI2_H_
#define _DRI2_H_
#include
#include
#include
-typedef struct
-{
- unsigned int attachment;
- unsigned int name;
- unsigned int pitch;
- unsigned int cpp;
-
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
On Wed, 26 Apr 2017 11:03:11 +0900 Michel Dänzer wrote: > On 25/04/17 06:14 PM, Gregory Hainaut wrote: > > Hello, > > > > I did more tests on my side. DRI3 + recent stack is fine. Older > > (Debian Jessie, ~2y old) XCB hangs/deadlock. So all DRI3 drivers > > should be fine (typically AMD). And all applications that called > > XInitThread soon enough. So yeah I think we can merge it. Note: I > > don't have commit access. > > Okay, I'm not blocking somebody else from pushing the series. > > > > Nouveau doesn't use DRI3 by default. So I'm looking to use XCB for > > dri2GetBuffer* operations. I copied the code from EGL/X11. So far it > > isn't a success. The dri2_drawable pdraw object is correctly populated > > with good value but I have tons of piglit failure. Debug is on-going > > but I'm afraid that it might not be possible to use XCB. > > Can you share the patch you're testing? > > Hello Michel, I found my issue, I forgot to set the *width, *height pointer. Piglit status is good. You can found the patch here https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
On 4/26/17, Michel Dänzer wrote: > On 26/04/17 05:07 PM, Gregory Hainaut wrote: >> Following the discussion in "[PATCH v4 0/3] asynchronous pbo transfer with >> glthread" >> >> It will help apps that are ported to XCB. > > How so? I didn't test it (yet). But I think it is safe to call XCB from various threads. However if one thread use XLIB, you're screwed (to access the same display). > >> But Xlib (without XInitThread) apps will still crash when glthread is >> enabled on DRI2. > > Do the crashes provide information about where glthread is still calling > libX11 APIs? I far from an expert so maybe I misunderstand some stuffs. So, as far as I understand XLIB and XCB are mixed together. They likely share the same queues. Let's say you have an app that does Xlib operations on display (for example XGetGeometry). As XInitThread wasn't called, you're in YOLO mode. glthread (gallium->DRI2) will do operation (GetBuffer) on the same display through an XCB connection but it is still the same display. XCB might lock it properly but Xlib call is lock-free. What happen in my case is that XCB reply was corrupted (count was huge which lead to memory bad access). My guess was that Xlib calls from app were the culprit. I will test my XCB version of my app + this patch. Hopefully, it would be crash free. >> I only tested the patch on Nouveau >> >> There are 3 remaining possibilities >> * Won't fix. DRI3/XCB is the future >> * Enable thread safe Xlib by default (which would make sense in multicore >> CPU era) >> * Track gl call that will use X11 API to force a sync > > Until either of the latter two happens, glthread should only be enabled > with DRI3. On one hand, I don't like crash neither but in other hand, it would be nice to keep glthread for application that call XInitThread properly. At least, glthread isn't enabled by default. > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 1/1] glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB
On 4/26/17, Michel Dänzer wrote: > On 26/04/17 05:07 PM, Gregory Hainaut wrote: >> >> Note: those dri2* functions are typically called by gallium/mesa state >> tracker to handle new backbuffer allocation. When the old backbuffer was >> previously invalidated due to vsync. > > FWIW, DRI2 buffer invalidation isn't directly related to vsync. Ok. What happen is that I received DRI2_InvalidateBuffers event instead of DRI2_BufferSwapComplete when I enabled vsync on my application. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
On Thu, 27 Apr 2017 17:11:30 +0900 Michel Dänzer wrote: > On 26/04/17 07:06 PM, Gregory Hainaut wrote: > > On 4/26/17, Michel Dänzer wrote: > [...] > [...] > [...] > > > > I didn't test it (yet). But I think it is safe to call XCB from > > various threads. However if one thread use XLIB, you're screwed (to > > access the same display). > > > [...] > [...] > [...] > > I far from an expert so maybe I misunderstand some stuffs. So, as far > > as I understand XLIB and XCB are mixed together. They likely share the > > same queues. > > > > Let's say you have an app that does Xlib operations on display (for > > example XGetGeometry). As XInitThread wasn't called, you're in YOLO > > mode. > > > > glthread (gallium->DRI2) will do operation (GetBuffer) on the same > > display through an XCB connection but it is still the same display. > > XCB might lock it properly but Xlib call is lock-free. > > I was hoping the lower XCB layer could be used in a thread-safe way even > if the higher libX11 layer isn't. But maybe that's just wishful thinking. Hello Michel, Yeah me too. Besides libX11 relies on (the thread-safe) XCB too. > > What happen in my case is that XCB reply was corrupted (count was huge > > which lead to memory bad access). My guess was that Xlib calls from > > app were the culprit. > > It would be nice to get some certainty, e.g. via the valgrind > helgrind/drd tools. So I tried drd. Unfortunately I'm afraid that my testcase (PCSX2) is way too complex for this kind of tool. However, I always got a huge value on the reply->count in GetBuffer. I investigated it further. Reply->count was 94373760 which can be read as 1440,1920 so my surface resolution. I think it is enough to prove that xcb_dri2_get_buffers_with_format_reply is stealing the reply of XGetGeometry. Note my 2nd kind of crash is XIOError due to a null reply on XGetGeometry. which make sense based on the above behavior. FWIW, the crash seem to be gone with this patch + my PCSX2-XCB patches. However, I need to check DRI3 behavior when I resize the window. As you said it could trigger buffer invalidation too. > > [...] > [...] > > On one hand, I don't like crash neither but in other hand, it would be > > nice to keep glthread for application that call XInitThread properly. > > We could check dpy->lock_fns and only enable glthread with DRI2 if it's > non-NULL. If it's NULL and the environment variable mesa_glthread=true > is set, we could print a warning to stderr explaining that glthread > can't be enabled because the application didn't call XInitThreads. > It is good idea. I will check, if I can manage to implement such a check. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 0/2] Disable glthread is libX11 isn't thread-safe
Hello, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. EGL case is more tricky so the pair (X11/libX11) is marked as unsafe. I think it is fine because modern EGL application should rely on XCB (on the X11 platform). Best regards, Gregory Hainaut (2): glx|egl: allow to test if glthread is safe enough on X11 platform glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 9 + src/egl/drivers/dri2/egl_dri2.c | 30 src/gallium/state_trackers/dri/dri_context.c | 10 -- src/glx/dri2_glx.c | 9 + src/glx/dri3_glx.c | 8 5 files changed, 64 insertions(+), 2 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/2] glx|egl: allow to test if glthread is safe enough on X11 platform
I extended the struct __DRIbackgroundCallableExtensionRec because
the other function pointer is already related for glthread.
DRI2/DRI3 glx code path check that display can be locked (basically
XInitThread was called)
EGL code path is more tricky as we don't want to pull X11 header. Instead
the code will assume that it is safe if X11 isn't used or there is no display
(i.e. 100% XCB)
The new function will be used in the next commit
Signed-off-by: Gregory Hainaut
---
include/GL/internal/dri_interface.h | 9 +
src/egl/drivers/dri2/egl_dri2.c | 30 ++
src/glx/dri2_glx.c | 9 +
src/glx/dri3_glx.c | 8
4 files changed, 56 insertions(+)
diff --git a/include/GL/internal/dri_interface.h
b/include/GL/internal/dri_interface.h
index 86efd1bdc9..28a52ccdb9 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1713,13 +1713,22 @@ struct __DRIbackgroundCallableExtensionRec {
* non-background thread (i.e. a thread that has already been bound to a
* context using __DRIcoreExtensionRec::bindContext()); when this happens,
* the \c loaderPrivate pointer must be equal to the pointer that was
* passed to the driver when the currently bound context was created.
*
* This call should execute quickly enough that the driver can call it with
* impunity whenever a background thread starts performing drawing
* operations (e.g. it should just set a thread-local variable).
*/
void (*setBackgroundContext)(void *loaderPrivate);
+ /**
+* Indicate that it is multithread safe to use glthread. Typically
+* XInitThread was called in GLX setup.
+*
+* \param loaderPrivate is the value that was passed to to the driver when
+* the context was created. This can be used by the loader to identify
+* which context any callbacks are associated with.
+*/
+ GLboolean (*isGlThreadSafe)(void *loaderPrivate);
};
#endif
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 2cab7d00c1..df2db97bcf 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -85,24 +85,54 @@
static void
dri_set_background_context(void *loaderPrivate)
{
_EGLContext *ctx = _eglGetCurrentContext();
_EGLThreadInfo *t = _eglGetCurrentThread();
_eglBindContextToThread(ctx, t);
}
+static GLboolean
+dri_is_glthread_safe(void *loaderPrivate)
+{
+#ifdef HAVE_X11_PLATFORM
+ struct dri2_egl_surface *dri2_surf = loaderPrivate;
+ _EGLDisplay *display = dri2_surf->base.Resource.Display;
+ Display *dpy;
+
+ // Only the libX11 isn't safe
+ if (display->Platform != _EGL_PLATFORM_X11)
+ return true;
+
+ // Will use pure XCB so no libX11 here either
+ if (display->PlatformDisplay == NULL)
+ return true;
+
+ // In an ideal world we would check the X11 lock pointer
+ // (display->PlatformDisplay->lock_fns). Unfortunately it
+ // requires to know the full type. And we don't want to bring X11
+ // headers here.
+ //
+ // So let's assume an unsafe behavior. Modern EGL code shouldn't use
+ // libX11 anyway.
+ return false;
+#else
+ return true;
+#endif
+}
+
const __DRIbackgroundCallableExtension background_callable_extension = {
.base = { __DRI_BACKGROUND_CALLABLE, 1 },
.setBackgroundContext = dri_set_background_context,
+ .isGlThreadSafe = dri_is_glthread_safe,
};
const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
};
EGLint dri2_to_egl_attribute_map[] = {
0,
EGL_BUFFER_SIZE,/* __DRI_ATTRIB_BUFFER_SIZE */
EGL_LEVEL,/* __DRI_ATTRIB_LEVEL */
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..8f4d2f027f 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -946,20 +946,28 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw)
return priv->swap_interval;
}
static void
driSetBackgroundContext(void *loaderPrivate)
{
struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
__glXSetCurrentContext(&pcp->base);
}
+static GLboolean
+driIsGlThreadSafe(void *loaderPrivate)
+{
+ struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
+ return pcp->base.psc->dpy->lock_fns != NULL;
+}
+
+
static const __DRIdri2LoaderExtension dri2LoaderExtension = {
.base = { __DRI_DRI2_LOADER, 3 },
.getBuffers = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= dri2GetBuffersWithFormat,
};
static const __DRIdri2LoaderExtension dri2LoaderExtension_old = {
.base = { __DRI_DRI2_LOADER, 3 },
@@ -970,20 +978,21 @@ static const __DRIdri2LoaderExtension
dri2LoaderExtension_old = {
};
static const __DRIuseInvalidateExtension dri2UseInvalidate
[Mesa-dev] [RFC 2/2] glthread/gallium: require safe_glthread to start glthread
Otherwise print a warning
Signed-off-by: Gregory Hainaut
---
src/gallium/state_trackers/dri/dri_context.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/gallium/state_trackers/dri/dri_context.c
b/src/gallium/state_trackers/dri/dri_context.c
index 92d79849c4..35b0c454be 100644
--- a/src/gallium/state_trackers/dri/dri_context.c
+++ b/src/gallium/state_trackers/dri/dri_context.c
@@ -153,22 +153,28 @@ dri_create_context(gl_api api, const struct gl_config *
visual,
if (ctx->st->cso_context) {
ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled,
ctx->st->cso_context);
ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context);
}
/* Do this last. */
if (ctx->st->start_thread &&
/* the driver loader must implement this */
screen->sPriv->dri2.backgroundCallable &&
- driQueryOptionb(&screen->optionCache, "mesa_glthread"))
- ctx->st->start_thread(ctx->st);
+ driQueryOptionb(&screen->optionCache, "mesa_glthread")) {
+
+ if
(ctx->sPriv->dri2.backgroundCallable->isGlThreadSafe(cPriv->loaderPrivate))
+ ctx->st->start_thread(ctx->st);
+ else
+ fprintf(stderr, "MESA warning: glthread can't be enabled because "
+ "the application didn't call XInitThreads\n");
+ }
*error = __DRI_CTX_ERROR_SUCCESS;
return GL_TRUE;
fail:
if (ctx && ctx->st)
ctx->st->destroy(ctx->st);
free(ctx);
return GL_FALSE;
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
