Proposing new CHROMIUM_sync_point API.

Proposing new CHROMIUM_sync_point API.

BUG=514815
Committed: https://crrev.com/126379c24691644db5bd83a312ca53d2a6f11a3c
Cr-Commit-Position: refs/heads/master@{#347824}

[David Yen, 2015-09-08 17:11:51]

dyen@chromium.org changed reviewers:
+ piman@chromium.org

[David Yen, 2015-09-08 17:11:52]

Before I begin implementing all the functions, here is my proposed new API for
GL_CHROMIUM_sync_point. Does it look okay to everyone?

Also with respect to future sync points, since nothing is using it should we
just disable those functions for now? We can probably make it work with the new
global order scheme.

[piman, 2015-09-08 18:21:01]

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:63: an INVALID_OPERATION
error is generated.
Can you clarify whether the CHROMIUMsync handle namespace is per-context,
per-share-group, per-client or global across all clients?

If one of the first 2, one would expect the CHROMIUMsync handle is deleted when
the context/share-group (respectively) is destroyed, like other similar names.
What about the later 2?

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:82: commands that are
already blocking on <sync_point>.
I'm not sure what "already blocking on <sync_point>" means. That wait happens
server-side, on another context/stream, so before/after is not well-defined.

[David Yen, 2015-09-08 19:07:53]

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:63: an INVALID_OPERATION
error is generated.
On 2015/09/08 18:21:01, piman (slow to review) wrote:
> Can you clarify whether the CHROMIUMsync handle namespace is per-context,
> per-share-group, per-client or global across all clients?
> 
> If one of the first 2, one would expect the CHROMIUMsync handle is deleted
when
> the context/share-group (respectively) is destroyed, like other similar names.
> What about the later 2?

Improved documentation on this. It is shared globally, but I have not decided
how the lifetime would be if the context was destroyed. It would probably be
well behaved if we just deleted the object on destruction of the context, but
technically waits do not care if the context is destroyed or not. If the other
contexts can get the gpu_channel id, routing id, and sync point id we can still
wait on it as long as all the commands are queued before it was destroyed. To
have that working though, we would need some way to ref count the object and
track who has the handle. I'm not sure if I want to do all of that.

Another simple solution would be if we just replaced the sync point object and
returned those 3 numbers directly. The client can then IPC the 3 integers to
other contexts without worrying about any lifetimes, but that may be leaking too
much implementation details and also does not give us the flexibility of
expanding the object later. Although I can't think of a reason why we would need
anymore than those 3, even streams do not add any extra necessary information.
Another downside is that it would also make it so they don't need to go through
GenSyncPointCHROMIUM (which verifies flushes are done correctly), since the gpu
channel and routing id are always the same.

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:82: commands that are
already blocking on <sync_point>.
On 2015/09/08 18:21:01, piman (slow to review) wrote:
> I'm not sure what "already blocking on <sync_point>" means. That wait happens
> server-side, on another context/stream, so before/after is not well-defined.

I wanted to communicate that on the client side the commands do not rely on the
CHROMIUMsync object to exist before it is flushed to the server. As long as the
WaitSyncPointCHROMIUM is called and it extracts out the necessary information
(gpu channel, routing id, sync point number) the sync point object can be
deleted.

I've tried to improve the documentation about this more.

[jbauman, 2015-09-08 20:54:12]

jbauman@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2015-09-08 20:54:13]

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: typedef struct
__CHROMIUMsync *CHROMIUMsync;
What exactly is __CHROMIUMsync? Can it be passed via IPC?

[David Yen, 2015-09-08 21:35:18]

On 2015/09/08 20:54:13, jbauman wrote:
>
https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):
> 
>
https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: typedef struct
> __CHROMIUMsync *CHROMIUMsync;
> What exactly is __CHROMIUMsync? Can it be passed via IPC?

As discussed with piman@, I changed it to use a fixed byte array instead to make
lifetime tracking easier. This should simplify things. For one, we no longer
need a deletion function.

[David Yen, 2015-09-08 21:35:29]

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/1/gpu/GLES2/extensions/CHROMI...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: typedef struct
__CHROMIUMsync *CHROMIUMsync;
On 2015/09/08 20:54:13, jbauman wrote:
> What exactly is __CHROMIUMsync? Can it be passed via IPC?

In order to make the tracking of lifetimes easier, I have changed this to use
something similar to how mailbox's are implemented. It makes it easier to pass
it via IPC as well.

[piman, 2015-09-08 22:19:48]

LGTM overall for the API shape, but see nits.

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: inserts a sync point
insertion point in the current command stream. The
nit: "inserts a sync point insertion point" doesn't read very well. I could be
useful to define our own terms, because we're using "sync point" a bit
loosely... I guess there's now 2 separated concepts:
1- the position in the current context that represents the point in the
execution that we will wait on (the uint)
2- an opaque token that globally represents that position, and that can be used
to wait (the GLbyte array)

I think if we put a name on each of them, this spec would read a little better.
We can rename the entrypoints in accordance too. Could be "execution point" (for
1) and "sync token" (for 2), but I'm not married to these names.

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:40: sync point acts as a
fence, which is signaled when previous commands have been submitted to the
server, or when the context is destroyed,
nit: wrapping at 80 columns

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:57: <sync_point> returns a
GL_SYNC_POINT_SIZE_CHROMIUM byte sized name
nit: .

