Hi Zhongmin,

On Wed, Jul 19, 2017 at 3:16 PM, Wu, Zhongmin <zhongmin...@intel.com> wrote:
> Hi  Tomasz:
>
> Thanks very much for your comments, I read it carefully, but still have some 
> questions below. Could you please have a look. Thanks again.
>
> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of 
> Tomasz Figa
> Sent: Wednesday, July 19, 2017 12:42
> To: Wu, Zhongmin <zhongmin...@intel.com>
> Cc: Gao, Shuo <shuo....@intel.com>; Liu, Zhiquan <zhiquan....@intel.com>; 
> Daniel Stone <dani...@collabora.com>; Antognolli, Rafael 
> <rafael.antogno...@intel.com>; Emil Velikov <emil.l.veli...@gmail.com>; Chad 
> Versace <chadvers...@chromium.org>; Eric Engestrom <e...@engestrom.ch>; 
> Marathe, Yogesh <yogesh.mara...@intel.com>; Kenneth Graunke 
> <kenn...@whitecape.org>; Kondapally, Kalyan <kalyan.kondapa...@intel.com>; 
> Rainer Hochecker <fernetme...@online.de>; ML mesa-dev 
> <mesa-dev@lists.freedesktop.org>; Nicolai Hähnle <nicolai.haeh...@amd.com>; 
> Varad Gautam <varad.gau...@collabora.com>
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
> Queue the buffer with a sync fence for Android OS (v3.1)
>
> Hi Zhongmin,
>
> Thanks for the patch. Please see my comments inline.
>
> On Wed, Jul 19, 2017 at 12:22 PM, Zhongmin Wu <zhongmin...@intel.com> wrote:
>> Before we queued the buffer with a invalid fence (-1), it will make
>> some benchmarks failed to test such as flatland.
>>
>> Now we get the out fence during the flushing buffer and then pass it
>> to SurfaceFlinger in eglSwapbuffer function.
>>
>> v2: a) Also implement the fence in cancelBuffer.
>>     b) The last sync fence is stored in drawable object
>>        rather than brw context.
>>     c) format clear.
>>
>> v3: a) Save the last fence fd in DRI Context object.
>>     b) Return the last fence if the batch buffer is empty and
>>        nothing to be flushed when _intel_batchbuffer_flush_fence
>>     c) Add the new interface in vbtl to set the retrieve fence
>>        in the egl surface. This is just for cancelBuffer.
>>     d) For queueBuffer, the fence is get with DRI fence interface.
>>        For cancelBuffer, the fence is get before the context is
>>        reset, and the fence is then  moved to its surface.
>> v3.1 a) close fd in the new vbtl interface on none Android platform
>>
>> Change-Id: Ic0773c19788d612a98d1402f5b5619dab64c1bc2
>> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-43936
>> Reported-On: https://bugs.freedesktop.org/show_bug.cgi?id=101655
>> Signed-off-by: Zhongmin Wu <zhongmin...@intel.com>
>> Reported-by: Li, Guangli <guangli...@intel.com>
>> Tested-by: Marathe, Yogesh <yogesh.mara...@intel.com>
>> ---
>>  src/egl/drivers/dri2/egl_dri2.c               |   17 +++++++++++++-
>>  src/egl/drivers/dri2/egl_dri2.h               |    3 +++
>>  src/egl/drivers/dri2/platform_android.c       |   30 
>> ++++++++++++++++++++++---
>>  src/egl/drivers/dri2/platform_drm.c           |    1 +
>>  src/egl/drivers/dri2/platform_surfaceless.c   |    1 +
>>  src/egl/drivers/dri2/platform_wayland.c       |    2 ++
>>  src/egl/drivers/dri2/platform_x11.c           |    2 ++
>>  src/egl/drivers/dri2/platform_x11_dri3.c      |    1 +
>>  src/mesa/drivers/dri/common/dri_util.c        |    3 +++
>>  src/mesa/drivers/dri/common/dri_util.h        |    2 ++
>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   25 ++++++++++++++++++---
>>  11 files changed, 80 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c
>> b/src/egl/drivers/dri2/egl_dri2.c index 020a0bc..19d346d 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -1351,9 +1351,17 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
>> _EGLSurface *dsurf,
>>     ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) : NULL;
>>     rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
>>     cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
>> -
>> +   int fence_fd = -1;
>>     if (old_ctx) {
>>        __DRIcontext *old_cctx =
>> dri2_egl_context(old_ctx)->dri_context;
>> +      if (old_dsurf) {
>> +         void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
>> +         if (fence) {
>> +            fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, 
>> fence);
>> +            dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
>> +         }
>> +         dri2_dpy->vtbl->set_retrieve_fence(old_dsurf, fence_fd);
>
> I think we don't need this callback. We can just set the fence in old_dsurf 
> directly, i.e.
>
> dri2_surf_set_release_fence(surf, fence_fd) {
>    if (surf->fence_fd >= 0)
>       close(surf->fence_fd);
>    surf->fence_fd = fence_fd;
> }
>
> // ...
>
> dri2_make_current()
> {
> // ...
>       int fence_fd = -1;
>       if (old_dsurf) {
>          void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
>          if (fence) {
>             fence_fd =
> dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, fence);
>             dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
>          }
>          dri2_surf_set_release_fence(old_dsurf, fence_fd);
>       }
> // ...
> }
>
> wzm :================== 
> ============================================================================================================================

