Got it, and I will follow this rule ;) Thanks, Randy
> -----Original Message----- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Thursday, May 11, 2017 8:26 PM > To: Xu, Randy <randy...@intel.com> > Cc: mesa-dev@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...@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@lists.freedesktop.org > >> Cc: Xu, Randy <randy...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
