All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
@ 2015-01-07 18:52 Tobias Klausmann
  2015-01-11  5:05 ` [Nouveau] " Ilia Mirkin
       [not found] ` <1420656766-7342-1-git-send-email-tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
  0 siblings, 2 replies; 7+ messages in thread
From: Tobias Klausmann @ 2015-01-07 18:52 UTC (permalink / raw)
  To: mesa-dev-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

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@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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Nouveau] [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
  2015-01-07 18:52 [RFC] mesa/st: Avoid passing a NULL buffer to the drivers Tobias Klausmann
@ 2015-01-11  5:05 ` Ilia Mirkin
  2015-01-12  0:43   ` Tobias Klausmann
       [not found] ` <1420656766-7342-1-git-send-email-tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
  1 sibling, 1 reply; 7+ messages in thread
From: Ilia Mirkin @ 2015-01-11  5:05 UTC (permalink / raw)
  To: Tobias Klausmann
  Cc: mesa-dev@lists.freedesktop.org, nouveau@lists.freedesktop.org

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@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Nouveau] [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
  2015-01-11  5:05 ` [Nouveau] " Ilia Mirkin
@ 2015-01-12  0:43   ` Tobias Klausmann
  2015-01-12  0:57     ` Ilia Mirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Tobias Klausmann @ 2015-01-12  0:43 UTC (permalink / raw)
  To: Ilia Mirkin; +Cc: mesa-dev@lists.freedesktop.org, nouveau@lists.freedesktop.org



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@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@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

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Nouveau] [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
  2015-01-12  0:43   ` Tobias Klausmann
@ 2015-01-12  0:57     ` Ilia Mirkin
  2015-01-12  1:11       ` Tobias Klausmann
  0 siblings, 1 reply; 7+ messages in thread
From: Ilia Mirkin @ 2015-01-12  0:57 UTC (permalink / raw)
  To: Tobias Klausmann
  Cc: mesa-dev@lists.freedesktop.org, nouveau@lists.freedesktop.org

On Sun, Jan 11, 2015 at 7:43 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
>
>
> 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@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@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!

The thing you're quoting is talking about the case where everything's
supposed to work. I haven't investigated, but I'm guessing that the
test has a layout(stream=1) but no buffer is bound at index 1. Is that
right? In that case, I would imagine that the TF output should
actually just get dropped on the floor. I would assume that this is in
the ARB_tf3 spec, but I don't have time to go digging right now.

  -ilia
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Nouveau] [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
  2015-01-12  0:57     ` Ilia Mirkin
@ 2015-01-12  1:11       ` Tobias Klausmann
       [not found]         ` <54B31F25.3070104-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Tobias Klausmann @ 2015-01-12  1:11 UTC (permalink / raw)
  To: Ilia Mirkin; +Cc: mesa-dev@lists.freedesktop.org, nouveau@lists.freedesktop.org



On 12.01.2015 01:57, Ilia Mirkin wrote:
> On Sun, Jan 11, 2015 at 7:43 PM, Tobias Klausmann
> <tobias.johannes.klausmann@mni.thm.de> wrote:
>>
>> 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@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@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!
> The thing you're quoting is talking about the case where everything's
> supposed to work. I haven't investigated, but I'm guessing that the
> test has a layout(stream=1) but no buffer is bound at index 1.

Actually no, the layout reads like this:
     layout(stream = 0) out float stream0_0_out;
     layout(stream = 1) out vec2 stream1_0_out;
     layout(stream = 2) out float stream2_0_out;
     layout(stream = 2) out vec4 stream2_1_out;
where no buffer is bound to stream2_1_out.

>   Is that right? In that case, I would imagine that the TF output should
> actually just get dropped on the floor. I would assume that this is in
> the ARB_tf3 spec, but I don't have time to go digging right now.

As the layout read more like the one from the specs, i'd still go with 
[n-1] and honestly (not proven correct) i assume that a stream without 
an output is just wrong and should not even get through the linker...

Tobias
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
       [not found]         ` <54B31F25.3070104-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
@ 2015-01-12  1:17           ` Ilia Mirkin
  0 siblings, 0 replies; 7+ messages in thread
From: Ilia Mirkin @ 2015-01-12  1:17 UTC (permalink / raw)
  To: Tobias Klausmann
  Cc: mesa-dev-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org

On Sun, Jan 11, 2015 at 8:11 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
>
>
> On 12.01.2015 01:57, Ilia Mirkin wrote:
>>
>> On Sun, Jan 11, 2015 at 7:43 PM, Tobias Klausmann
>> <tobias.johannes.klausmann@mni.thm.de> wrote:
>>>
>>>
>>> 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@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@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!
>>
>> The thing you're quoting is talking about the case where everything's
>> supposed to work. I haven't investigated, but I'm guessing that the
>> test has a layout(stream=1) but no buffer is bound at index 1.
>
>
> Actually no, the layout reads like this:
>     layout(stream = 0) out float stream0_0_out;
>     layout(stream = 1) out vec2 stream1_0_out;
>     layout(stream = 2) out float stream2_0_out;
>     layout(stream = 2) out vec4 stream2_1_out;
> where no buffer is bound to stream2_1_out.
>
>>   Is that right? In that case, I would imagine that the TF output should
>> actually just get dropped on the floor. I would assume that this is in
>> the ARB_tf3 spec, but I don't have time to go digging right now.
>
>
> As the layout read more like the one from the specs, i'd still go with [n-1]
> and honestly (not proven correct) i assume that a stream without an output
> is just wrong and should not even get through the linker...

Well, linking has nothing to do with current bindings; it has to do
with connecting inter-stage inputs/outputs... Anyways, I'm moderately
sure that the thing you're currently doing has no justification. Try
to find what the proper way of dealing with this is (I assume i965
deals with it, see what they do), or ask around. If only no buffer is
bound to stream2_1_out, I assume that it's doing the "separate"
outputs? That probably complicates matters...

  -ilia
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Mesa-dev] [RFC] mesa/st: Avoid passing a NULL buffer to the drivers
       [not found] ` <1420656766-7342-1-git-send-email-tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
@ 2015-01-12 12:14   ` Marek Olšák
  0 siblings, 0 replies; 7+ messages in thread
From: Marek Olšák @ 2015-01-12 12:14 UTC (permalink / raw)
  To: Tobias Klausmann
  Cc: mesa-dev-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	nouveau

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@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-01-12 12:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-07 18:52 [RFC] mesa/st: Avoid passing a NULL buffer to the drivers Tobias Klausmann
2015-01-11  5:05 ` [Nouveau] " Ilia Mirkin
2015-01-12  0:43   ` Tobias Klausmann
2015-01-12  0:57     ` Ilia Mirkin
2015-01-12  1:11       ` Tobias Klausmann
     [not found]         ` <54B31F25.3070104-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
2015-01-12  1:17           ` Ilia Mirkin
     [not found] ` <1420656766-7342-1-git-send-email-tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
2015-01-12 12:14   ` [Mesa-dev] " Marek Olšák

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.