Am 03.04.2016 um 02:31 schrieb Rob Clark: > 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*? 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 <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