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

[gregory hainaut]

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 ?
 
> 
> > 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.

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.
> >