[Mesa-dev] [RFC PATCH] gallium: add a common uploader to pipe_context

[Marek Olšák]

On Mon, Feb 6, 2017 at 9:58 PM, Brian Paul <brianp at vmware.com> wrote:
> On 02/06/2017 12:11 PM, Marek Olšák wrote:
>>
>> On Mon, Feb 6, 2017 at 5:15 PM, Brian Paul <brianp at vmware.com> wrote:
>>>
>>> On 02/03/2017 02:41 PM, Marek Olšák wrote:
>>>>
>>>>
>>>> On Fri, Feb 3, 2017 at 9:45 PM, Brian Paul <brianp at vmware.com> wrote:
>>>>>
>>>>>
>>>>> On 02/01/2017 02:23 PM, Brian Paul wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 01/27/2017 04:00 AM, Marek Olšák wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Jan 27, 2017 at 10:05 AM, Nicolai Hähnle <nhaehnle at gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 27.01.2017 00:51, Marek Olšák wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>>>>>
>>>>>>>>> For lower memory usage and more efficient updates of the buffer
>>>>>>>>> residency
>>>>>>>>> list. (e.g. if drivers keep seeing the same buffer for many
>>>>>>>>> consecutive
>>>>>>>>> "add" calls, the calls can be turned into no-ops trivially)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This makes sense to me, but how are you planning to deal with the
>>>>>>>> bind
>>>>>>>> flags? They are currently set differently for different upload mgrs.
>>>>>>>> We
>>>>>>>> should probably do away with them entirely anyway.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Drivers can set the bind flags they need. Some drivers will set all 3
>>>>>>> bind flags. Other drivers don't have to set any.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I need to look into this part more closely.  I think we may have
>>>>>> trouble
>>>>>> mixing constants with index/vertex data in our VMware driver...
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Marek,
>>>>>
>>>>> Your patch series, as-is, did indeed cause trouble with our VMware
>>>>> driver.
>>>>> We need to keep constants in a separate buffer.
>>>>>
>>>>> The good news is I don't think this is a huge problem and I've updated
>>>>> (a
>>>>> subset of) your patches to accommodate both your needs and ours.
>>>>>
>>>>> The basic idea is to add a pipe_context::get_stream_uploader() hook
>>>>> that
>>>>> allows drivers to use just one or separate uploaders for
>>>>> vertex/index/constant data.  Plus, I added a
>>>>> pipe_context::unmap_stream_uploaders() helper, but this isn't strictly
>>>>> necessary.
>>>>>
>>>>> WIP patch attached (only lightly tested).  Let me know what you think.
>>>>
>>>>
>>>>
>>>> Can we simply add these 2 fields into pipe_context instead of the
>>>> callback?
>>>>
>>>> pipe_context::stream_uploader // vertex + index
>>>> pipe_context::const_uploader
>>>
>>>
>>>
>>> Yeah, that might work too.  Though, now I have to test the case of vertex
>>> data and index data being in the same VBO.  We may need three uploader
>>> pointers...
>>
>>
>> What's the issue with svga that it needs different buffers for each
>> type? OpenGL allows the same buffer to be used for vertex, index,
>> const, even the same buffer range.
>
>
> Our virtual device protocol uses DX10 conventions and in DX10, constant
> buffers are distinct from vertex/index buffers.  That is, you can't set both
> D3D10_BIND_CONSTANT_BUFFER and D3D10_BIND_VERTEX_BUFFER for one buffer.
> Luckily, we haven't come across any GL apps that need that (and it wouldn't
> be too hard to work around it if needed).
>
> I was pretty sure that vertex+index data in one buffer was OK but I wanted
> to check.  Since I don't think we have any piglit tests (or apps) that
> exercise that specific case, I just wrote one and it seems to work fine.
>
> So, pipe_context::stream_uploader + pipe_context::const_uploader should be
> OK for us.  Do you want to implement that?

Yes, I'll add const_uploader.

Marek