Re: [Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-28 Thread Carl Worth 
Paul Berry stereotype...@gmail.com writes:
 Previous to this patch, the _mesa_{Begin,Resume}TransformFeedback
 functions were using ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX]

I see that this commit was nominated for the stable branch when it was
committed.

However, it appears to depend at least on commit b22146dc714 which is on
master, but is not on the 10.0 branch. So this patch cannot be applied
as-is to 10.0.

My current inclination is to simply drop this patch from the list of
candidates for 10.0. If you think differently, please recommend an
alternate plan.

-Carl


pgpgf7TFlcKKu.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-28 Thread Paul Berry 
On 28 January 2014 13:15, Carl Worth cwo...@cworth.org wrote:

 Paul Berry stereotype...@gmail.com writes:
  Previous to this patch, the _mesa_{Begin,Resume}TransformFeedback
  functions were using ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX]

 I see that this commit was nominated for the stable branch when it was
 committed.

 However, it appears to depend at least on commit b22146dc714 which is on


I think you mean 3b22146dc714 (mesa: Replace
ctx-Shader.Current{Vertex,Fragment,Geometry}Program with an array.).


 master, but is not on the 10.0 branch. So this patch cannot be applied
 as-is to 10.0.

 My current inclination is to simply drop this patch from the list of
 candidates for 10.0. If you think differently, please recommend an
 alternate plan.

 -Carl


(Also from IRC: cworth stereotype441: In my recent mail, I should have
included 43e77215b13 as well. (It's another patch I plan to drop from
stable unless you guide me differently.))

In principle we could make versions of these patches that don't depend on
commit 3b22146dc714, but I don't think it's worth it--the bugs I've fixed
in these patches are unlikely to arise until ARB_separate_shader_objects is
supported.

So yeah, I'm ok with dropping these two patches from the list of candidates
for 10.0.

I believe it was Ken who initially suggested marking these patches as
candidates for stable.  Cc'ing him in case he wants to express a dissenting
opinion.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-28 Thread Kenneth Graunke 
On 01/28/2014 01:26 PM, Paul Berry wrote:
 On 28 January 2014 13:15, Carl Worth cwo...@cworth.org
 mailto:cwo...@cworth.org wrote:
 
 Paul Berry stereotype...@gmail.com
 mailto:stereotype...@gmail.com writes:
  Previous to this patch, the _mesa_{Begin,Resume}TransformFeedback
  functions were using ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX]
 
 I see that this commit was nominated for the stable branch when it was
 committed.
 
 However, it appears to depend at least on commit b22146dc714 which is on
 
 
 I think you mean 3b22146dc714 (mesa: Replace
 ctx-Shader.Current{Vertex,Fragment,Geometry}Program with an array.).
  
 
 master, but is not on the 10.0 branch. So this patch cannot be applied
 as-is to 10.0.
 
 My current inclination is to simply drop this patch from the list of
 candidates for 10.0. If you think differently, please recommend an
 alternate plan.
 
 -Carl
 
 
 (Also from IRC: cworth stereotype441: In my recent mail, I should have
 included 43e77215b13 as well. (It's another patch I plan to drop from
 stable unless you guide me differently.))
 
 In principle we could make versions of these patches that don't depend
 on commit 3b22146dc714, but I don't think it's worth it--the bugs I've
 fixed in these patches are unlikely to arise until
 ARB_separate_shader_objects is supported.
 
 So yeah, I'm ok with dropping these two patches from the list of
 candidates for 10.0.
 
 I believe it was Ken who initially suggested marking these patches as
 candidates for stable.  Cc'ing him in case he wants to express a
 dissenting opinion.

Given that we discovered the only use of shader is a NULL-check to
determine if it's GLSL or ARB programs...I don't think it's nearly as
important as I thought.  I'd just drop it from stable.



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-22 Thread Paul Berry 
Previous to this patch, the _mesa_{Begin,Resume}TransformFeedback
functions were using ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX] to
find the program that would be the source of transform feedback data.
This isn't correct--if there's a geometry shader present it should be
ctx-Shader.CurrentProgram[MESA_SHADER_GEOMETRY].  (These might be
different if separate shader objects are in use).

This patch creates a function get_xfb_source(), which figures out the
correct program to use based on GL state, and updates
_mesa_{Begin,Resume}TransformFeedback to call it.  get_xfb_source() is
written in terms of the gl_shader_stage enum, so it should not need
modification when we add tessellation shaders in the future.  It also
creates a new driver flag, NewTransformFeedbackProg, which is flagged
whenever this program changes.

To reduce future confusion, this patch also rewords some comments and
error message text to avoid referring to vertex shaders.
---
 src/mesa/main/mtypes.h|  8 --
 src/mesa/main/transformfeedback.c | 52 +--
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 3dd9678..7fd3298 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -1815,8 +1815,9 @@ struct gl_transform_feedback_object
 
