Skip to content

Call glFlush after eglCreateSync, not before#15

Open
kbrenneman wants to merge 1 commit intoNVIDIA:mainfrom
kbrenneman:flush-after-create-sync
Open

Call glFlush after eglCreateSync, not before#15
kbrenneman wants to merge 1 commit intoNVIDIA:mainfrom
kbrenneman:flush-after-create-sync

Conversation

@kbrenneman
Copy link
Collaborator

By the EGL_ANDROID_native_fence_sync spec, when you create a fence with eglCreateSync, the file descriptor isn't valid until the next glFlush, so we should be calling glFlush after eglCreateSync, not before.

In SyncRendering, when we need to create a new syncfd, call glFlush
after eglCreateSync, not before.

According to the EGL_ANDROID_native_fence_sync spec, when you create a
new EGLSync, it's not until the next flush that it'll assign the
EGL_SYNC_NATIVE_FENCE_FD_ANDROID attribute.
goto done;
}

pwin->inst->platform->priv->egl.Flush();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite what the spec claims, this isn't actually how our implementation behaves. It flushes internally during CreateSync() and attaches the fence created by that flush operation. I think this was and is still just perturbing some timing/race if it affects anything at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible -- I'm still trying to make sense of that myself.

But at the very least, this is the one sequence that the spec requires to work, so even if there's a driver bug lurking, this should continue to work once it's fixed, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that it won't make things worse, yes. I just worry there's a worse bug somewhere that this continues to paper over. This might actually break the papering over, in that the after-create flush will probably get reduced to a no-op by the driver logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus far, I haven't managed to get an incorrect result, short of trying to draw a frame that takes long enough to cause timeouts (in which case the result is identical to the current code).

The fact that eglQuerySync without a glFlush gets signaled at the wrong point does suggest a driver bug, but matching the spec seems like the best hedge against whatever changes in driver behavior might happen in the future.

Strictly speaking, with the current code in egl-x11, the EGLSync isn't even required to be signaled at all.

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

Successfully merging this pull request may close these issues.

2 participants