[Intel-gfx] [PATCH 6/6] Move vertex buffer out of gen4_static_state into new gen4_dynamic_state

[Carl Worth]

On Wed, 2008-10-22 at 20:45 -0700, Eric Anholt wrote:
> ACK on the previous patches,

Thanks. Those are pushed.

>  and comments inline on this one.

Much thanks for the review! I'll follow up with a new patch addressing
the issues you've raised.

> >  #define MAX_VERTEX_PER_COMPOSITE    24
> > -#define MAX_VERTEX_BUFFERS	    256
> > +#define VERTEX_BUFFER_SIZE	    (256 * MAX_VERTEX_PER_COMPOSITE)
> 
> This isn't your fault (it may even be mine), but I'd love to see
> MAX_VERTEX_PER_COMPOSITE get renamed.  The current value of 24 means 4
> vertices * 3 texcoords/vertex * 2 floats/texcoord, i.e. not actually a
> number of vertices.  Maybe MAX_FLOATS_PER_COMPOSITE or something?

It probably was my fault originally. I went with
VERTEX_FLOATS_PER_COMPOSITE so there's at least some sense of
namespacing on the defines, and I added a comment with the math leading
to this value.
 
> > +    bo_table[0] = render_state->dynamic_state_bo;
> 
> You need to include the batchbuffer in the array, otherwise the check
> won't know about all the other things you're planning to load in with
> this batchbuffer.

Fixed.

> > +    /* Arrange for a dynamic_state buffer object with sufficient space
> > +     * for our vertices. */
> > +    if (render_state->vb_offset + MAX_VERTEX_PER_COMPOSITE > VERTEX_BUFFER_SIZE) {
> > +	dri_bo_unreference (render_state->dynamic_state_bo);
> > +
> > +	render_state->dynamic_state_bo = dri_bo_alloc (pI830->bufmgr, "vb",
> > +						       sizeof (gen4_dynamic_state),
> > +						       4096);
> >  	render_state->vb_offset = 0;
> >      }
>
> When you allocate a new vb here, you need to check_aperture again.
> Otherwise you may end up accumulating a batchbuffer referencing a ton of
> VBs so that you can't fit the whole thing in at once.  Not big deal
> today (particularly since the assert that you tripped over last time is
> gone), but when we've got our pixmaps in BOs you'll near the failing
> point a lot more often.  Basically, just set up the array again with
> your batch and your new vb, and if it's too big then flush.  After that,
> you're sure that you'll be successful post-flushing and don't need to
> check again.

I felt a little bad about duplicating the allocation code, so knowing
that I need to check_aperture here as well makes it easy to justify a
new function for this common functionality.

I've got some lame name in the patch now, (_allocate_dynamic_state or
so). But this function will soon acquire the job of emitting all setup
into the batch, (which will be required as we switch surface_state and
binding_table to buffer objects), so that will force a rename of the
function anyway.

-Carl

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081023/bc0e9bed/attachment.sig>