1743632 - Strange FD management in DMABufSurface

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jan Alexander Steffens [:heftig]]

While looking at DMABufSurface.cpp, I noticed the following code:

  const EGLint attribs[] = {LOCAL_EGL_SYNC_NATIVE_FENCE_FD_ANDROID, mSyncFd,
                            LOCAL_EGL_NONE};
  mSync = egl->fCreateSync(LOCAL_EGL_SYNC_NATIVE_FENCE_ANDROID, attribs);
  close(mSyncFd);
  mSyncFd = -1;

It closes the mSyncFd after handing it to fCreateSync, whether the latter succeeds or not.

However, the spec says that the FD is owned by EGL afterwards.
So EGL is responsible for closing it, not the application, and the FD should not be closed.

The other uses of fCreateSync with an FD (e.g. SharedSurfaceAndroidHardwareBuffer) do avoid the close:

  auto rawFD = fenceFd.TakePlatformHandle();
  const EGLint attribs[] = {LOCAL_EGL_SYNC_NATIVE_FENCE_FD_ANDROID, rawFD.get(),
                            LOCAL_EGL_NONE};

  EGLSync sync = egl->fCreateSync(LOCAL_EGL_SYNC_NATIVE_FENCE_ANDROID, attribs);
  if (!sync) {
    gfxCriticalNote << "Failed to create EGLSync from fd";
    return;
  }
  // Release fd here, since it is owned by EGLSync
  Unused << rawFD.release();

The code of DMABufSurface should also be ported to use UniqueFileHandle to manage FDs more safely, not raw ints and close.

Comment 1

[Robert Mader [:rmader]]

Interesting find! This is Martins field of expertise though.
Martin, does this sound reasonable to you?

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Yes, the UniqueFileHandle transition would be great.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Looks like DMABufSurface::FenceWait() is not longer used which is strange.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1743632 [Wayland] Don't release eglCreateSync FD when it's owned by GL, r?emilio

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1743632 [Linux] Polish GLFence management of DMABufSurface, r?emilio

• Remove FenceImportFromFd() and import fence in DMABufSurface::FenceWait().
• Don't close Fence FD when fence import was sucessfull.
• Don't save EGLSync to DMABufSurface after eglClientWaitSync().

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1743632 [Linux] Use GL Fence in SharedSurfaceDMABUF r?sotaro

• Implement SharedSurface_DMABUF::WaitForBufferOwnership() by DMABufSurface::FenceWait().

Depends on D133901

Comment 8

[Martin Stránský [:stransky] (ni? me)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d7f637bfa29ce46e1fc2ec0385b0ef238fbe106

Comment 9

[Martin Stránský [:stransky] (ni? me)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9e6292cfb29ba5687ccca2984c8a2605217dfd5

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/175b3cfd1fd3
[Linux] Polish GLFence management of DMABufSurface, r=emilio
https://hg.mozilla.org/integration/autoland/rev/1023d9d498f4
[Linux] Use GL Fence in SharedSurfaceDMABUF

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/175b3cfd1fd3
https://hg.mozilla.org/mozilla-central/rev/1023d9d498f4

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:stransky, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

No need to uplift. DMABuf surface was not used before this patch.