[Mesa-dev] [PATCH 4/6] gallium: add way for drivers to create fences without flushing

[Roland Scheidegger]

I don't really have anything against the new interface (though don't
really feel qualified for a proper review), but it is indeed the
existence of the two interfaces living in parallel seemingly doing
mostly the same which bothers me. To make things worse, the per context
fence_finish is even called the same, with all the same parameters,
which looks like a disaster waiting to happen (you could probably use
the wrong one and it might just work on a lucky day...). Maybe you could
at least rename it to fence_flush_finish or something.
But ideally I think indeed if you could rip out the old interface and
replace with the new one that would solve my issues with this. Ideally
it should be straightforward to do so...
FWIW some documentation int the gallium docs section wouldn't hurt
neither (yes it's not like the existing one has much there...).


Roland


Am 02.04.2016 um 05:01 schrieb Rob Clark:
> I did toy with the idea of adding a
> DONT_REALLY_FLUSH_JUST_CREATE_A_FENCE flag to existing pctx->flush()..
> I'm not really sure I could call that better.  And either way, we want
> the ctx ptr in fence_finish(), otherwise we are hiding the ctx ptr
> internally in the driver's pipe_fence struct so that it can
> potentially flush in screen->fence_finish() (and that doesn't make the
> threading explicit.. when the fence_finish() is associated w/ the ctx
> it is clear that it might have to do ctx related stuff).
> 
> I'm open to better suggestions if anyone has a better idea.  But right
> now this, plus maybe eventually ripping out the old way, seems like
> the best plan.
> 
> BR,
> -R
> 
> On Fri, Apr 1, 2016 at 8:03 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> I'll admit I'm not an expert on this but I got a bad feeling on it.
>> Do you really need another per-context fence_finish function?
>> This looks to me like rather than improving the existing api, it throws
>> another one at the same problem, which is to be sort of used in parallel
>> making things confusing as hell.
>>
>> Roland
>>
>> Am 01.04.2016 um 22:29 schrieb Rob Clark:
>>> From: Rob Clark <robclark at freedesktop.org>
>>>
>>> Since current thing is kinda horrible for tilers.  And that issue will
>>> be even worse with EGL_ANDROID_native_fence_sync.
>>>
>>> Not wired up yet for gl syncobj, which can come later.  For now we just
>>> need this with EGL.
>>>
>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>> ---
>>>  src/gallium/include/pipe/p_context.h  | 24 ++++++++++++++++++++++++
>>>  src/gallium/state_trackers/dri/dri2.c | 29 ++++++++++++++++++++---------
>>>  2 files changed, 44 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>>> index 1c97e82..02a946b 100644
>>> --- a/src/gallium/include/pipe/p_context.h
>>> +++ b/src/gallium/include/pipe/p_context.h
>>> @@ -457,6 +457,30 @@ struct pipe_context {
>>>                   unsigned flags);
>>>
>>>     /**
>>> +    * Create a fence without necessarily flushing rendering.  Note
>>> +    * that if the driver implements this, it must also implement
>>> +    * ctx->fence_finish() which will be used instead of
>>> +    * screen->fence_finish() to give the driver an opportunity to
>>> +    * flush.
>>> +    *
>>> +    * This allows drivers, in particular tilers, to defer flush
>>> +    * until someone actually wants to wait on a fence.
>>> +    *
>>> +    * \param fence  if not NULL, an old fence to unref and transfer a
>>> +    *    new fence reference to
>>> +    */
>>> +   void (*create_fence)(struct pipe_context *pipe,
>>> +                        struct pipe_fence_handle **fence);
>>> +
>>> +   /**
>>> +    * Wait for the fence to finish.
>>> +    * \param timeout in nanoseconds (may be PIPE_TIMEOUT_INFINITE).
>>> +    */
>>> +   boolean (*fence_finish)(struct pipe_context *pipe,
>>> +                           struct pipe_fence_handle *fence,
>>> +                           uint64_t timeout);
>>> +
>>> +   /**
>>>      * Create a view on a texture to be used by a shader stage.
>>>      */
>>>     struct pipe_sampler_view * (*create_sampler_view)(struct pipe_context *ctx,
>>> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
>>> index fb0a180..b66d885 100644
>>> --- a/src/gallium/state_trackers/dri/dri2.c
>>> +++ b/src/gallium/state_trackers/dri/dri2.c
>>> @@ -1320,7 +1320,12 @@ dri2_create_fence(__DRIcontext *_ctx)
>>>     if (!fence)
>>>        return NULL;
>>>
>>> -   ctx->flush(ctx, &fence->pipe_fence, 0);
>>> +   if (ctx->create_fence) {
>>> +      debug_assert(ctx->fence_finish);
>>> +      ctx->create_fence(ctx, &fence->pipe_fence);
>>> +   } else {
>>> +      ctx->flush(ctx, &fence->pipe_fence, 0);
>>> +   }
>>>
>>>     if (!fence->pipe_fence) {
>>>        FREE(fence);
>>> @@ -1376,27 +1381,33 @@ static GLboolean
>>>  dri2_client_wait_sync(__DRIcontext *_ctx, void *_fence, unsigned flags,
>>>                        uint64_t timeout)
>>>  {
>>> +   struct pipe_context *ctx = dri_context(_ctx)->st->pipe;
>>>     struct dri2_fence *fence = (struct dri2_fence*)_fence;
>>>     struct dri_screen *driscreen = fence->driscreen;
>>>     struct pipe_screen *screen = driscreen->base.screen;
>>> +   struct pipe_fence_handle *pipe_fence = NULL;
>>>
>>> -   /* No need to flush. The context was flushed when the fence was created. */
>>> +   /* No need to flush. The context was flushed when the fence was created,
>>> +    * or the ctx implements ctx->fence_finish() which will take care of
>>> +    * flushing if required
>>> +    */
>>>
>>>     if (fence->pipe_fence)
>>> -      return screen->fence_finish(screen, fence->pipe_fence, timeout);
>>> +      pipe_fence = fence->pipe_fence;
>>>     else if (fence->cl_event) {
>>> -      struct pipe_fence_handle *pipe_fence =
>>> -         driscreen->opencl_dri_event_get_fence(fence->cl_event);
>>> -
>>> -      if (pipe_fence)
>>> -         return screen->fence_finish(screen, pipe_fence, timeout);
>>> -      else
>>> +      pipe_fence = driscreen->opencl_dri_event_get_fence(fence->cl_event);
>>> +      if (!pipe_fence)
>>>           return driscreen->opencl_dri_event_wait(fence->cl_event, timeout);
>>>     }
>>>     else {
>>>        assert(0);
>>>        return false;
>>>     }
>>> +
>>> +   if (ctx->fence_finish)
>>> +      return ctx->fence_finish(ctx, pipe_fence, timeout);
>>> +
>>> +   return screen->fence_finish(screen, pipe_fence, timeout);
>>>  }
>>>
>>>  static void
>>>
>>