Nouveau - Jan 2015 - Re: [RFC] mesa/st: Avoid passing a NULL buffer to the drivers

If we capture transform feedback from n stream in (n-1) buffers we face a
NULL buffer, use the buffer (n-1) to capture the output of stream n.

This fixes one piglit test with nvc0:
   arb_gpu_shader5-xfb-streams-without-invocations

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at mni.thm.de>
---
 src/mesa/state_tracker/st_cb_xformfb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/mesa/state_tracker/st_cb_xformfb.c
b/src/mesa/state_tracker/st_cb_xformfb.c
index 8f75eda..5a12da4 100644
--- a/src/mesa/state_tracker/st_cb_xformfb.c
+++ b/src/mesa/state_tracker/st_cb_xformfb.c
@@ -123,6 +123,11 @@ st_begin_transform_feedback(struct gl_context *ctx, GLenum
mode,
       struct st_buffer_object *bo = st_buffer_object(sobj->base.Buffers[i]);
 
       if (bo) {
+         if (!bo->buffer)
+            /* If we capture transform feedback from n streams into (n-1)
+             * buffers we have to write to buffer (n-1) for stream n.
+             */
+            bo = st_buffer_object(sobj->base.Buffers[i-1]);
          /* Check whether we need to recreate the target. */
          if (!sobj->targets[i] ||
              sobj->targets[i] == sobj->draw_count ||
-- 
2.2.1

Can you elaborate a bit as to why that's the right thing to do?

On Wed, Jan 7, 2015 at 1:52 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:
> If we capture transform feedback from n stream in (n-1) buffers we face a
> NULL buffer, use the buffer (n-1) to capture the output of stream n.
>
> This fixes one piglit test with nvc0:
>    arb_gpu_shader5-xfb-streams-without-invocations
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
> ---
>  src/mesa/state_tracker/st_cb_xformfb.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/src/mesa/state_tracker/st_cb_xformfb.c
b/src/mesa/state_tracker/st_cb_xformfb.c
> index 8f75eda..5a12da4 100644
> --- a/src/mesa/state_tracker/st_cb_xformfb.c
> +++ b/src/mesa/state_tracker/st_cb_xformfb.c
> @@ -123,6 +123,11 @@ st_begin_transform_feedback(struct gl_context *ctx,
GLenum mode,
>        struct st_buffer_object *bo =
st_buffer_object(sobj->base.Buffers[i]);
>
>        if (bo) {
> +         if (!bo->buffer)
> +            /* If we capture transform feedback from n streams into (n-1)
> +             * buffers we have to write to buffer (n-1) for stream n.
> +             */
> +            bo = st_buffer_object(sobj->base.Buffers[i-1]);
>           /* Check whether we need to recreate the target. */
>           if (!sobj->targets[i] ||
>               sobj->targets[i] == sobj->draw_count ||
> --
> 2.2.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

On 11.01.2015 06:05, Ilia Mirkin wrote:
> Can you elaborate a bit as to why that's the right thing to do?
>
> On Wed, Jan 7, 2015 at 1:52 PM, Tobias Klausmann
> <tobias.johannes.klausmann at mni.thm.de> wrote:
>> If we capture transform feedback from n stream in (n-1) buffers we face
a
>> NULL buffer, use the buffer (n-1) to capture the output of stream n.
>>
>> This fixes one piglit test with nvc0:
>>     arb_gpu_shader5-xfb-streams-without-invocations
>>
>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
>> ---
>>   src/mesa/state_tracker/st_cb_xformfb.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/src/mesa/state_tracker/st_cb_xformfb.c
b/src/mesa/state_tracker/st_cb_xformfb.c
>> index 8f75eda..5a12da4 100644
>> --- a/src/mesa/state_tracker/st_cb_xformfb.c
>> +++ b/src/mesa/state_tracker/st_cb_xformfb.c
>> @@ -123,6 +123,11 @@ st_begin_transform_feedback(struct gl_context
*ctx, GLenum mode,
>>         struct st_buffer_object *bo =
st_buffer_object(sobj->base.Buffers[i]);
>>
>>         if (bo) {
>> +         if (!bo->buffer)
>> +            /* If we capture transform feedback from n streams into
(n-1)
>> +             * buffers we have to write to buffer (n-1) for stream n.
>> +             */
>> +            bo = st_buffer_object(sobj->base.Buffers[i-1]);
>>            /* Check whether we need to recreate the target. */
>>            if (!sobj->targets[i] ||
>>                sobj->targets[i] == sobj->draw_count ||
>> --
>> 2.2.1
Quoted from Ilia Mirkin, to specify what shall be elaborated:
"Can you explain (on-list) why using buffer n - 1 is the right thing to
do to capture output of stream n? I would have thought that the output
for that stream should be discarded or something.

Like with a spec quotation or some other justification. i.e. why is
the code you wrote correct? Why is it better than, say, bo buffers[0], or some
other thing entirely?"

Yeah thats the most concerning point i see as well. The problem is that 
there is a interaction between arb_gpu_shader5 and 
arb_transform_feedback3, but after a bit of reading i think the patch is 
actually what we should do:

 From the arb_transfrom_feedback3 spec:
"
(3) How might you use transform feedback with geometry shaders and
         multiple vertex streams?

       RESOLVED:  As a simple example, let's say you are processing 
triangles
       and capture both processed triangle vertices and some values that are
       computed per-primitive (e.g., facet normal).  The geometry shader
       might declare its outputs like the following:

         layout(stream = 0) out vec4 position;
         layout(stream = 0) out vec4 texcoord;
         layout(stream = 1) out vec4 normal;

       "position" and "texcoord" would be per-vertex
attributes written to
       vertex stream 0; "normal" would be a per-triangle facet normal.
The
       geometry shader would emit three vertices to stream zero (the 
processed
       input vertices) and a single vertex to stream one (the per-triangle
       data).  The transform feedback API usage for this case would be
       something like:

         // Set up buffer objects 21 and 22 to capture data for 
per-vertex and
         // per primitive values.
         glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, 21);
         glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 1, 22);

         // Set up XFB to capture position and texcoord to buffer binding
         // point 0 (buffer 21 bound), and normal to binding point 1 
(buffer
         // 22 bound).
         char *strings[] = { "position", "texcoord",
"gl_NextBuffer",
                             "normal" };
"

-> Especially the comments are enlightening as to where the outputs 
should go. Thats what happens with the 
"arb_gpu_shader5-xfb-streams-without-invocations" test, where two 
stream(outputs) are captured into one buffer.

One might argue now if we have to count .Buffers[i-1] for all buffers 
after this...

Comments and additional feedback is always appreciated!

Greetings,
Tobias

NAK.

NULL buffers are allowed. All transform feedback writes to NULL
buffers should be discarded.

Marek

On Wed, Jan 7, 2015 at 7:52 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:
> If we capture transform feedback from n stream in (n-1) buffers we face a
> NULL buffer, use the buffer (n-1) to capture the output of stream n.
>
> This fixes one piglit test with nvc0:
>    arb_gpu_shader5-xfb-streams-without-invocations
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
> ---
>  src/mesa/state_tracker/st_cb_xformfb.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/src/mesa/state_tracker/st_cb_xformfb.c
b/src/mesa/state_tracker/st_cb_xformfb.c
> index 8f75eda..5a12da4 100644
> --- a/src/mesa/state_tracker/st_cb_xformfb.c
> +++ b/src/mesa/state_tracker/st_cb_xformfb.c
> @@ -123,6 +123,11 @@ st_begin_transform_feedback(struct gl_context *ctx,
GLenum mode,
>        struct st_buffer_object *bo =
st_buffer_object(sobj->base.Buffers[i]);
>
>        if (bo) {
> +         if (!bo->buffer)
> +            /* If we capture transform feedback from n streams into (n-1)
> +             * buffers we have to write to buffer (n-1) for stream n.
> +             */
> +            bo = st_buffer_object(sobj->base.Buffers[i-1]);
>           /* Check whether we need to recreate the target. */
>           if (!sobj->targets[i] ||
>               sobj->targets[i] == sobj->draw_count ||
> --
> 2.2.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev