Am 03.04.2016 um 17:05 schrieb Rob Clark: > On Sat, Apr 2, 2016 at 8:31 PM, Rob Clark <robdcl...@gmail.com> wrote: >> 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. > > 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 <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