/**
 * The shader program active when BeginTransformFeedback() was called.
-* When active and unpaused, this equals
-* ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX].
+* When active and unpaused, this equals ctx-Shader.CurrentProgram[stage],
+* where stage is the pipeline stage that is the source of data for
+* transform feedback.
 */
struct gl_shader_program *shader_program;
 
@@ -3779,6 +3780,9 @@ struct gl_driver_flags
/** gl_context::TransformFeedback::CurrentObject */
GLbitfield NewTransformFeedback;
 
+   /** gl_context::TransformFeedback::CurrentObject::shader_program */
+   GLbitfield NewTransformFeedbackProg;
+
/** gl_context::RasterDiscard */
GLbitfield NewRasterizerDiscard;
 
diff --git a/src/mesa/main/transformfeedback.c 
b/src/mesa/main/transformfeedback.c
index 74897ba..9376a9e 100644
--- a/src/mesa/main/transformfeedback.c
+++ b/src/mesa/main/transformfeedback.c
@@ -24,7 +24,7 @@
 
 
 /*
- * Vertex transform feedback support.
+ * Transform feedback support.
  *
  * Authors:
  *   Brian Paul
@@ -376,25 +376,48 @@ _mesa_compute_max_transform_feedback_vertices(
  **/
 
 
+/**
+ * Figure out which stage of the pipeline is the source of transform feedback
+ * data given the current context state, and return its gl_shader_program.
+ *
+ * If no active program can generate transform feedback data (i.e. no vertex
+ * shader is active), returns NULL.
+ */
+static struct gl_shader_program *
+get_xfb_source(struct gl_context *ctx)
+{
+   int i;
+   for (i = MESA_SHADER_FRAGMENT - 1; i = MESA_SHADER_VERTEX; i--) {
+  if (ctx-Shader.CurrentProgram[i] != NULL)
+ return ctx-Shader.CurrentProgram[i];
+   }
+   return NULL;
+}
+
+
 void GLAPIENTRY
 _mesa_BeginTransformFeedback(GLenum mode)
 {
struct gl_transform_feedback_object *obj;
-   struct gl_transform_feedback_info *info;
+   struct gl_transform_feedback_info *info = NULL;
+   struct gl_shader_program *source;
GLuint i;
unsigned vertices_per_prim;
GET_CURRENT_CONTEXT(ctx);
 
obj = ctx-TransformFeedback.CurrentObject;
 
-   if (ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX] == NULL) {
+   /* Figure out what pipeline stage is the source of data for transform
+* feedback.
+*/
+   source = get_xfb_source(ctx);
+   if (source == NULL) {
   _mesa_error(ctx, GL_INVALID_OPERATION,
   glBeginTransformFeedback(no program active));
   return;
}
 
-   info =
-  ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX]-LinkedTransformFeedback;
+   info = source-LinkedTransformFeedback;
 
if (info-NumOutputs == 0) {
   _mesa_error(ctx, GL_INVALID_OPERATION,
@@ -452,7 +475,10 @@ _mesa_BeginTransformFeedback(GLenum mode)
   obj-GlesRemainingPrims = max_vertices / vertices_per_prim;
}
 
-   obj-shader_program = ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX];
+   if (obj-shader_program != source) {
+  ctx-NewDriverState |= ctx-DriverFlags.NewTransformFeedbackProg;
+  obj-shader_program = source;
+   }
 
