[Mesa-dev] [PATCH] mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound

[Nicolai Hähnle]

On 28.03.2017 22:37, gregory hainaut wrote:
> On Mon, 27 Mar 2017 16:10:32 +0200
> Gregory Hainaut <gregory.hainaut at gmail.com> wrote:
>
>> Hello,
>>
>> Sorry I was in vacation. I will update my patch with a hash map to
>> trace buffer creation/destruction.
>>
>> Cheers,
>> Gregory
>>
>> On 3/20/17, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 20.03.2017 14:33, Markus Wick wrote:
>>>> Am 2017-03-20 14:21, schrieb Nicolai Hähnle:
>>>>> On 17.03.2017 18:59, gregory hainaut wrote:
>>>>>> If the application is badly/strangely coded, glthread will make it
>>>>>> worst.
>>>>>> The solution ought to be either fix the app or don't use glthread.
>>>>>
>>>>> It would be nice if glthread could handle this properly, but I don't
>>>>> currently see how.
>>>>
>>>> The dispatcher thread needs a map of all valid buffer objects. So we
>>>> need to update such a map on all glGenBuffers/glDeleteBuffers calls. So
>>>> the overhead will be the map lookup on all affected glBindBuffer calls.
>>>
>>> You're right. Ridiculous details of the OpenGL spec make this
>>> interesting, actually, because Section 6.1 (Creating and Binding Buffer
>>> Objects) of the OpenGL 4.5 (Compability Profile) spec says:
>>>
>>> "A buffer object is created by binding an unused name to a buffer
>>> target. The binding is effected by calling
>>>
>>>      void BindBuffer( enum target, uint buffer );
>>>
>>> target must be one of the targets listed in table 6.1. If the buffer
>>> object named buffer has not been previously bound, or has been deleted
>>> since the last binding, the GL creates a new state vector, initialized
>>> with a zero-sized memory buffer and comprising all the state and with
>>> the same initial values listed in table 6.2."
>>>
>>> But this language is different from that for core profiles, where a call
>>> to glGenBuffers is required. So in this case, the compatibility profile
>>> spec is actually better for performance :/
>>>
>>> Cheers,
>>> Nicolai
>>>
>
> Hello,
>
> Actually I found another issue on this patch. A deleted buffer is unbound from the context.
> So validity must be updated accordingly.
>
> However I have a tricky question, I think that it is possible to share buffers
> between multiple contexts. Do we want to support it with glthread ? If yes, it will
> require either

Ouch, yeah. It's getting complicated.


> * to duplicate the reference counting logic

Do we really need to duplicate the logic? Perhaps the shadow copy of 
bindings in the application thread can have pointers to the buffer 
objects as well? Then it's the same reference counting logic, and some 
code can be shared between the application and GL threads.

This requires creating buffer objects in the application thread, and 
changing the marshalling of GenBuffers and CreateBuffers to pass those 
pointers, but I think it's doable.

I think that Gen/CreateBuffers could even be asynchronous, though I 
think that's dependent on all buffer ops doing the buffer name lookups 
in the application thread -- otherwise you get buffer objects "traveling 
back in time", and code like this might execute successfully:

   glNamedBufferStorage(N, ...); /* N does not exist yet */
   glCreateBuffers(1, &bufobj); /* Allocates buffer name N as buffer */

Maybe it all boils down to this: If buffer name handling is spread 
across the application thread and the GL thread, things get tricky and 
there are corner cases to watch out for.

So one option is to move all buffer name handling into the application 
thread. This means that all API functions that take a buffer name as 
parameter need special marshaling that looks up the buffer object in the 
application thread and marshals the buffer object pointer instead of the 
buffer name. There's an awful lot of those functions with DSA, but at 
least it should still be possible to autogen the marshaling.

For API functions that take a buffer target, we'd have a choice of 
having them work as before, or adjusting them in the same way. Adjusting 
them in the same way would put more load on the application thread 
without a clear advantage, so best to leave them as-is.

I like that it's a conceptually clean approach, but it's an awfully big 
change.


> * Or to use a synchronous delete and peep into the context to check if the buffer is still alive

As long as the shadow copy of bindings is in terms of buffer names, you 
need to watch out for the ABA-style problem that you get with 
asynchronous delete:

1. Context A has a binding of buffer object currently named N.
2. Context B deletes buffer object N.
3. Context A create buffer object, ends up re-using buffer name N.
4. Context A deletes buffer object, re-uses buffer name N.

At step 4, the application thread will scan the shadow bindings for 
context A, see the binding of buffer name N, and unbind it. But that's 
incorrect, because the automatic unbinding is based on the buffer 
object, not the buffer name.

I guess this could be fixed by a separate reference count for the buffer 
names, but I haven't thought that through fully.

Hmm. I think synchronous create + delete, with a shadow copy of bindings 
that stores pointers instead of buffer names could work and be 
relatively lightweight.

Cheers,
Nicolai


> * Or any better idea ^^


>
> Cheers,
> Gregory
>
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.