[Mesa-dev] [PATCH v2 0/3] asynchronous pbo transfer with glthread

[Nicolai Hähnle]

Hi Gregory,

Sorry, this got dropped somehow.


On 13.04.2017 17:49, gregory hainaut wrote:
> On Wed, 5 Apr 2017 12:52:03 +0200
> Gregory Hainaut <gregory.hainaut at gmail.com> wrote:
>
>>> Still, I believe there is the following bug in the series:
>>
>>> glGenBuffers(1, &pbo);
>>> glBindBuffer(..., pbo);
>>> glDeleteBuffers(1, &pbo);
>>> glTexSubImage2D(...); // will be asynchronous, but refers to user memory because the pbo was implicitly unbound
>>
>> I unbound the buffer when it is deleted in the
>> track_buffers_destruction function. See code chunk below. So I think
>> it must work. Or did I miss something ?
>>
>>> +      if (buffers[i] == glthread->pixel_pack_buffer_bound)
>>> +         glthread->pixel_pack_buffer_bound = 0;
>>> +
>>> +      if (buffers[i] == glthread->pixel_unpack_buffer_bound)
>>> +         glthread->pixel_unpack_buffer_bound = 0;
>>
>
> Hello Nicolai,
>
> Do we agree that above pattern will work with my patches ?

Yes, I'm convinced now.


>
>>
>>> Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name
>> You can't have the same name because name is still bound in context A.
> Actually I'm wrong here. glDeleteBuffers makes the name available again. Which
> lead to interesting question if the buffer is bound again after the name is reused
> by a glGenBuffers call. Anyway it isn't important.

Also agreed.

There's still the comment on patch 2&3.

Cheers,
Nicolai

>
> Chapter 5.1.3 of 4.5 core spec
> "Since the name is marked unused, binding the name will create a new object with
> the same name, and attaching the name will generate an error."
>
>
> Cheers,
> Gregory
>
>> So maybe you mean something instead for your example ?
>>
>>> Ctx A: glGenBuffers(1, &pbo);
>>> Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
>>> Ctx B: glDeleteBuffers(1, &pbo);
>> ...
>>> Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ be asynchronous (because the PBO that was generated first is still bound!)
>>
>> And yes I confirm, it isn't optimal. I did it on purpose because it is
>> much safer/easier and as you said it is a crazy case.
>>
>>
>> Cheers,
>> Gregory
>>
>> On 4/5/17, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 01.04.2017 11:42, Gregory Hainaut wrote:
>>>> Hello,
>>>>
>>>> Please find a new version to handle invalid buffer handles.
>>>>
>>>> Allow to handle this kind of case:
>>>>    genBuffer(&pbo);
>>>>    DeleteBuffer(pbo);
>>>>    BindBuffer(pbo)
>>>>    TexSubImage2D(user_memory_pointer); // Data transfer will be
>>>> synchronous
>>>>
>>>> There are various subtely to handle multi threaded shared context. In
>>>> order to
>>>> keep the code sane, I've considered a buffer invalid when it is deleted by
>>>> a
>>>> context even it is still bound to others contexts. It will force a
>>>> synchronous
>>>> transfer which is always safe.
>>>
>>> Thanks! I think you're right that tracking pointers is not actually
>>> needed as long as Create/Delete are synchronous. It's less accurate, but
>>> the inaccuracy doesn't necessarily lead to correctness problems.
>>>
>>> Still, I believe there is the following bug in the series:
>>>
>>> glGenBuffers(1, &pbo);
>>> glBindBuffer(..., pbo);
>>> glDeleteBuffers(1, &pbo);
>>> glTexSubImage2D(...); // will be asynchronous, but refers to user memory
>>> because the pbo was implicitly unbound
>>>
>>> Once you fix this bug by updating the current context's bindings in
>>> glDeleteBuffers, you'll have the following multi-context situation,
>>> which is sub-optimal but not a bug -- and applications which do that are
>>> crazy anyway, so let's not worry about them. I'm just mentioning it for
>>> completeness:
>>>
>>> Ctx A: glGenBuffers(1, &pbo);
>>> Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
>>> Ctx B: glDeleteBuffers(1, &pbo);
>>> Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name
>>> Ctx A: glDeleteBuffers(1, &pbo);
>>> Ctx A: glTexSubImage2D(...); // will be synchronous, even though it
>>> _could_ be asynchronous (because the PBO that was generated first is
>>> still bound!)
>>>
>>> Cheers,
>>> Nicolai
>>>
>>>
>>>>
>>>> Gregory Hainaut (3):
>>>>   mesa/glthread: track buffer creation/destruction
>>>>   mesa/glthread: add tracking of PBO binding
>>>>   mapi/glthread: generate asynchronous code for PBO transfer
>>>>
>>>>  src/mapi/glapi/gen/ARB_direct_state_access.xml |  18 +--
>>>>  src/mapi/glapi/gen/ARB_robustness.xml          |   2 +-
>>>>  src/mapi/glapi/gen/gl_API.dtd                  |  10 +-
>>>>  src/mapi/glapi/gen/gl_API.xml                  |  32 +++---
>>>>  src/mapi/glapi/gen/gl_marshal.py               |  23 +++-
>>>>  src/mapi/glapi/gen/marshal_XML.py              |  19 +++-
>>>>  src/mesa/main/glthread.h                       |  10 ++
>>>>  src/mesa/main/marshal.c                        | 149
>>>> ++++++++++++++++++++++++-
>>>>  src/mesa/main/marshal.h                        |  24 ++++
>>>>  src/mesa/main/mtypes.h                         |   5 +
>>>>  src/mesa/main/shared.c                         |   4 +
>>>>  11 files changed, 257 insertions(+), 39 deletions(-)
>>>>
>>>
>>>
>>> --
>>> Lerne, wie die Welt wirklich ist,
>>> Aber vergiss niemals, wie sie sein sollte.
>>>


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