assert(ctx-Driver.BeginTransformFeedback);
ctx-Driver.BeginTransformFeedback(ctx, mode, obj);
@@ -519,7 +545,7 @@ bind_buffer_range(struct gl_context *ctx, GLuint index,
 
 
 /**
- * Specify a buffer object to receive vertex shader results.  Plus,
+ * Specify a buffer object to receive transform feedback results.  Plus,
  * specify the starting offset to place the results, and max size.
  * Called from the glBindBufferRange() function.
  */
@@ -563,7 +589,7 @@ _mesa_bind_buffer_range_transform_feedback(struct 
gl_context *ctx,
 
 
 /**
- * Specify a buffer object to receive vertex shader results.
+ * Specify a

Re: [Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-22 Thread Kenneth Graunke 
On 01/22/2014 06:07 AM, Paul Berry wrote:
 Previous to this patch, the _mesa_{Begin,Resume}TransformFeedback
 functions were using ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX] to
 find the program that would be the source of transform feedback data.
 This isn't correct--if there's a geometry shader present it should be
 ctx-Shader.CurrentProgram[MESA_SHADER_GEOMETRY].  (These might be
 different if separate shader objects are in use).
 
 This patch creates a function get_xfb_source(), which figures out the
 correct program to use based on GL state, and updates
 _mesa_{Begin,Resume}TransformFeedback to call it.  get_xfb_source() is
 written in terms of the gl_shader_stage enum, so it should not need
 modification when we add tessellation shaders in the future.  It also
 creates a new driver flag, NewTransformFeedbackProg, which is flagged
 whenever this program changes.
 
 To reduce future confusion, this patch also rewords some comments and
 error message text to avoid referring to vertex shaders.
 ---
  src/mesa/main/mtypes.h|  8 --
  src/mesa/main/transformfeedback.c | 52 
 +--
  2 files changed, 45 insertions(+), 15 deletions(-)
 
 diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
 index 3dd9678..7fd3298 100644
 --- a/src/mesa/main/mtypes.h
 +++ b/src/mesa/main/mtypes.h
 @@ -1815,8 +1815,9 @@ struct gl_transform_feedback_object
  
 /**
  * The shader program active when BeginTransformFeedback() was called.
 -* When active and unpaused, this equals
 -* ctx-Shader.CurrentProgram[MESA_SHADER_VERTEX].
 +* When active and unpaused, this equals 
 ctx-Shader.CurrentProgram[stage],
 +* where stage is the pipeline stage that is the source of data for
 +* transform feedback.
  */
 struct gl_shader_program *shader_program;
  
 @@ -3779,6 +3780,9 @@ struct gl_driver_flags
 /** gl_context::TransformFeedback::CurrentObject */
 GLbitfield NewTransformFeedback;
  
 +   /** gl_context::TransformFeedback::CurrentObject::shader_program */
 +   GLbitfield NewTransformFeedbackProg;
 +
 /** gl_context::RasterDiscard */
 GLbitfield NewRasterizerDiscard;
  
 diff --git a/src/mesa/main/transformfeedback.c 
 b/src/mesa/main/transformfeedback.c
 index 74897ba..9376a9e 100644
 --- a/src/mesa/main/transformfeedback.c
 +++ b/src/mesa/main/transformfeedback.c
 @@ -24,7 +24,7 @@
  
  
  /*
 - * Vertex transform feedback support.
 + * Transform feedback support.
   *
   * Authors:
   *   Brian Paul
 @@ -376,25 +376,48 @@ _mesa_compute_max_transform_feedback_vertices(
   **/
  
  
 +/**
 + * Figure out which stage of the pipeline is the source of transform feedback
 + * data given the current context state, and return its gl_shader_program.
 + *
 + * If no active program can generate transform feedback data (i.e. no vertex
 + * shader is active), returns NULL.
 + */
 +static struct gl_shader_program *
 +get_xfb_source(struct gl_context *ctx)
 +{
 +   int i;
 +   for (i = MESA_SHADER_FRAGMENT - 1; i = MESA_SHADER_VERTEX; i--) {

I think this would be clearer as:

for (i = MESA_SHADER_GEOMETRY; i = MESA_SHADER_VERTEX; i--) {
   ...
}

Note that the pipeline ordering is:
Vertex - Tess. Control - Tess. Eval - Geometry - Transform Feedback
(http://www.opengl.org/wiki/Rendering_Pipeline_Overview)

So either implementation would work even with tessellation shaders.

Either way, this series is:
Reviewed-by: Kenneth Graunke kenn...@whitecape.org
Cc: 10.0 mesa-sta...@lists.freedesktop.org



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: Ensure that transform feedback refers to the correct program.

2014-01-22 Thread Paul Berry 
On 22 January 2014 08:20, Kenneth Graunke kenn...@whitecape.org wrote:

 On 01/22/2014 06:07 AM, Paul Berry wrote:
  @@ -376,25 +376,48 @@ _mesa_compute_max_transform_feedback_vertices(
**/
 
 
  +/**
  + * Figure out which stage of the pipeline is the source of transform
 feedback
  + * data given the current context state, and return its
 gl_shader_program.
  + *
  + * If no active program can generate transform feedback data (i.e. no
 vertex
  + * shader is active), returns NULL.
  + */
  +static struct gl_shader_program *
  +get_xfb_source(struct gl_context *ctx)
  +{
  +   int i;
  +   for (i = MESA_SHADER_FRAGMENT - 1; i = MESA_SHADER_VERTEX; i--) {

 I think this would be clearer as:

 for (i = MESA_SHADER_GEOMETRY; i = MESA_SHADER_VERTEX; i--) {
...
 }

 Note that the pipeline ordering is:
 Vertex - Tess. Control - Tess. Eval - Geometry - Transform Feedback
 (http://www.opengl.org/wiki/Rendering_Pipeline_Overview)

 So either implementation would work even with tessellation shaders.

 Either way, this series is:
 Reviewed-by: Kenneth Graunke kenn...@whitecape.org
 Cc: 10.0 mesa-sta...@lists.freedesktop.org


That's a good point--I like your suggestion.  Thanks for the review!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev