[Mesa-dev] [PATCH 4/6] gallium: add way for drivers to create fences without flushing
Rob Clark
robdclark at gmail.com
Sun Apr 3 15:05:20 UTC 2016
On Sat, Apr 2, 2016 at 8:31 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Apr 2, 2016 at 7:55 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> 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...).
>
> I guess the compiler should at least tell you if you are passing a
> pipe_screen* vs pipe_context*?
>
> But no issues to rename it to be even more explicit if that is preferred.
>
>> 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...
>
> If no one objects, I'd like to rip out the old interface. Maybe not
> all in one patchset.. I'm not a huge fan of flag-days if they are
> avoidable, but I don't mind working on follow up patches to change the
> other drivers and state trackers. I don't have hw to test everything
> but if I can count on others to eventually test things, and then in
> the end push the removal of the old interface, that seems like a sane
> plan to me. But I'd rather not block the native-fence-fd support on
> that.
>
>> FWIW some documentation int the gallium docs section wouldn't hurt
>> neither (yes it's not like the existing one has much there...).
>
> agreed, and the follow-on patches probably need a bit of docs too.. I
> don't mind adding that. I have a bit of work to-do still on
> kernel/libdrm_freedreno/freedreno to get the native-fence-fd stuff
> completely wired up, so I expect at least one or two more iterations
> of the patchset, (although I think barring any bugs that turn up, the
> egl and dri bits should be more or less complete). I'll add some more
> docs for the gallium bits before I send the next round.
btw, one thought.. it should be relatively straight forward to add
some helpers to implement the new fence APIs in terms of the old ones,
and plug those in to all the drivers. This way, state trackers
wouldn't have a transient period of having to deal w/ both old and new
APIs. This way we could update state trackers one-by-one, and once
all the state trackers are converted, and all the drivers converted,
drop the helpers and old APIs.
BR,
-R
> BR,
> -R
>
>>
>> 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
>>>>>
>>>>
>>
More information about the mesa-dev
mailing list