[Mesa-dev] Unified pipe_context::draw_vbo thoughts

[Chia-I Wu]

On Wed, Jul 21, 2010 at 10:29 PM, Keith Whitwell <keithw at vmware.com> wrote:
> 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.
I was thinking about the case where there is no element array object bound.
Despite what a pipe driver might do internally, it is simpler for st/mesa to
wrap them in multiple pipe user buffers.
>> 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.
If we try to support glMultiDrawElements directly in the interface, then to
support the case where the offsets to the element array are not aligned with
the index size, two possible ways I see now are to, a) allow multiple index
buffers as "offset" is part of the index buffer state, or b), remove "offset"
from pipe_index_buffer and add "index_offset" to pipe_draw_info.  Neither ways
seem to fit how the hardware works.
> 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...
It is likely.  The idea of this patch is to (make index buffer a state and)
have a unified "draw" function.  To me, a struct is easier to work with when it
is unclear what belongs, or will belong, to the draw command.  It is kind of
like having "pipe_context::set_draw_state", where what belongs to the state
may change over time.

-- 
olv at LunarG.com