Please don't add such delimiters to replies, they really make it
almost impossible to read your messages. The normal quoting sign is
enough to distinguish between quoted and new text...

> I am a little confused here, you mean we can add the fd field in _EGLSurface 
> directly ? I checked the definition of _EGLSurface, it is not like a place to 
> add the extra member.
> ==================================================================================================================================================

My suggestion was to add it to the dri2_egl_surface associated with
given _EGLSurface.

>
> Then we can just call dri2_surf_set_release_fence(surf, -1) in
> dri2_destroy_surface() after the platform callback returns. The platform 
> callback must set surf->fence_fd to -1 if ownership of the FD was given to 
> the platform window system.
> wzm 
> :===============================================================================================================================================
> But we can't call dri2_surf_set_release_fence(surf, -1) after platforms 
> callback return, the surf may be freed in callback.

You're right. In this case having an dri2_init_surface() that
initializes the _EGLSurface and sets the FD to -1 (mentioned in my
earlier comment) and dri2_destroy_surface() that closes it if it's >=
0 and sets to -1, would make even more sense.

> ====================================================================================================================================================
> wzm: 
> ===============================================================================================================================================
> In this patch, I just add the fd member in dri2_egl_surface for Android 
> platform, and Only Android platform will initialize/de-initialize the fd when 
> create and destroy the buffer.
> If we move the fd member to _EGLSurface, it will be seen on all platforms, 
> right ?  We do need a good place to initialize/de-initialize the fd.
> And none Android platform will not use such fence actually.

Please see my comments above. I think we might be able to have some
way for the platform to notify the egl_dri2 core that it's interested
in fences. As it was mentioned in previous messages, fences are not an
Android-specific feature anymore and other platforms are going to use
them as well.

