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
