[Mesa-dev] [PATCH 3/9] st/mesa: make user index buffers optional

[Marek Olšák]

On Mon, Apr 30, 2012 at 9:48 PM, Brian Paul <brianp at vmware.com> wrote:
> On 04/30/2012 12:54 PM, Marek Olšák wrote:
>>
>> On Mon, Apr 30, 2012 at 7:53 PM, Brian Paul<brianp at vmware.com>  wrote:
>>>
>>> On 04/26/2012 12:54 PM, Jose Fonseca wrote:
>>>>
>>>>
>>>> ----- Original Message -----
>>>>>
>>>>>
>>>>> ---
>>>>>  src/mesa/state_tracker/st_context.c    |    4 +++-
>>>>>  src/mesa/state_tracker/st_context.h    |    1 +
>>>>>  src/mesa/state_tracker/st_draw.c       |    5 +++++
>>>>>  src/mesa/state_tracker/st_extensions.c |    4 ++++
>>>>>  4 files changed, 13 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/state_tracker/st_context.c
>>>>> b/src/mesa/state_tracker/st_context.c
>>>>> index 84aae81..164cc45 100644
>>>>> --- a/src/mesa/state_tracker/st_context.c
>>>>> +++ b/src/mesa/state_tracker/st_context.c
>>>>> @@ -155,7 +155,9 @@ st_create_context_priv( struct gl_context *ctx,
>>>>> struct pipe_context *pipe )
>>>>>     st->dirty.mesa = ~0;
>>>>>     st->dirty.st = ~0;
>>>>>
>>>>> -   st->uploader = u_upload_create(st->pipe, 65536, 4,
>>>>> PIPE_BIND_VERTEX_BUFFER);
>>>>> +   st->uploader = u_upload_create(st->pipe, 128 * 1024, 4,
>>>>> +                                  PIPE_BIND_VERTEX_BUFFER |
>>>>> +                                  PIPE_BIND_INDEX_BUFFER);
>>>>
>>>>
>>>>
>>>> Marek,
>>>>
>>>> Instead of lumping this into the same hardware buffer, I think it would
>>>> be
>>>> better to use two separate uploaders so that the driver can effectively
>>>> do
>>>> optimization based on PIPE_BIND_VERTEX_BUFFER or PIPE_BIND_INDEX_BUFFER.
>>>>  A
>>>> quick look on current drivers showed that they do look at these bind
>>>> flags.
>>>>
>>>> Otherwise I don't see anything wrong with this series. It seems a nice
>>>> cleanup/speedup.
>>>>
>>>> Brian's OOTO till Monday, so allow more time for him to comment.
>>>>
>>>> Also, once you updated the series, please provide it in a clonable git
>>>> branch for testing.
>>>
>>>
>>>
>>> I'm testing the gallium-userbuf branch now.  With piglit's draw-batch
>>> test
>>> there's a failing assertion in svga_resource_buffer_upload.c at line 557
>>> (the buffer in question is in a mapped state).  I disabled the assertion
>>> but
>>> then the test fails (it passes on master).  I'll try to dig a big deeper.
>>
>>
>> Ooops, sorry about that. There's a missing call to u_upload_unmap in
>> setup_index_buffer (st_draw.c). I'll commit a fix in a moment.
>
>
> OK, that helps.  Thanks.
>
> But now I'm seeing a crash w/ google earth.  At line 70 of svga_swtnl_draw.c
> (below) there's a null vertex buffer pointer (svga->curr.vb[0].buffer=null).
>
> 64
> 65         /*
> 66          * Map vertex buffers
> 67          */
> 68         for (i = 0; i < svga->curr.num_vertex_buffers; i++) {
> 69            map = pipe_buffer_map(&svga->pipe,
> 70                                  svga->curr.vb[i].buffer,
> 71                                  PIPE_TRANSFER_READ,
> 72                                  &vb_transfer[i]);
> 73
>
> It looks like when pipe->set_vertex_buffers() is called, some of the buffers
> in [0..count-1] may be null.  I don't think that ever happened before.  That
> is, buffers[0..num_buffers-1] would always be non-null.
>
> The set_vertex_buffers() function is called from u_vbuf.c.  I took a quick
> look but I'm not sure what's all going on there.
>
> I hacked the svga code to avoid/skip the null buffers so I guess I can fix
> things there.  But maybe you can clarify the story with the buffers passed
> to pipe->set_vertex_buffers().

One of the codepaths in u_vbuf uses the translate module, which
usually takes a new vertex buffer slot. If you receive this in
set_vertex_buffers: count=4, buffers={NULL, NULL, NULL, buffer}, it
means the last buffer probably comes from translate and the other 3
were originally user buffers or buffers with an unaligned offset or
stride, which u_vbuf never lets through, so they end up being NULL.
The NULL buffers are never used by the vertex element state.

Usually the [min_index, max_index] range of vertex buffers is uploaded
and then the resulting non-user buffers occupy the same slots. The
exception is an indexed draw call, which has scattered vertices (i.e.
max_index-min_index is much greater than the vertex count), then the
translate module is used to turn an indexed draw call into a
non-indexed one. This is the only case where you may get a NULL buffer
if you support everything but user buffers.

Marek