> ====================================================================================================================================================
>
>> +      }
>>        dri2_dpy->core->unbindContext(old_cctx);
>>     }
>>
>> @@ -1403,6 +1411,13 @@ dri2_surface_get_dri_drawable(_EGLSurface *surf)
>>     return dri2_surf->dri_drawable;
>>  }
>>
>> +void
>> +dri2_set_retrieve_fence(_EGLSurface *surf, int fd) {
>> +       close(fd);
>> +       return;
>> +}
>> +
>>  /*
>>   * Called from eglGetProcAddress() via drv->API.GetProcAddress().
>>   */
>> diff --git a/src/egl/drivers/dri2/egl_dri2.h
>> b/src/egl/drivers/dri2/egl_dri2.h index bbba7c0..b097345 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.h
>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>> @@ -150,6 +150,8 @@ struct dri2_egl_display_vtbl {
>>                                   EGLuint64KHR *sbc);
>>
>>     __DRIdrawable *(*get_dri_drawable)(_EGLSurface *surf);
>> +
>> +   void (*set_retrieve_fence)(_EGLSurface *surf, int fd);
>>  };
>>
>>  struct dri2_egl_display
>> @@ -314,6 +316,7 @@ struct dri2_egl_surface
>>        struct ANativeWindowBuffer *buffer;
>>        int age;
>>     } color_buffers[3], *back;
>> +   int retrieve_fd;
>>  #endif
>>
>>  #if defined(HAVE_SURFACELESS_PLATFORM)
>> diff --git a/src/egl/drivers/dri2/platform_android.c
>> b/src/egl/drivers/dri2/platform_android.c
>> index cc2e4a6..b8d53b8 100644
>> --- a/src/egl/drivers/dri2/platform_android.c
>> +++ b/src/egl/drivers/dri2/platform_android.c
>> @@ -306,9 +306,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
>> dri2_egl_surface *dri2_sur
>>      *    is responsible for closing it.
>>      */
>>     int fence_fd = -1;
>> +   _EGLContext *ctx = _eglGetCurrentContext();
>> +   struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
>> +   void * fence = dri2_dpy->fence->create_fence_fd(dri2_ctx->dri_context, 
>> -1);
>> +   if (fence) {
>> +      fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
>> +                                               fence);
>> +      dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
>> +   }
>
> If we are already handling the make current case in the generic egl_dri2, we 
> can do the same with swap_buffers IMHO and have the fence always stored in 
> dri2_surf->fence_fd.
>
>>     dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
>>                                    fence_fd);
>> -
>
> No need to remove this line in this patch.
>
>>     dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
>>     dri2_surf->buffer = NULL;
>>     dri2_surf->back = NULL;
>> @@ -327,8 +334,10 @@ static void
>>  droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)  {
>>     int ret;
>> -
>> -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, 
>> dri2_surf->buffer, -1);
>> +   int fence_fd = -1;
>> +   fence_fd = dup(dri2_surf->retrieve_fd);
>> +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
>> +                                         dri2_surf->buffer,
>> + fence_fd);
>
> The surface is being destroyed here. I don't think we need to dup() the FD. 
> Instead we should simply set it to -1.
> wzm: 
> =============================================================================================================================================
> That is right, we can do this
> And what about queuebuffer , if we apply the same style, can we set the fd to 
> -1 in queue buffer ? In fact, I am not clear about all the possible sequences 
> of such  functions swapbuffer,
> Flush, make current, and so on. So I used the  most safe way without prior 
> knowledge ...

swapbuffer gives the ownership for the buffer to the native window
system and it's free to do anything it wants with the buffer, so the
fence on Mesa side wouldn't have any meaning anymore. So it doesn't
make sense to keep it anymore.

> ==================================================================================================================================================
>>     if (ret < 0) {
>>        _eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
>>        dri2_surf->base.Lost = EGL_TRUE; @@ -434,6 +443,8 @@
>> droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>>        dri2_surf->window = window;
>>     }
>>
>> +   dri2_surf->retrieve_fd = -1;
>> +
>
> nit: Maybe we should think about having a dri2_init_surface() helper that 
> calls _eglInitSurface() and sets some dri2 specific fields for us as well?
> Wzm 
> ===========================================================================================================================
> A little confused again,  if we add the fd member in _EGLSurface as above,  
> we can initialize  fd to -1 in _eglInitSurface() directly.  Every surface 
> creation on each platform will call
> _eglInitSurface(), right?  The trouble thing is where to de-initialize as we 
> don’t have _eglDeInitSurface() in each platform's destroy path....

See my comments above again. My suggestions were for egl_dri2_surface,
not _EGLSurface.

> ================================================================================================================================
>>     return &dri2_surf->base;
>>
>>  cleanup_surface:
>> @@ -488,6 +499,9 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay
>> *disp, _EGLSurface *surf)
>>
>>     dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
>>
>> +   if (dri2_surf->retrieve_fd != -1)
>> +      close(dri2_surf->retrieve_fd);
>> +
>
> I think this would make more sense in dri2_destroy_surface().
>
>>     free(dri2_surf);
>>
>>     return EGL_TRUE;
>> @@ -1073,6 +1087,15 @@ droid_create_image_khr(_EGLDriver *drv,
>> _EGLDisplay *disp,  }
>>
>>  static void
>> +droid_set_retrieve_fence(_EGLSurface *surf, int fd) {
>> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>> +   if (dri2_surf->retrieve_fd != -1)
>> +      close(dri2_surf->retrieve_fd);
>> +   dri2_surf->retrieve_fd = fd;
>> +}
>
> Shouldn't need this with my earlier suggestion.
>
>> +
>> +static void
>>  droid_flush_front_buffer(__DRIdrawable * driDrawable, void
>> *loaderPrivate)  {  } @@ -1289,6 +1312,7 @@ static const struct
>> dri2_egl_display_vtbl droid_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = droid_set_retrieve_fence,
>>  };
>>
>>  static const __DRIdri2LoaderExtension droid_dri2_loader_extension = {
>> diff --git a/src/egl/drivers/dri2/platform_drm.c
>> b/src/egl/drivers/dri2/platform_drm.c
>> index 8b0562c..97f897e 100644
>> --- a/src/egl/drivers/dri2/platform_drm.c
>> +++ b/src/egl/drivers/dri2/platform_drm.c
>> @@ -665,6 +665,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_drm_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  EGLBoolean
>> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c
>> b/src/egl/drivers/dri2/platform_surfaceless.c
>> index 0eb3fb7..78b2571 100644
>> --- a/src/egl/drivers/dri2/platform_surfaceless.c
>> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
>> @@ -244,6 +244,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_surfaceless_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  static void
>> diff --git a/src/egl/drivers/dri2/platform_wayland.c
>> b/src/egl/drivers/dri2/platform_wayland.c
>> index f746f0b..458bd6e 100644
>> --- a/src/egl/drivers/dri2/platform_wayland.c
>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>> @@ -1087,6 +1087,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_wl_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_wl_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  static const __DRIextension *dri2_loader_extensions[] = { @@ -1781,6
>> +1782,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_wl_swrast_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  static const __DRIswrastLoaderExtension swrast_loader_extension = {
>> diff --git a/src/egl/drivers/dri2/platform_x11.c
>> b/src/egl/drivers/dri2/platform_x11.c
>> index 74d3a16..0def3ca 100644
>> --- a/src/egl/drivers/dri2/platform_x11.c
>> +++ b/src/egl/drivers/dri2/platform_x11.c
>> @@ -1149,6 +1149,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_x11_swrast_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_fallback_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  static const struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
>> @@ -1170,6 +1171,7 @@ static const struct dri2_egl_display_vtbl 
>> dri2_x11_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri2_x11_get_sync_values,
>>     .get_dri_drawable = dri2_surface_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  static const __DRIswrastLoaderExtension swrast_loader_extension = {
>> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c
>> b/src/egl/drivers/dri2/platform_x11_dri3.c
>> index 3148f49..cae322d 100644
>> --- a/src/egl/drivers/dri2/platform_x11_dri3.c
>> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
>> @@ -466,6 +466,7 @@ struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
>>     .create_wayland_buffer_from_image = 
>> dri2_fallback_create_wayland_buffer_from_image,
>>     .get_sync_values = dri3_get_sync_values,
>>     .get_dri_drawable = dri3_get_dri_drawable,
>> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>>  };
>>
>>  EGLBoolean
>> diff --git a/src/mesa/drivers/dri/common/dri_util.c
>> b/src/mesa/drivers/dri/common/dri_util.c
>> index f6df488..d0465b8 100644
>> --- a/src/mesa/drivers/dri/common/dri_util.c
>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>> @@ -446,6 +446,7 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
>>      context->driScreenPriv = screen;
>>      context->driDrawablePriv = NULL;
>>      context->driReadablePriv = NULL;
>> +    context->retrieve_fd = -1;
>>
>>      if (!screen->driver->CreateContext(mesa_api, modes, context,
>>                                         major_version, minor_version,
>> @@ -499,6 +500,8 @@ static void  driDestroyContext(__DRIcontext *pcp)
>> {
>>      if (pcp) {
>> +    if (pcp->retrieve_fd != -1)
>> +       close(pcp->retrieve_fd);
>>         pcp->driScreenPriv->driver->DestroyContext(pcp);
>>         free(pcp);
>>      }
>> diff --git a/src/mesa/drivers/dri/common/dri_util.h
>> b/src/mesa/drivers/dri/common/dri_util.h
>> index 8fcd632..f723a90 100644
>> --- a/src/mesa/drivers/dri/common/dri_util.h
>> +++ b/src/mesa/drivers/dri/common/dri_util.h
>> @@ -218,6 +218,8 @@ struct __DRIcontextRec {
>>         int draw_stamp;
>>         int read_stamp;
>>      } dri2;
>> +
>> +    int retrieve_fd;
>>  };
>
> Do we need to store this in a generic DRI struct, if it's only used by i965? 
> IMHO it would make more sense to just store this in brw_context.
>
> Also, let's use some more commonly used naming scheme, for example 
> out_fence_fd.
>
> wzm: =================================
> You are right, we can just use brw_context.
> =====================================
>
>>
>>  /**
>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> index 62d2fe8..77bf8f6 100644
>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> @@ -648,9 +648,25 @@ do_flush_locked(struct brw_context *brw, int 
>> in_fence_fd, int *out_fence_fd)
>>           /* Add the batch itself to the end of the validation list */
>>           add_exec_bo(batch, batch->bo);
>>
>> +         if (brw->driContext &&
>> +            brw->driContext->retrieve_fd != -1) {
>> +            close(brw->driContext->retrieve_fd);
>> +            brw->driContext->retrieve_fd = -1;
>> +         }
>> +
>> +         int fd = -1;
>>           ret = execbuffer(dri_screen->fd, batch, hw_ctx,
>>                            4 * USED_BATCH(*batch),
>> -                          in_fence_fd, out_fence_fd, flags);
>> +                          in_fence_fd, &fd, flags);
>> +
>> +         if (out_fence_fd != NULL) {
>> +            *out_fence_fd = fd;
>> +            if (brw->driContext)
>> +               brw->driContext->retrieve_fd = dup(fd);
>> +         } else {
>> +            if (brw->driContext)
>> +               brw->driContext->retrieve_fd  = fd;
>> +         }
>
> I think this code can be simplified significantly. Including my comments 
> above:
>
>    brw->out_fence_fd = fd;
>    if (out_fence_fd)
>       *out_fence_fd = (fd >= 0) ? dup(fd) : -1;
>
>>        }
>>
>>        throttle(brw);
>> @@ -684,8 +700,11 @@ _intel_batchbuffer_flush_fence(struct brw_context
>> *brw,  {
>>     int ret;
>>
>> -   if (USED_BATCH(brw->batch) == 0)
>> -      return 0;
>> +   if (USED_BATCH(brw->batch) == 0) {
>> +      if (out_fence_fd && brw->driContext->retrieve_fd != -1)
>> +         *out_fence_fd = dup(brw->driContext->retrieve_fd);
>
> We need to check for >= 0 if we only allow valid FDs. Also it should actually 
> make sense to assign -1 if we don't have any fence.
>
>    if (out_fence_fd)
>       *out_fence_fd = (brw->out_fence_fd >= 0) ? dup(brw->out_fence_fd) : -1;
>
>> +         return 0;
>
> Wrong indentation?
>
>> +   }
>>
>>     if (brw->throttle_batch[0] == NULL) {
>>        brw->throttle_batch[0] = brw->batch.bo;
>
> Generally I still believe i965 driver in its current shape inserts commands 
> to the buffer when brw_dri_create_fence_fd() is called, so all the 
> i965-specific changes should go to a separate patch, as an additional 
> optimization.
> wzm:=======================================================================================================
> You are right,  it is better to have a separate and entire optimization patch 
> for  i965 fence logic first...
> ============================================================================================================

Or have the EGL patches first and i965 logic optimization later. It
shouldn't matter, as they don't seem to depend on each other.

Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to