[Mesa-dev] [PATCH RFC] gallium: interface changes necessary to implement transform feedback

[Marek Olšák]

I am cc'ing Zack, because he was the one to design the first interface.

See also my inline comments below.

On Thu, Oct 6, 2011 at 4:50 PM, Brian Paul <brianp at vmware.com> wrote:
[snip]
> Thanks for digging into this.  I was working on xformfb a while back (and
> still have some patches sitting around) but was far from finishing it.  If
> you have any interest in those patches I could send them to you.

I have been working on it quite a while too and I have some more
patches than this. In my local non-public branch, I have the st/mesa
code nearly ready for EXT_transform_feedback and
ARB_transform_feedback2. I have partially done the GLSL linker part in
glsl_to_tgsi, including querying locations of varyings from the GLSL
IR, but that is not complete yet and maybe I'll move that into
src/glsl, because it doesn't depend on Gallium at all. Intel folks are
also digging into this. I have also done some Radeon support to the
extent that some piglit tests pass. You can send me your patches if
you like, although it's just the linker which needs more work here and
I haven't had a look at the queries yet.

[snip]
>>  struct lp_so_state {
>> -   struct pipe_stream_output_state base;
>> +   struct pipe_stream_output_info base;
>>  };
>
> Maybe call this lp_so_info to avoid mixing "state" and "info"?

OK. This change is just to prevent compile failures. I chose "info" to
reflect it's shader info and not a state. There is also some
stream-out code in the software drivers that I had to comment out
before this commit. I'd like to do hardware support first and make
sure that r600g doesn't have to jump through hoops. Software drivers
can do pretty much anything, so they can wait.

[snip]
>>  struct sp_so_state {
>> -   struct pipe_stream_output_state base;
>> +   struct pipe_stream_output_info base;
>>  };
>
> Again, maybe call this sp_so_info.

OK.

[snip]
>> +   struct pipe_stream_output_target *(*create_stream_output_target)(
>> +                        struct pipe_context *,
>> +                        struct pipe_resource *,
>> +                        unsigned buffer_offset,
>> +                        unsigned buffer_size);
>> +
>> +   void (*stream_output_target_destroy)(struct pipe_context *,
>> +                                        struct pipe_stream_output_target
>> *);
>
>
> The naming of pipe_context methods has become a bit mixed up.
>
> Initially, we were using "create_xxx()" and "delete_xxx()" but now we've got
> "destroy_xxx()" and "xxx_destroy()" too.  You're being consistent with some
> of the newer methods but someday was should probably clean up our naming
> conventions.

I thought the delete_* hooks are for CSOs and the *_destroy hooks are
for reference-counted objects. Anyway, as far as the cleanups go, I am
more concerned about the queries. There is PIPE_QUERY_GPU_FINISHED,
which looks equivalent to fences. Besides that,
PIPE_QUERY_PRIMITIVES_EMITTED is a strict subset of
PIPE_QUERY_SO_STATISTICS, which returns a structure instead of one
number.

[snip]
>> +struct pipe_stream_output_target
>> +{
>> +   struct pipe_reference reference;
>> +   struct pipe_resource *buffer; /**<  buffer into which this is a target
>> view */
>> +   struct pipe_context *context; /**<  context this view belongs to */
>> +
>> +   unsigned buffer_offset;  /**<  offset where data should be written, in
>> bytes */
>> +   unsigned buffer_size;    /**<  how much data is allowed to be written
>> */
>
> When would the buffer_size be less than the buffer/resource's size?

It's for BindBufferRange, which specifies the range where transform
feedback is allowed to write data. The other parts of the buffer can
be used for something entirely different (e.g. VBO, TBO, ...)
simultaneously.

>
>
>> +};
>> +
>> +
>> +/**
>>   * Information to describe a vertex attribute (position, color, etc)
>>   */
>>  struct pipe_vertex_element
>> @@ -481,6 +512,22 @@ struct pipe_draw_info
>>      */
>>     boolean primitive_restart;
>>     unsigned restart_index;
>> +
>> +   /**
>> +    * Stream output target. If not NULL, it's used to provide the 'count'
>> +    * parameter based on the number vertices captured by the stream
>> output
>> +    * stage. (or generally, based on the number of bytes captured)
>> +    *
>> +    * Only 'mode', 'start_instance', and 'instance_count' are taken into
>> +    * account, all the other variables from pipe_draw_info are ignored.
>> +    *
>> +    * 'start' is implicitly 0 and 'count' is set as discussed above.
>> +    * The draw command is non-indexed.
>> +    *
>> +    * Note that this only provides the count. The vertex buffers must
>> +    * be set via set_vertex_buffers manually.
>> +    */
>> +   struct pipe_stream_output_target *count_from_stream_output;
>>  };
>
> I'd have to do some digging, but I seem to recall that pipe_vertex_element
> might be used as a hash key somewhere.  I'm wondering if adding a pointer to
> this struct could upset the hashing (artificially increase the number of
> hash keys/entries).

That was actually pipe_draw_info.

Marek