[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:44:01 UTC 2016
Am 03.04.2016 um 02:31 schrieb Rob Clark:
> 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*?
True enough. Nevertheless I think the potential for confusion is there.
>
> 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.
Yes, ripping out the old interface sounds like a great idea - I don't
mind if the new fence_finish function has the same name as the old one,
provided you're going to eliminate the old one shortly.
Roland
>
>> 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.
>
> 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