On Sat, Apr 2, 2016 at 7:55 PM, Roland Scheidegger <srol...@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.
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 <srol...@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 <robcl...@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 <robcl...@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
>>>>
>>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
