Using the generic extension from the previous patch, a specific multisync
extension enables more than one in/out binary syncobj per job submission.
Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares
of determining the stage for sync (wait deps) according to the job
queue.
v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable (Iago)
v3:
- fix conditions to consider single or multiples in/out_syncs (Iago)
- remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/ffd8b2e3dd2e0c686db441a0c0a4a0181ff85328.1633016479.git.mwen@igalia.com
Add support to attach generic extensions on job submission. This patch
is third prep work to enable multiple syncobjs on job submission. With
this work, when the job submission interface needs to be extended to
accommodate a new feature, we will use a generic extension struct where
an id determines the data type to be pointed. The first application is
to enable multiples in/out syncobj (next patch), but the base is
already done for future features. Therefore, to attach a new feature,
a specific extension struct should subclass drm_v3d_extension and
update the list of extensions in a job submission.
v2:
- remove redundant elements to subclass struct (Daniel)
v3:
- add comment for v3d_get_extensions
Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/ed53b1cd7e3125b76f18fe3fb995a04393639bc6.1633016479.git.mwen@igalia.com
Move job memory allocation to v3d_job_init function. This aim to facilitate
error handling in job initialization, since cleanup steps are similar for
all (struct v3d_job)-based types of job involved in a command submission.
To generalize v3d_job_init(), this change takes into account that all job
structs have the first element a struct v3d_job (bin, render, tfu, csd) or
it is a v3d_job itself (clean_job) for pointer casting.
v3:
- explicitly init job as NULL (Iago)
- fix pm failure handling on v3_job_init (Iago)
Suggested-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/4d12e07bd334d2cddb51cabd359e99edde595619.1633016479.git.mwen@igalia.com
As requested in Documentation/gpu/todo.rst, replace driver calls to
drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
DRM_MODESET_LOCK_ALL_END()
While the previous commit was a simple "search and replace", this time I
had to do a bit of refactoring as only one call to
DRM_MODESET_LOCK_ALL_BEGIN() is allowed inside one same function.
Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20210924064324.229457-14-greenfoo@u92.eu
This can be used to create a separate DRM file description, thus
creating a new GEM handle namespace.
My use-case is wlroots. The library splits responsibilities between
separate components: the GBM allocator creates buffers, the GLES2
renderer uses EGL to import them and render to them, the DRM
backend imports the buffers and displays them. wlroots has a
modular architecture, and any of these components can be swapped
and replaced with something else. For instance, the pipeline can
be set up so that the DRM dumb buffer allocator is used instead of
GBM and the Pixman renderer is used instead of GLES2. Library users
can also replace any of these components with their own custom one.
DMA-BUFs are used to pass buffer references across components. We
could use GEM handles instead, but this would result in pain if
multiple GPUs are in use: wlroots copies buffers across GPUs as
needed. Importing a GEM handle created on one GPU into a completely
different GPU will blow up (fail at best, mix unrelated buffers
otherwise).
Everything is fine if all components use Mesa. However, this isn't
always desirable. For instance when running with DRM dumb buffers
and the Pixman software renderer it's unfortunate to depend on GBM
in the DRM backend just to turn DMA-BUFs into FB IDs. GBM loads
Mesa drivers to perform an action which has nothing driver-specific.
Additionally, drivers will fail the import if the 3D engine can't
use the imported buffer, for instance amdgpu will refuse to import
DRM dumb buffers [1]. We might also want to be running with a Vulkan
renderer and a Vulkan allocator in the future, and GBM wouldn't be
welcome in this setup.
To address this, GBM can be side-stepped in the DRM backend, and
can be replaced with drmPrimeFDToHandle calls. However because of
GEM handle reference counting issues, care must be taken to avoid
double-closing the same GEM handle. In particular, it's not
possible to share a DRM FD with GBM or EGL and perform some
drmPrimeFDToHandle calls manually.
So wlroots needs to re-open the DRM FD to create a new GEM handle
namespace. However there's no guarantee that the file-system
permissions will be set up so that the primary FD can be opened
by the compsoitor. On modern systems seatd or logind is a privileged
process responsible for doing this, and other processes aren't
expected to do it. For historical reasons systemd still allows
physically logged in users to open primary DRM nodes, but this
doesn't work on non-systemd setups and it's desirable to lock
them down at some point.
Some might suggest to open the render node instead of re-opening
the primary node. However some systems don't have a render node
at all (e.g. no GPU, or a split render/display SoC).
Solutions to this issue have been discussed in [2]. One solution
would be to open the magic /proc/self/fd/<fd> file, but it's a
Linux-specific hack (wlroots supports BSDs too). Another solution
is to add support for re-opening a DRM primary node to seatd/logind,
but they don't support it now and really haven't been designed for
this (logind would need to grow a completely new API, because it
assumes unique dev_t IDs). Also this seems like pushing down a
kernel limitation to user-space a bit too hard.
Another solution is to allow creating empty DRM leases. The lessee
FD would have its own GEM handle namespace, so wouldn't conflict
wth GBM/EGL. It would have the master bit set, but would be able
to manage zero resources. wlroots doesn't intend to share this FD
with any other process.
All in all IMHO that seems like a pretty reasonable solution to the
issue at hand.
Note, I've discussed with Jonas Ådahl and Mutter plans to adopt a
similar design in the future.
Example usage in wlroots is available at [3]. IGT test available
at [4].
[1]: https://github.com/swaywm/wlroots/issues/2916
[2]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
[3]: https://github.com/swaywm/wlroots/pull/3158
[4]: https://patchwork.freedesktop.org/series/94323/
Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Dave Airlie <airlied@redhat.com>
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210903130000.1590-2-contact@emersion.fr
Daniel pointed me towards this function and there are multiple obvious problems
in the implementation.
First of all the retry loop is not working as intended. In general the retry
makes only sense if you grab the reference first and then check the sequence
values.
Then we should always also wait for the exclusive fence.
It's also good practice to keep the reference around when installing callbacks
to fences you don't own.
And last the whole implementation was unnecessary complex and rather hard to
understand which could lead to probably unexpected behavior of the IOCTL.
Fix all this by reworking the implementation from scratch. Dropping the
whole RCU approach and taking the lock instead.
Only mildly tested and needs a thoughtful review of the code.
Pushing through drm-misc-next to avoid merge conflicts and give the code
another round of testing.
v2: fix the reference counting as well
v3: keep the excl fence handling as is for stable
v4: back to testing all fences, drop RCU
v5: handle in and out separately
v6: add missing clear of events
v7: change coding style as suggested by Michel, drop unused variables
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Tested-by: Michel Dänzer <mdaenzer@redhat.com>
CC: stable@vger.kernel.org
Link: https://patchwork.freedesktop.org/patch/msgid/20210720131110.88512-1-christian.koenig@amd.com
