[Mesa-dev] Unified pipe_context::draw_vbo thoughts

[Keith Whitwell]

On Mon, 2010-07-19 at 00:46 -0700, Chia-I Wu wrote:
> [change the subject because of typo and it is not about the patch]
> 
> On Sun, Jul 18, 2010 at 7:31 PM, Keith Whitwell <keithw at vmware.com> wrote:
> > If we're reorganising these interfaces, there are a couple of things I'd
> > like to see done differently.  In particular, within your draw_info
> > struct there are a couple of entities which are often better handled as
> > persistent state - namely the primitive mode and index buffer binding.
> >
> > Right now we've got a single call which sets as much state as
> > glMultiDrawElementsIndexed, but which can only draw a single primitive.
> >
> > The ways out of this situation are to either move state out of
> > per-primitive transaction, or to modify the interface to accept >1
> > primitive at a time.
> >
> > My preference would be to move primitive mode & index buffer (and other
> > stuff too?) out of your info struct and turn them into new dynamic state
> > calls.
> > Having done that, it may still be worth considering passing >1 of your
> > info structs to the new unified entrypoints...
> I considered moving the index buffer state out, but I am not sure how to
> formulate int.  Consider having a method that supports glMultiDrawElements.  If
> the indices pointers point to different buffers in client memory, we need
> multiple index buffers.  

I think the meaning of those pointers when an element array object is
bound is just different offsets into that buffer, as below.

> Even if all indices pointers are offsets to the
> currently bound index buffer, which may not be aligned to the index size, it
> seems to me that multiple index buffers are still desired.

I don't really see why that's the case -- GL itself is happy having just
a single element array binding point - we're just discussing promoting
the same concept into gallium.

So basically I think you're worrying about something which isn't an
actual requirement.

> As i965 does not seem to support multiple index buffers, I am favoring single
> index buffer, and skiping direct support for glMultiDrawElements in
> pipe_context at the moment.  Then, the index buffer state would look like
> 
>   struct pipe_index_buffer
>   {
>      unsigned index_size;    /**< size of an index, in bytes */
>      unsigned offset;        /**< offset to start of data in buffer */
>      struct pipe_resource *buffer; /**< the actual buffer */
>   };

This looks good.

> pipe_context::draw_vbo is unchanged, but pipe_draw_info is changed to
> 
>   struct pipe_draw_info
>   {
>      unsigned mode;
>      unsigned start;
>      unsigned count;
> 
>      unsigned first_instance;
>      unsigned instance_count;
> 
>      int index_bias;
>      unsigned min_index;
>      unsigned max_index;
>   };
> 
> The last 3 fields are redundant for non-indexed drawing, but they still make
> sense (min_index == start, max_index == start + count - 1).  Having the fields
> in the struct and having a single draw_vbo allow us to support indexed and
> non-indexed drawing in a single loop in st_draw_vbo.
> 
> The struct can be extended to include "index buffer index" if multiple index
> buffers are desired.  The purpose of using a struct here as the argument is
> that it is easier to be passed around inside a pipe driver and is easier to
> extend.
> 
> I keep mode in the struct so that if in the future, the imaginary
> glMultiModeMultiDrawElementsInstancedBaseVertex is introduced, we can support
> it by changing draw_vbo to take an array of pipe_draw_info.  Hardware (i965)
> also seems to send the primitive type along with the draw command.

It does, but the driver also sets up a bunch of hardware state in a
primitive dependent fashion.

I don't really feel strongly about primitive in/out of the struct, but
it does seem to be something which ends up being treated as state inside
most drivers - ie changes to primitive or reduced_primitive end up
triggering an update_state(), while usually the rest of the data there
does not.

> This is my current thoughts.  Suggestions appreciated.

I like this a lot.  I don't mind either way on mode, so this looks like
a good change to me.

I guess if we move enough stuff into state, the number of arguments to
this call get small enough that it no longer seems necessary to use a
struct...  

Keith