On 2017-11-03 05:17 AM, Nicolai Hähnle wrote:
On 02.11.2017 04:57, Andres Rodriguez wrote:
Bits to implement ServerWaitSemaphoreObject/ServerSignalSemaphoreObject
Signed-off-by: Andres Rodriguez <andre...@gmail.com>
---
src/mesa/state_tracker/st_cb_semaphoreobjects.c | 28
+++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/src/mesa/state_tracker/st_cb_semaphoreobjects.c
b/src/mesa/state_tracker/st_cb_semaphoreobjects.c
index 47ece47..4cff3fd 100644
--- a/src/mesa/state_tracker/st_cb_semaphoreobjects.c
+++ b/src/mesa/state_tracker/st_cb_semaphoreobjects.c
@@ -1,5 +1,7 @@
+
#include "main/imports.h"
#include "main/mtypes.h"
+#include "main/context.h"
#include "main/externalobjects.h"
@@ -47,10 +49,36 @@ st_import_semaphoreobj_fd(struct gl_context *ctx,
#endif
}
+static void
+st_server_wait_semaphore(struct gl_context *ctx,
+ struct gl_semaphore_object *semObj)
+{
+ struct st_semaphore_object *st_obj = st_semaphore_object(semObj);
+ struct st_context *st = st_context(ctx);
+ struct pipe_context *pipe = st->pipe;
+
+ _mesa_flush(ctx);
What's the justification of this flush?
Thanks for all the review feedback :)
Technically according to the spec we shouldn't need to flush here. But
the flush is required due to how amdgpu handles syncobjs.
For amdgpu, the wait and signal operations happens on a submission
boundary. I.e, we can say "wait for this syncobj before executing this
submission", or "signal this syncobj after this submission completes".
But we can't really introduce a wait/signal in the middle of a
commandstream as the spec requires us to do.
This flush (and the one in st_server_signal_semaphore) exist to adjust
the submission boundaries. If we align the submission boundary with the
semaphore operation boundary we can accurately emulate the timing of the
semaphore operations.
In this case, when st_server_wait_semaphore() is called, the user
expects that any work queued up until this point will not wait for the
semaphore. Any work submitted after the wait call returns should wait
for the semaphore to signal. Hence, we call flush so that the amdgpu
winsys will submit all the current work. Then we queue up the syncobj
for future submissions to wait on.
In the signal case, we flush so that the syncobj will signal when the
current work finishes (instead of at "current work + N"). That way we
don't have any unnecessary latency.
However, I'm not 100% certain my approach here is the best solution. My
main concern is that an amdgpu specific detail is surfaced in what
should be a pipe agnostic layer. If you have any recommendations on how
to move this code into the pipe layer, or achieve the same results with
a different strategy I'd love to take the advice.
From the performance point of view, I'm not as concerned. Wait/signal
tend to be aligned at frame boundaries so flushing there shouldn't be a
big deal. But I do think that improving the code structure from my
current iteration would have value.
+ pipe->semobj_wait(pipe, st_obj->semaphore);
+}
+
+static void
+st_server_signal_semaphore(struct gl_context *ctx,
+ struct gl_semaphore_object *semObj)
+{
+ struct st_semaphore_object *st_obj = st_semaphore_object(semObj);
+ struct st_context *st = st_context(ctx);
+ struct pipe_context *pipe = st->pipe;
+
You have to flush state-tracker internal state here. This means
FLUSH_VERTICES, st_flush_bitmap, possibly others. I don't think we have
this factored out well yet, but see below.
Thanks for pointing this out, this actually gives me an idea for getting
rid of the full flushes, see below.
+ pipe->semobj_signal(pipe, st_obj->semaphore);
+ _mesa_flush(ctx);
What's the justification of this flush? Does EXT_external_objects
contain any language to guarantee that SignalSemaphoreEXT will "finish
in finite time"? If no, the flush should probably not be there. If yes,
please add a spec quote.
See above for the flush reasoning.
Furthermore, this whole code section is a strong hint that merging
fences and semaphores at the Gallium level is indeed a good idea.
Basically, the idea is that you'd end up calling
pipe->flush(pipe, &st_obj->semaphore, flags);
where flags is PIPE_FLUSH_DEFERRED | PIPE_FLUSH_REUSE_FENCE or
PIPE_FLUSH_ASYNC | PIPE_FLUSH_REUSE_FENCE, depending on how the previous
paragraph is resolved. In one of my recent series that includes deferred
fences for threaded contexts, I'm already doing the equivalent of adding
a "PIPE_FLUSH_REUSE_FENCE". We could formalize this as a more broadly
applicable feature in the Gallium interface.
Thanks for pointing out PIPE_FLUSH_REUSE_FENCE, I wasn't aware of this flag.
My reasoning for keeping semaphore operations separate is that a
different pipe implementation might be able to emit a wait/signal packet
into the middle of the commandstream, with no flushing required.
However, that argument has a giant hole in it. I have a _mesa_flush()
which completely negates any benefits of a "pipe agnostic"
implementation. So in this case, as you suggested, I think merging
semaphores with re-usable fences would be a good idea.
I do have one alternative proposal try to keep the option of
mid-commandstream semaphores open. How about something like:
flush_state_tracker_internal_state() {
flush_vertices();
flush_bitmaps();
}
st_server_wait_semaphore() {
flush_state_tracker_internal_state();
pipe->semobj_wait(pipe, st_obj->semaphore);
// The amdgpu semobj_wait() implementation would
// then be responsible for adjusting the submission
// windows internally, i.e. calling flush()
}
st_server_signal_semaphore() {
flush_state_tracker_internal_state();
pipe->semobj_signal(pipe, st_obj->semaphore);
// The amdgpu semobj_signal() implementation would
// then be responsible for adjusting the submission
// windows internally, i.e. calling flush()
}
Let me know what you think of this slightly more agnostic implementation.
Sorry for the long reply!
Regards,
Andres
Cheers,
Nicolai
+}
+
void
st_init_semaphoreobject_functions(struct dd_function_table *functions)
{
functions->NewSemaphoreObject = st_semaphoreobj_alloc;
functions->DeleteSemaphoreObject = st_semaphoreobj_free;
functions->ImportSemaphoreFd = st_import_semaphoreobj_fd;
+ functions->ServerWaitSemaphoreObject = st_server_wait_semaphore;
+ functions->ServerSignalSemaphoreObject = st_server_signal_semaphore;
}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
