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

[Brian Paul]

On 04/30/2012 02:37 PM, Marek Olšák wrote:
> 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.

OK, in this particular case, the arrays are in user memory.

I'll post a few patches soon.

-Brian