Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EGL_ANDROID_native_fence_sync: Clarification of what a "fence command" is #94

Open
ascent12 opened this issue Nov 13, 2019 · 9 comments
Assignees

Comments

@ascent12
Copy link

This arises from a mesa issue found here: https://gitlab.freedesktop.org/mesa/mesa/issues/2080

In the situation where the OpenGL command stream contains

eglCreateSyncKHR(EGL_SYNC_NATIVE_FENCE_ANDROID);
glClear(); // i.e. do some work
eglCreateSyncKHR(EGL_SYNC_NATIVE_FENCE_ANDROID);

and is flushed, when the first native fence is extracted, the radeonsi mesa driver returns a native fence from the previous submission rather than creating a new one. This leads to surprising results when we extract the timestamp from this fence (using Linux-specific APIs).

From the spec:

When a fence sync object is created or when an EGL native fence sync object is created with the EGL_SYNC_NATIVE_FENCE_FD_ANDROID attribute set to EGL_NO_NATIVE_FENCE_FD_ANDROID, eglCreateSyncKHR also inserts a fence command into the command stream of the bound client API's current context (i.e., the context returned by eglGetCurrentContext), and associates it with the newly created sync object.

To me, this would imply that a new native fence is created and is signalled when the GPU reaches that point in the OpenGL command stream, assuming that what what a "fence command" means.

But it was argued that this is not what a fence command is, and it is really just telling the driver to fetch the fence from the last batch of work that was completed:
https://gitlab.freedesktop.org/mesa/mesa/issues/2080#note_289275

Is radeonsi's behaviour conformant to this specification and my understanding is wrong? Is the spec not worded precisely enough, and should it be changed to clarify it?

@critsec
Copy link
Member

critsec commented Nov 13, 2019

Fences signal when all work submitted before them is completed. Their primary use case is synchronization, not timing. And in general for synchronization you want to unblock the dependent work as early as possible.

So that leads to designs where if you already have a synchronization object marking the end of previous work, and you get a request to create a new EGLSync, you just make it refer to that existing synchronization object. When you do this, sometimes you naturally end up with fences that have a signal timestamp from before the EGLSync was created. For synchronization, that's perfectly fine and in fact usually what you want.

@ppaalanen
Copy link

Right, but the problem is the wording in the spec.

The spec says, as quoted above, that creating a fence will insert a fence command into the command stream. That means that new work is pending, created by creating the fence. The new work (fence command) must then be submitted to the GPU to be executed. The fence signals when this work has been completed.

I really cannot see any other way to interpret the spec wording.

@stonesthrow
Copy link
Contributor

What critsec described is reality. Unfortunately you can't infer Android specific behavior from this interpretation. Sorry can't give you a more useful answer. Closing.

@ppaalanen
Copy link

Why not fix the spec then?

Remove the word "command" from the phrase "fence command", and add a Q&A that explains that fences are not executable commands but something else. Also, remove the wording that says "inserts a command into the command stream" because implementations obviously do not do that, and apparently cannot do that.

The point here is to figure out what the spec means and clarify the situation.

Please, re-open.

@ppaalanen
Copy link

As it apparently has been established that the current implementations are correct with respect to the spirit of the specification, then the wording of the specification needs to be corrected to not mislead application developers.

@critsec critsec self-assigned this Jan 16, 2020
@critsec
Copy link
Member

critsec commented Jan 16, 2020

I agree the language could be a little better. Re-opening and assigning myself, though I'm pretty busy at the moment and might not get to it for a bit.

@critsec critsec reopened this Jan 16, 2020
@ppaalanen
Copy link

Thank you very much. Looking forward to seeing a new wording while not holding my breath. ;-)

@emersion
Copy link

Hi, any news about this @critsec? Just got hit by the same misunderstanding.

@emersion
Copy link

emersion commented Sep 6, 2021

Ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants