[Mesa-dev] [PATCH] i965: Resolve framebuffers before signaling the fence

[Chad Versace]

On Mon 12 Jun 2017, Chris Wilson wrote:
> From KHR_fence_sync:
> 
>   When the condition of the sync object is satisfied by the fence
>   command, the sync is signaled by the associated client API context,
>   causing any eglClientWaitSyncKHR commands (see below) blocking on
>   <sync> to unblock. The only condition currently supported is
>   EGL_SYNC_PRIOR_COMMANDS_COMPLETE_KHR, which is satisfied by
>   completion of the fence command corresponding to the sync object,
>   and all preceding commands in the associated client API context's
>   command stream. The sync object will not be signaled until all
>   effects from these commands on the client API's internal and
>   framebuffer state are fully realized. No other state is affected by
>   execution of the fence command.
> 
> If clients are passing the fence fd (from EGL_ANDROID_native_fence_sync)
> to a compositor, that fence must only be signaled once the framebuffer
> is resolved and not before as is currently the case.

The intent is correct, but some details are wrong. Comments below.

> Reported-by: Sergi Granell <xerpi.g.12 at gmail.com>
> Fixes: c636284ee8ee ("i965/sync: Implement DRI2_Fence extension")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Sergi Granell <xerpi.g.12 at gmail.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Chad Versace <chadversary at chromium.org>
> Cc: Daniel Stone <daniels at collabora.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_sync.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_sync.c b/src/mesa/drivers/dri/i965/brw_sync.c
> index 427108610d..ec4836ce07 100644
> --- a/src/mesa/drivers/dri/i965/brw_sync.c
> +++ b/src/mesa/drivers/dri/i965/brw_sync.c
> @@ -110,6 +110,34 @@ brw_fence_finish(struct brw_fence *fence)
>  static bool MUST_CHECK
>  brw_fence_insert_locked(struct brw_context *brw, struct brw_fence *fence)
>  {
> +   __DRIcontext *driContext = brw->driContext;
> +   __DRIdrawable *driDrawable = driContext->driDrawablePriv;
> +
> +   /*
> +    * From KHR_fence_sync:
> +    *
> +    *   When the condition of the sync object is satisfied by the fence
> +    *   command, the sync is signaled by the associated client API context,
> +    *   causing any eglClientWaitSyncKHR commands (see below) blocking on
> +    *   <sync> to unblock. The only condition currently supported is
> +    *   EGL_SYNC_PRIOR_COMMANDS_COMPLETE_KHR, which is satisfied by
> +    *   completion of the fence command corresponding to the sync object,
> +    *   and all preceding commands in the associated client API context's
> +    *   command stream. The sync object will not be signaled until all
> +    *   effects from these commands on the client API's internal and
> +    *   framebuffer state are fully realized. No other state is affected by
> +    *   execution of the fence command.
> +    *
> +    * Note the emphasis there on ensuring that the framebuffer is fully
> +    * realised before the fence is signal. We cannot just flush the batch,
> +    * but must also resolve the drawable first. The importance of this is,
> +    * for example, in creating a fence for a frame to be passed to a
> +    * remote compositor. Otherwise, the resolve will be in a following batch
> +    * (when the client finally calls SwapBuffers, or forces a resolve via
> +    * some other path) and the compositor may read the fraembuffer beforehand.
> +    */
> +   if (driDrawable)
> +      intel_resolve_for_dri2_flush(brw, driDrawable);

This hunk looks good. Nice find.

>     brw_emit_mi_flush(brw);
>  
>     switch (fence->type) {
> @@ -335,6 +363,8 @@ brw_gl_fence_sync(struct gl_context *ctx, struct gl_sync_object *_sync,
>     struct brw_context *brw = brw_context(ctx);
>     struct brw_gl_sync *sync = (struct brw_gl_sync *) _sync;
>  
> +   assert(condition == EGL_SYNC_PRIOR_COMMANDS_COMPLETE_KHR);
> +

This function is the entrypoint for glFenceSync;
brw_dri_client_wait_sync() is the entry point for eglClientWaitSync. So
the assertion should be on GL_SYNC_GPU_COMMANDS_COMPLETE. For the
record, GL_SYNC_GPU_COMMANDS_COMPLETE != EGL_SYNC_PRIOR_COMMANDS_COMPLETE.

In fact, this assertion should probably go into a separate patch, as
this codepath is unrelated to EGL_KHR_fence_sync.

Fix the assertion; and optionally move the assertion to a separate
patch; and then this will be
Reviewed-by: Chad Versace <chadversary at chromium.org>