[Mesa-dev] [PATCH] i965: avoid fence fd dup in EGL layer

[Xu, Randy]

Got it, and I will follow this rule ;)

Thanks,
Randy

> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Thursday, May 11, 2017 8:26 PM
> To: Xu, Randy <randy.xu at intel.com>
> Cc: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] i965: avoid fence fd dup in EGL layer
> 
> Hi Randy,
> 
> On 5 May 2017 at 08:17, Xu, Randy <randy.xu at intel.com> wrote:
> > Ping Chad & Emil & Tapani
> >
> > Please help to review it, I just verified it on Intel i965 driver.
> >
> All three of us are already subscribed to the list, so we get the patch ;-)
> 
> Please don't ping immediately after you post a patch - this tends to deterrent
> devs.
> Instead, give it a few days (a week ideally) before pinging/bumping the patch.
> 
> > Thanks,
> > Randy
> >
> >
> >> -----Original Message-----
> >> From: Xu, Randy
> >> Sent: Friday, May 5, 2017 3:15 PM
> >> To: mesa-dev at lists.freedesktop.org
> >> Cc: Xu, Randy <randy.xu at intel.com>
> >> Subject: [PATCH] i965: avoid fence fd dup in EGL layer
> >>
> >> Follow up "i965: Solve Android native fence fd double close"
> >> The _EGLSync.SyncFd is not neccesary to keep after pass to dri driver.
> >>
> >> Test: Run Vulkan and GLES stress test and no crash.
> >> ---
> >>  src/egl/drivers/dri2/egl_dri2.c      | 10 ++++++----
> >>  src/mesa/drivers/dri/i965/brw_sync.c |  2 +-
> >>  2 files changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c
> >> b/src/egl/drivers/dri2/egl_dri2.c index 0be7132..9ef35d3 100644
> >> --- a/src/egl/drivers/dri2/egl_dri2.c
> >> +++ b/src/egl/drivers/dri2/egl_dri2.c
> >> @@ -2637,6 +2637,7 @@ dri2_create_sync(_EGLDriver *drv, _EGLDisplay
> >> *dpy,
> >>           free(dri2_sync);
> >>           return NULL;
> >>        }
> >> +      dri2_sync->base.SyncFd = EGL_NO_NATIVE_FENCE_FD_ANDROID;
> Do we need this? AFAICT the initialization is correctly done in _eglInitSync()
> 
> >>        break;
> >>     }
> >>
> >> @@ -2678,24 +2679,25 @@ dri2_dup_native_fence_fd(_EGLDriver *drv,
> >> _EGLDisplay *dpy, _EGLSync *sync)  {
> >>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> >>     struct dri2_egl_sync *dri2_sync = dri2_egl_sync(sync);
> >> +   EGLint SyncFd = sync->SyncFd;
> >>
> >>     assert(sync->Type == EGL_SYNC_NATIVE_FENCE_ANDROID);
> >>
> >> -   if (sync->SyncFd == EGL_NO_NATIVE_FENCE_FD_ANDROID) {
> >> +   if (SyncFd == EGL_NO_NATIVE_FENCE_FD_ANDROID) {
> >>        /* try to retrieve the actual native fence fd.. if rendering is
> >>         * not flushed this will just return -1, aka NO_NATIVE_FENCE_FD:
> >>         */
> >> -      sync->SyncFd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
> >> +      SyncFd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
> >>                                                     dri2_sync->fence);
> >>     }
> >>
> >> -   if (sync->SyncFd == EGL_NO_NATIVE_FENCE_FD_ANDROID) {
> >> +   if (SyncFd == EGL_NO_NATIVE_FENCE_FD_ANDROID) {
> >>        /* if native fence fd still not created, return an error: */
> >>        _eglError(EGL_BAD_PARAMETER, "eglDupNativeFenceFDANDROID");
> >>        return EGL_NO_NATIVE_FENCE_FD_ANDROID;
> >>     }
> >>
> >> -   return dup(sync->SyncFd);
> >> +   return SyncFd;
> Nice find. This removes a double dup which I did not spot with previously.
> I'd suggest keeping it separate patch and pimping up the commit message:
> 
> Don't call dup() twice when calling eglDupNativeFenceFDANDROID.
> 
> Currently, both libEGL and the DRI module call dup when the function is
> called. Remove the former since it's ultimately a driver decision how to
> manage the file descriptor.
> With follow-up patches we'll unwind the remaining confusion, by directly
> propagating the FD to the driver and keeping no references in libEGL.
> 
> Fixes: 0201f01dc4e ("egl: add EGL_ANDROID_native_fence_sync")
> 
> >>  }
> >>
> >>  static EGLint
> >> diff --git a/src/mesa/drivers/dri/i965/brw_sync.c
> >> b/src/mesa/drivers/dri/i965/brw_sync.c
> >> index a8356c3..5b78503 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_sync.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_sync.c
> >> @@ -470,7 +470,7 @@ brw_dri_create_fence_fd(__DRIcontext *dri_ctx,
> >> int
> >> fd)
> >>           goto fail;
> >>     } else {
> >>        /* Import the sync fd as an in-fence. */
> >> -      fence->sync_fd = dup(fd);
> >> +      fence->sync_fd = fd;
> This is off, I'm afraid. Please re-read my earlier suggestion. An alternative
> explanation is available above - as before, the documentation in
> dri_interface.h should be improved with the final patch of the series.
> 
> Thanks
> Emil