On 2017-11-03 05:15 AM, Nicolai Hähnle wrote:
Hi Andres,
Thank you for doing this. I haven't read the series in detail yet, but I
have a bunch of questions and comments.
Thanks for all the feedback.
I find it very unfortunate to have both fence and semaphore objects in
the Gallium interface. The two even become mixed up in the amdgpu
winsys. My first thought is that it should be possible to improve this
by simply turning *all* Gallium fences into semaphores as far as the
semantics of the Gallium interface are concerned (the underlying
implementation can of course still vary). All the existing uses of
fences basically become semaphores that are create and signaled
immediately by calls to pipe_context::flush.
Agreed with this point. See the discussion on patch 9 for a query on
taking a slightly different approach. But regardless of the outcome I
agree that merging these would be cleaner.
This brings me to the next point: all new related Gallium interfaces
should really be in the same commit. So the first commit should
introduce PIPE_CAP_SEMAPHORE and all new pipe_context/pipe_screen
functions. It should also contain documentation (see context.rst /
screen.rst). Writing up clean documentation for semaphores should also
help with resolving the question of the previous paragraph.
Adding the implementations in ddebug, trace, and u_threaded_context can
(and probably should) be in a separate commit.
transition_resource: since it's currently a no-op, I would just leave it
out entirely for now. Also, if anything, I think the semantics are
better served by adding the layout parameter to
pipe_context::flush_resource and using only that (I believe you're
always calling both functions together when calling transition_resource,
which is a pretty strong hint :-)).
I wasn't sure if leaving it out would be considered good form. That is
less rebasing work for me, so I'm happy with that approach :)
As an additional minor point, the sequence of patches should really be:
- Gallium interface
- Gallium utils
- mesa patches
- enabling the extension in mesa/st comes last
- winsys & radeonsi patches
Will do. This plus your other feedback above on squashing the gallium
interfaces and splitting out the other ddebug/etc changes helps me
understand which of the little details are important. Thanks for taking
the time to point it out.
Regards,
Andres
Not a big deal, but should be possible to fix very easily.
I'm sending some more comments about flushes on individual patches.
Cheers,
Nicolai
On 02.11.2017 04:57, Andres Rodriguez wrote:
This series adds radeonsi support for GL_EXT_semaphore.
GL_EXT_semaphore is used by steam's vrclient to synchronize access to
shared
surfaces (vulkan <-> gl interop). It allows our gl vrclients to
enqueue their
framebuffer surface without waiting for a glFinish().
If anyone has any suggestions on improvements for the implicit flush
added in
patch 9 please let me know. Either a way to remove it, or a way to
move it to
a codepath that is a bit more specific to radeonsi.
Test results for this series (pretty alpha quality auto testing):
https://lostgoat.me/gpuci/jenkins/blue/organizations/jenkins/amdgpu-reviews/detail/amdgpu-reviews/184/pipeline
Test results for each individual patch can also be found here (see
'Related
Changes' section):
https://gerrit.lostgoat.me/#/c/454/
And here is a baseline for test results for comparison:
https://lostgoat.me/gpuci/jenkins/blue/organizations/jenkins/amdgpu-master/detail/amdgpu-master/1269/tests
Tests include Vulkan CTS and piglit on Kaveri and Polaris 10.
I just kicked off the tests, so they might still show in a queued/in
progress
state for a few hours. Hopefully my ryzen server and my test slaves
survive the
unsupervised effort (*fingers crossed it doesn't catch fire while I
sleep*).
Patches 1-12 add support for semaphore wait/signal/import
Patch 13 implements buffer/texture barriers
Patches 14-16 implement layout transitions
Patch 17 exposes the extension
Andres Rodriguez (17):
gallium: introduce PIPE_CAP_SEMAPHORE
mesa/st: expose EXT_semaphore and EXT_semaphore_fd
mesa: add support for semaphore object creation/import/delete
mesa: add semaphore parameter stub
gallium: introduce semaphore object
u_threaded_context: add support for semaphore wait/signal
mesa/st: add support for semaphore object create/import/delete
mesa: add support for semaphore object signal/wait
mesa/st: add support for waiting for semaphore objects
mesa: minor tidy up for memory object error strings
winsys/amdgpu: add support for syncobj signaling
radeonsi: implement semaphore operations
mesa: implement buffer/texture barriers for semaphore wait/signal
gallium: add transition_resource call
mesa/st: hook up resource transitions for semaphore calls
radeonsi: implement pipe transition_resource callback
radeonsi: advertise support for GL_EXT_semaphore
src/gallium/auxiliary/util/u_threaded_context.c | 52 ++++
.../auxiliary/util/u_threaded_context_calls.h | 1 +
src/gallium/docs/source/screen.rst | 1 +
src/gallium/drivers/ddebug/dd_context.c | 23 ++
src/gallium/drivers/ddebug/dd_screen.c | 25 ++
src/gallium/drivers/etnaviv/etnaviv_screen.c | 1 +
src/gallium/drivers/freedreno/freedreno_screen.c | 1 +
src/gallium/drivers/i915/i915_screen.c | 1 +
src/gallium/drivers/llvmpipe/lp_screen.c | 1 +
src/gallium/drivers/nouveau/nv30/nv30_screen.c | 1 +
src/gallium/drivers/nouveau/nv50/nv50_screen.c | 1 +
src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 1 +
src/gallium/drivers/r300/r300_screen.c | 1 +
src/gallium/drivers/r600/r600_pipe.c | 1 +
src/gallium/drivers/radeon/r600_pipe_common.c | 52 ++++
src/gallium/drivers/radeon/r600_pipe_common.h | 5 +
src/gallium/drivers/radeon/radeon_winsys.h | 12 +
src/gallium/drivers/radeonsi/si_blit.c | 11 +
src/gallium/drivers/radeonsi/si_pipe.c | 3 +
src/gallium/drivers/softpipe/sp_screen.c | 1 +
src/gallium/drivers/svga/svga_screen.c | 1 +
src/gallium/drivers/swr/swr_screen.cpp | 1 +
src/gallium/drivers/trace/tr_context.c | 36 +++
src/gallium/drivers/trace/tr_screen.c | 39 +++
src/gallium/drivers/vc4/vc4_screen.c | 1 +
src/gallium/drivers/virgl/virgl_screen.c | 1 +
src/gallium/include/pipe/p_context.h | 36 +++
src/gallium/include/pipe/p_defines.h | 12 +
src/gallium/include/pipe/p_screen.h | 22 ++
src/gallium/include/pipe/p_state.h | 8 +
src/gallium/winsys/amdgpu/drm/amdgpu_cs.c | 81 +++++-
src/gallium/winsys/amdgpu/drm/amdgpu_cs.h | 4 +
src/mesa/Makefile.sources | 2 +
src/mesa/drivers/common/driverfuncs.c | 3 +
src/mesa/main/dd.h | 58 +++++
src/mesa/main/extensions_table.h | 2 +
src/mesa/main/externalobjects.c | 274
+++++++++++++++++++--
src/mesa/main/externalobjects.h | 34 ++-
src/mesa/main/mtypes.h | 9 +
src/mesa/main/shared.c | 17 ++
src/mesa/meson.build | 2 +
src/mesa/state_tracker/st_cb_semaphoreobjects.c | 164 ++++++++++++
src/mesa/state_tracker/st_cb_semaphoreobjects.h | 25 ++
src/mesa/state_tracker/st_context.c | 2 +
src/mesa/state_tracker/st_extensions.c | 2 +
45 files changed, 1012 insertions(+), 19 deletions(-)
create mode 100644 src/mesa/state_tracker/st_cb_semaphoreobjects.c
create mode 100644 src/mesa/state_tracker/st_cb_semaphoreobjects.h
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