[David Yen, 2015-09-08 22:54:14]

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: inserts a sync point
insertion point in the current command stream. The
On 2015/09/08 22:19:47, piman (slow to review) wrote:
> nit: "inserts a sync point insertion point" doesn't read very well. I could be
> useful to define our own terms, because we're using "sync point" a bit
> loosely... I guess there's now 2 separated concepts:
> 1- the position in the current context that represents the point in the
> execution that we will wait on (the uint)
> 2- an opaque token that globally represents that position, and that can be
used
> to wait (the GLbyte array)
> 
> I think if we put a name on each of them, this spec would read a little
better.
> We can rename the entrypoints in accordance too. Could be "execution point"
(for
> 1) and "sync token" (for 2), but I'm not married to these names.

Maybe just use "fence sync" and "sync object" to match how the ARB_sync spec is
written? So it would be InsertFenceSyncCHROMIUM, GenSyncObjectCHROMIUM, and
WaitSyncObjectCHROMIUM.

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:40: sync point acts as a
fence, which is signaled when previous commands have been submitted to the
server, or when the context is destroyed,
On 2015/09/08 22:19:47, piman (slow to review) wrote:
> nit: wrapping at 80 columns

Done.

https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:57: <sync_point> returns a
GL_SYNC_POINT_SIZE_CHROMIUM byte sized name
On 2015/09/08 22:19:47, piman (slow to review) wrote:
> nit: .

Done.

[piman, 2015-09-08 23:02:36]

On Tue, Sep 8, 2015 at 3:54 PM, <dyen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):
>
>
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: inserts a sync
> point insertion point in the current command stream. The
> On 2015/09/08 22:19:47, piman (slow to review) wrote:
>
>> nit: "inserts a sync point insertion point" doesn't read very well. I
>>
> could be
>
>> useful to define our own terms, because we're using "sync point" a bit
>> loosely... I guess there's now 2 separated concepts:
>> 1- the position in the current context that represents the point in
>>
> the
>
>> execution that we will wait on (the uint)
>> 2- an opaque token that globally represents that position, and that
>>
> can be used
>
>> to wait (the GLbyte array)
>>
>
> I think if we put a name on each of them, this spec would read a
>>
> little better.
>
>> We can rename the entrypoints in accordance too. Could be "execution
>>
> point" (for
>
>> 1) and "sync token" (for 2), but I'm not married to these names.
>>
>
> Maybe just use "fence sync" and "sync object" to match how the ARB_sync
> spec is written? So it would be InsertFenceSyncCHROMIUM,
> GenSyncObjectCHROMIUM, and WaitSyncObjectCHROMIUM.


SGTM, though #2 is not really an "object" in the GL sense (it doesn't have
a lifetime).


>
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:40: sync point
> acts as a fence, which is signaled when previous commands have been
> submitted to the server, or when the context is destroyed,
> On 2015/09/08 22:19:47, piman (slow to review) wrote:
>
>> nit: wrapping at 80 columns
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:57: <sync_point>
> returns a GL_SYNC_POINT_SIZE_CHROMIUM byte sized name
> On 2015/09/08 22:19:47, piman (slow to review) wrote:
>
>> nit: .
>>
>
> Done.
>
> https://codereview.chromium.org/1323263005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2015-09-08 23:09:00]

I renamed sync object to sync token instead.

On 2015/09/08 23:02:36, piman (slow to review) wrote:
> On Tue, Sep 8, 2015 at 3:54 PM, <mailto:dyen@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> > File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt (right):
> >
> >
> >
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:39: inserts a sync
> > point insertion point in the current command stream. The
> > On 2015/09/08 22:19:47, piman (slow to review) wrote:
> >
> >> nit: "inserts a sync point insertion point" doesn't read very well. I
> >>
> > could be
> >
> >> useful to define our own terms, because we're using "sync point" a bit
> >> loosely... I guess there's now 2 separated concepts:
> >> 1- the position in the current context that represents the point in
> >>
> > the
> >
> >> execution that we will wait on (the uint)
> >> 2- an opaque token that globally represents that position, and that
> >>
> > can be used
> >
> >> to wait (the GLbyte array)
> >>
> >
> > I think if we put a name on each of them, this spec would read a
> >>
> > little better.
> >
> >> We can rename the entrypoints in accordance too. Could be "execution
> >>
> > point" (for
> >
> >> 1) and "sync token" (for 2), but I'm not married to these names.
> >>
> >
> > Maybe just use "fence sync" and "sync object" to match how the ARB_sync
> > spec is written? So it would be InsertFenceSyncCHROMIUM,
> > GenSyncObjectCHROMIUM, and WaitSyncObjectCHROMIUM.
> 
> 
> SGTM, though #2 is not really an "object" in the GL sense (it doesn't have
> a lifetime).
> 
> 
> >
> >
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:40: sync point
> > acts as a fence, which is signaled when previous commands have been
> > submitted to the server, or when the context is destroyed,
> > On 2015/09/08 22:19:47, piman (slow to review) wrote:
> >
> >> nit: wrapping at 80 columns
> >>
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/1323263005/diff/40001/gpu/GLES2/extensions/CH...
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt:57: <sync_point>
> > returns a GL_SYNC_POINT_SIZE_CHROMIUM byte sized name
> > On 2015/09/08 22:19:47, piman (slow to review) wrote:
> >
> >> nit: .
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/1323263005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2015-09-08 23:17:30]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-09-08 23:17:31]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1323263005/#ps100001
(title: "Renamed sync object to sync token")

[commit-bot: I haz the power, 2015-09-08 23:18:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323263005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323263005/100001

[commit-bot: I haz the power, 2015-09-08 23:24:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-08 23:25:04]

Patchset 6 (id:??) landed as
https://crrev.com/126379c24691644db5bd83a312ca53d2a6f11a3c
Cr-Commit-Position: refs/heads/master@{#347824}