[Mesa-dev] [PATCH 4/6] gallium: add way for drivers to create fences without flushing
Roland Scheidegger
sroland at vmware.com
Sun Apr 3 23:45:36 UTC 2016
Am 03.04.2016 um 17:05 schrieb Rob Clark:
> 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.
Yes, that is probably the right way to do it. Should also give a lot
more test coverage of the new way of handling things...
Roland
>
> 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