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

[Dieter Nützel]

Hello Gregory,

do you have the v3 'ready'?
v2 do NOT apply any longer since Samuel Pitoiset's changes for 'bind 
less'.

I'm running with this on Turks XT since you've sent it.

Greetings,
Dieter

Am 13.04.2017 17:49, schrieb gregory hainaut:

On Wed, 5 Apr 2017 12:52:03 +0200
Gregory Hainaut  wrote:



>> 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(-)

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  wrote:


Still, I believe there is the following bug in the series:



glGenBuffers(1, );
glBindBuffer(..., pbo);
glDeleteBuffers(1, );
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, ); // 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, );
Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
Ctx B: glDeleteBuffers(1, );

...

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  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();
   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, );
glBindBuffer(..., pbo);
glDeleteBuffers(1, );
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, );
Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
Ctx B: glDeleteBuffers(1, );
Ctx A: glGenBuffers(1, ); // assume that this gets the same buffer name
Ctx A: glDeleteBuffers(1, );
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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[gregory hainaut]

On Wed, 5 Apr 2017 12:52:03 +0200
Gregory Hainaut  wrote:

> > Still, I believe there is the following bug in the series:
> 
> > glGenBuffers(1, );
> > glBindBuffer(..., pbo);
> > glDeleteBuffers(1, );
> > 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, ); // 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, );
> > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
> > Ctx B: glDeleteBuffers(1, );
> ...
> > 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  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();
> >>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, );
> > glBindBuffer(..., pbo);
> > glDeleteBuffers(1, );
> > 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, );
> > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
> > Ctx B: glDeleteBuffers(1, );
> > Ctx A: glGenBuffers(1, ); // assume that this gets the same buffer name
> > Ctx A: glDeleteBuffers(1, );
> > 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.
> >
___
mesa-dev mailing 

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

[Gregory Hainaut]

> Still, I believe there is the following bug in the series:

> glGenBuffers(1, );
> glBindBuffer(..., pbo);
> glDeleteBuffers(1, );
> 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;




> Ctx A: glGenBuffers(1, ); // assume that this gets the same buffer name
You can't have the same name because name is still bound in context A.

So maybe you mean something instead for your example ?

> Ctx A: glGenBuffers(1, );
> Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
> Ctx B: glDeleteBuffers(1, );
...
> 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  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();
>>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, );
> glBindBuffer(..., pbo);
> glDeleteBuffers(1, );
> 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, );
> Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
> Ctx B: glDeleteBuffers(1, );
> Ctx A: glGenBuffers(1, ); // assume that this gets the same buffer name
> Ctx A: glDeleteBuffers(1, );
> 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.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Nicolai Hähnle]

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();
   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, );
glBindBuffer(..., pbo);
glDeleteBuffers(1, );
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, );
Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo);
Ctx B: glDeleteBuffers(1, );
Ctx A: glGenBuffers(1, ); // assume that this gets the same buffer name
Ctx A: glDeleteBuffers(1, );
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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gregory Hainaut]

Hello,

Please find a new version to handle invalid buffer handles.

Allow to handle this kind of case:
   genBuffer();
   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.

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(-)

-- 
2.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev