We have multiple Vulkan context types that are awaiting for the addition
of the sync object DRM UAPI support to the VirtIO-GPU kernel driver:
1. Venus context
2. Native contexts (virtio-freedreno, virtio-intel, virtio-amdgpu)
Mesa core supports DRM sync object UAPI, providing Vulkan drivers with a
generic fencing implementation that we want to utilize.
This patch adds initial sync objects support. It creates fundament for a
further fencing improvements. Later on we will want to extend the VirtIO-GPU
fencing API with passing fence IDs to host for waiting, it will be a new
additional VirtIO-GPU IOCTL and more. Today we have several VirtIO-GPU context
drivers in works that require VirtIO-GPU to support sync objects UAPI.
The patch is heavily inspired by the sync object UAPI implementation of the
MSM driver.
Changelog:
v3: - Switched to use dma_fence_unwrap_for_each(), like was suggested by
Rob Clark.
- Fixed missing dma_fence_put() in error code path that was spotted by
Rob Clark.
- Removed obsoleted comment to virtio_gpu_execbuffer_ioctl(), like was
suggested by Rob Clark.
v2: - Fixed chain-fence context matching by making use of
dma_fence_chain_contained().
- Fixed potential uninitialized var usage in error code patch of
parse_post_deps(). MSM driver had a similar issue that is fixed
already in upstream.
- Added new patch that refactors job submission code path. I found
that it was very difficult to add a new/upcoming host-waits feature
because of how variables are passed around the code, the virtgpu_ioctl.c
also was growing to unmanageable size.
Dmitry Osipenko (2):
drm/virtio: Refactor job submission code path
drm/virtio: Support sync objects
drivers/gpu/drm/virtio/Makefile | 2 +-
drivers/gpu/drm/virtio/virtgpu_drv.c | 3 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 4 +
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 182 ---------
drivers/gpu/drm/virtio/virtgpu_submit.c | 513 ++++++++++++++++++++++++
include/uapi/drm/virtgpu_drm.h | 16 +-
6 files changed, 535 insertions(+), 185 deletions(-)
create mode 100644 drivers/gpu/drm/virtio/virtgpu_submit.c
--
2.39.2
Move virtio_gpu_execbuffer_ioctl() into separate virtgpu_submit.c file
and refactor the code along the way to ease addition of new features to
the ioctl.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/Makefile | 2 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 4 +
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 182 --------------
drivers/gpu/drm/virtio/virtgpu_submit.c | 302 ++++++++++++++++++++++++
4 files changed, 307 insertions(+), 183 deletions(-)
create mode 100644 drivers/gpu/drm/virtio/virtgpu_submit.c
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..d2e1788a8227 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -6,6 +6,6 @@
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
virtgpu_display.o virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
- virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+ virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o virtgpu_submit.o
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..4126c384286b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -486,4 +486,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
struct sg_table *sgt,
enum dma_data_direction dir);
+/* virtgpu_submit.c */
+int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
+
#endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index da45215a933d..b24b11f25197 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -38,36 +38,6 @@
VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
-static int virtio_gpu_fence_event_create(struct drm_device *dev,
- struct drm_file *file,
- struct virtio_gpu_fence *fence,
- uint32_t ring_idx)
-{
- struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
- struct virtio_gpu_fence_event *e = NULL;
- int ret;
-
- if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
- return 0;
-
- e = kzalloc(sizeof(*e), GFP_KERNEL);
- if (!e)
- return -ENOMEM;
-
- e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
- e->event.length = sizeof(e->event);
-
- ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
- if (ret)
- goto free;
-
- fence->e = e;
- return 0;
-free:
- kfree(e);
- return ret;
-}
-
/* Must be called with &virtio_gpu_fpriv.struct_mutex held. */
static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
struct virtio_gpu_fpriv *vfpriv)
@@ -108,158 +78,6 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
&virtio_gpu_map->offset);
}
-/*
- * Usage of execbuffer:
- * Relocations need to take into account the full VIRTIO_GPUDrawable size.
- * However, the command as passed from user space must *not* contain the initial
- * VIRTIO_GPUReleaseInfo struct (first XXX bytes)
- */
-static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file)
-{
- struct drm_virtgpu_execbuffer *exbuf = data;
- struct virtio_gpu_device *vgdev = dev->dev_private;
- struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
- struct virtio_gpu_fence *out_fence;
- int ret;
- uint32_t *bo_handles = NULL;
- void __user *user_bo_handles = NULL;
- struct virtio_gpu_object_array *buflist = NULL;
- struct sync_file *sync_file;
- int out_fence_fd = -1;
- void *buf;
- uint64_t fence_ctx;
- uint32_t ring_idx;
-
- fence_ctx = vgdev->fence_drv.context;
- ring_idx = 0;
-
- if (vgdev->has_virgl_3d == false)
- return -ENOSYS;
-
- if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
- return -EINVAL;
-
- if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX)) {
- if (exbuf->ring_idx >= vfpriv->num_rings)
- return -EINVAL;
-
- if (!vfpriv->base_fence_ctx)
- return -EINVAL;
-
- fence_ctx = vfpriv->base_fence_ctx;
- ring_idx = exbuf->ring_idx;
- }
-
- virtio_gpu_create_context(dev, file);
- if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
- struct dma_fence *in_fence;
-
- in_fence = sync_file_get_fence(exbuf->fence_fd);
-
- if (!in_fence)
- return -EINVAL;
-
- /*
- * Wait if the fence is from a foreign context, or if the fence
- * array contains any fence from a foreign context.
- */
- ret = 0;
- if (!dma_fence_match_context(in_fence, fence_ctx + ring_idx))
- ret = dma_fence_wait(in_fence, true);
-
- dma_fence_put(in_fence);
- if (ret)
- return ret;
- }
-
- if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
- out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
- if (out_fence_fd < 0)
- return out_fence_fd;
- }
-
- if (exbuf->num_bo_handles) {
- bo_handles = kvmalloc_array(exbuf->num_bo_handles,
- sizeof(uint32_t), GFP_KERNEL);
- if (!bo_handles) {
- ret = -ENOMEM;
- goto out_unused_fd;
- }
-
- user_bo_handles = u64_to_user_ptr(exbuf->bo_handles);
- if (copy_from_user(bo_handles, user_bo_handles,
- exbuf->num_bo_handles * sizeof(uint32_t))) {
- ret = -EFAULT;
- goto out_unused_fd;
- }
-
- buflist = virtio_gpu_array_from_handles(file, bo_handles,
- exbuf->num_bo_handles);
- if (!buflist) {
- ret = -ENOENT;
- goto out_unused_fd;
- }
- kvfree(bo_handles);
- bo_handles = NULL;
- }
-
- buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
- if (IS_ERR(buf)) {
- ret = PTR_ERR(buf);
- goto out_unused_fd;
- }
-
- if (buflist) {
- ret = virtio_gpu_array_lock_resv(buflist);
- if (ret)
- goto out_memdup;
- }
-
- out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
- if(!out_fence) {
- ret = -ENOMEM;
- goto out_unresv;
- }
-
- ret = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
- if (ret)
- goto out_unresv;
-
- if (out_fence_fd >= 0) {
- sync_file = sync_file_create(&out_fence->f);
- if (!sync_file) {
- dma_fence_put(&out_fence->f);
- ret = -ENOMEM;
- goto out_unresv;
- }
-
- exbuf->fence_fd = out_fence_fd;
- fd_install(out_fence_fd, sync_file->file);
- }
-
- virtio_gpu_cmd_submit(vgdev, buf, exbuf->size,
- vfpriv->ctx_id, buflist, out_fence);
- dma_fence_put(&out_fence->f);
- virtio_gpu_notify(vgdev);
- return 0;
-
-out_unresv:
- if (buflist)
- virtio_gpu_array_unlock_resv(buflist);
-out_memdup:
- kvfree(buf);
-out_unused_fd:
- kvfree(bo_handles);
- if (buflist)
- virtio_gpu_array_put_free(buflist);
-
- if (out_fence_fd >= 0)
- put_unused_fd(out_fence_fd);
-
- return ret;
-}
-
static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
new file mode 100644
index 000000000000..2ce2459c6bc2
--- /dev/null
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * Authors:
+ * Dave Airlie
+ * Alon Levy
+ */
+
+#include <linux/dma-fence-unwrap.h>
+#include <linux/file.h>
+#include <linux/sync_file.h>
+#include <linux/uaccess.h>
+
+#include <drm/drm_file.h>
+#include <drm/virtgpu_drm.h>
+
+#include "virtgpu_drv.h"
+
+struct virtio_gpu_submit {
+ struct virtio_gpu_object_array *buflist;
+ struct drm_virtgpu_execbuffer *exbuf;
+ struct virtio_gpu_fence *out_fence;
+ struct virtio_gpu_fpriv *vfpriv;
+ struct virtio_gpu_device *vgdev;
+ struct drm_file *file;
+ uint64_t fence_ctx;
+ uint32_t ring_idx;
+ int out_fence_fd;
+ void *buf;
+};
+
+static int virtio_gpu_do_fence_wait(struct virtio_gpu_submit *submit,
+ struct dma_fence *in_fence)
+{
+ uint32_t context = submit->fence_ctx + submit->ring_idx;
+
+ if (dma_fence_match_context(in_fence, context))
+ return 0;
+
+ return dma_fence_wait(in_fence, true);
+}
+
+static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
+ struct dma_fence *fence)
+{
+ struct dma_fence_unwrap itr;
+ struct dma_fence *f;
+ int err;
+
+ dma_fence_unwrap_for_each(f, &itr, fence) {
+ err = virtio_gpu_do_fence_wait(submit, f);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int virtio_gpu_fence_event_create(struct drm_device *dev,
+ struct drm_file *file,
+ struct virtio_gpu_fence *fence,
+ uint32_t ring_idx)
+{
+ struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+ struct virtio_gpu_fence_event *e = NULL;
+ int ret;
+
+ if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
+ return 0;
+
+ e = kzalloc(sizeof(*e), GFP_KERNEL);
+ if (!e)
+ return -ENOMEM;
+
+ e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
+ e->event.length = sizeof(e->event);
+
+ ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
+ if (ret) {
+ kfree(e);
+ return ret;
+ }
+
+ fence->e = e;
+
+ return 0;
+}
+
+static int virtio_gpu_init_submit_buflist(struct virtio_gpu_submit *submit)
+{
+ struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
+ uint32_t *bo_handles;
+
+ if (!exbuf->num_bo_handles)
+ return 0;
+
+ bo_handles = kvmalloc_array(exbuf->num_bo_handles, sizeof(uint32_t),
+ GFP_KERNEL);
+ if (!bo_handles)
+ return -ENOMEM;
+
+ if (copy_from_user(bo_handles, u64_to_user_ptr(exbuf->bo_handles),
+ exbuf->num_bo_handles * sizeof(uint32_t))) {
+ kvfree(bo_handles);
+ return -EFAULT;
+ }
+
+ submit->buflist = virtio_gpu_array_from_handles(submit->file, bo_handles,
+ exbuf->num_bo_handles);
+ if (!submit->buflist) {
+ kvfree(bo_handles);
+ return -ENOENT;
+ }
+
+ kvfree(bo_handles);
+
+ return 0;
+}
+
+static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
+{
+ if (!IS_ERR(submit->buf))
+ kvfree(submit->buf);
+
+ if (submit->buflist)
+ virtio_gpu_array_put_free(submit->buflist);
+
+ if (submit->out_fence_fd >= 0)
+ put_unused_fd(submit->out_fence_fd);
+
+ if (submit->out_fence)
+ dma_fence_put(&submit->out_fence->f);
+}
+
+static void virtio_gpu_submit(struct virtio_gpu_submit *submit)
+{
+ virtio_gpu_cmd_submit(submit->vgdev, submit->buf, submit->exbuf->size,
+ submit->vfpriv->ctx_id, submit->buflist,
+ submit->out_fence);
+ virtio_gpu_notify(submit->vgdev);
+}
+
+static void virtio_gpu_complete_submit(struct virtio_gpu_submit *submit)
+{
+ submit->buf = NULL;
+ submit->buflist = NULL;
+ submit->out_fence = NULL;
+ submit->out_fence_fd = -1;
+}
+
+static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
+ struct drm_virtgpu_execbuffer *exbuf,
+ struct drm_device *dev,
+ struct drm_file *file,
+ uint64_t fence_ctx, uint32_t ring_idx)
+{
+ struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+ struct virtio_gpu_fence *out_fence;
+ int err;
+
+ memset(submit, 0, sizeof(*submit));
+
+ out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
+ if (!out_fence)
+ return -ENOMEM;
+
+ err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
+ if (err) {
+ dma_fence_put(&out_fence->f);
+ return err;
+ }
+
+ submit->out_fence = out_fence;
+ submit->fence_ctx = fence_ctx;
+ submit->ring_idx = ring_idx;
+ submit->out_fence_fd = -1;
+ submit->vfpriv = vfpriv;
+ submit->vgdev = vgdev;
+ submit->exbuf = exbuf;
+ submit->file = file;
+
+ err = virtio_gpu_init_submit_buflist(submit);
+ if (err)
+ return err;
+
+ submit->buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
+ if (IS_ERR(submit->buf))
+ return PTR_ERR(submit->buf);
+
+ if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
+ err = get_unused_fd_flags(O_CLOEXEC);
+ if (err < 0)
+ return err;
+
+ submit->out_fence_fd = err;
+ }
+
+ return 0;
+}
+
+static int virtio_gpu_wait_in_fence(struct virtio_gpu_submit *submit)
+{
+ int ret = 0;
+
+ if (submit->exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
+ struct dma_fence *in_fence =
+ sync_file_get_fence(submit->exbuf->fence_fd);
+ if (!in_fence)
+ return -EINVAL;
+
+ /*
+ * Wait if the fence is from a foreign context, or if the fence
+ * array contains any fence from a foreign context.
+ */
+ ret = virtio_gpu_dma_fence_wait(submit, in_fence);
+
+ dma_fence_put(in_fence);
+ }
+
+ return ret;
+}
+
+static int virtio_gpu_install_out_fence_fd(struct virtio_gpu_submit *submit)
+{
+ if (submit->out_fence_fd >= 0) {
+ struct sync_file *sync_file =
+ sync_file_create(&submit->out_fence->f);
+ if (!sync_file)
+ return -ENOMEM;
+
+ submit->exbuf->fence_fd = submit->out_fence_fd;
+ fd_install(submit->out_fence_fd, sync_file->file);
+ }
+
+ return 0;
+}
+
+static int virtio_gpu_lock_buflist(struct virtio_gpu_submit *submit)
+{
+ if (submit->buflist)
+ return virtio_gpu_array_lock_resv(submit->buflist);
+
+ return 0;
+}
+
+int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+ struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+ uint64_t fence_ctx = vgdev->fence_drv.context;
+ struct drm_virtgpu_execbuffer *exbuf = data;
+ struct virtio_gpu_submit submit;
+ uint32_t ring_idx = 0;
+ int ret = -EINVAL;
+
+ if (vgdev->has_virgl_3d == false)
+ return -ENOSYS;
+
+ if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
+ return ret;
+
+ if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX)) {
+ if (exbuf->ring_idx >= vfpriv->num_rings)
+ return ret;
+
+ if (!vfpriv->base_fence_ctx)
+ return ret;
+
+ fence_ctx = vfpriv->base_fence_ctx;
+ ring_idx = exbuf->ring_idx;
+ }
+
+ virtio_gpu_create_context(dev, file);
+
+ ret = virtio_gpu_init_submit(&submit, exbuf, dev, file,
+ fence_ctx, ring_idx);
+ if (ret)
+ goto cleanup;
+
+ ret = virtio_gpu_wait_in_fence(&submit);
+ if (ret)
+ goto cleanup;
+
+ ret = virtio_gpu_install_out_fence_fd(&submit);
+ if (ret)
+ goto cleanup;
+
+ ret = virtio_gpu_lock_buflist(&submit);
+ if (ret)
+ goto cleanup;
+
+ virtio_gpu_submit(&submit);
+ virtio_gpu_complete_submit(&submit);
+cleanup:
+ virtio_gpu_cleanup_submit(&submit);
+
+ return ret;
+}
--
2.39.2
Add sync object DRM UAPI support to VirtIO-GPU driver. It's required
for enabling a full-featured Vulkan fencing by Venus and native context
VirtIO-GPU Mesa drivers.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_drv.c | 3 +-
drivers/gpu/drm/virtio/virtgpu_submit.c | 211 ++++++++++++++++++++++++
include/uapi/drm/virtgpu_drm.h | 16 +-
3 files changed, 228 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index add075681e18..a22155577152 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -176,7 +176,8 @@ static const struct drm_driver driver = {
* If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
* out via drm_device::driver_features:
*/
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+ .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC |
+ DRIVER_SYNCOBJ | DRIVER_SYNCOBJ_TIMELINE,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
index 2ce2459c6bc2..9ea4390948bf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_submit.c
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -14,11 +14,26 @@
#include <linux/uaccess.h>
#include <drm/drm_file.h>
+#include <drm/drm_syncobj.h>
#include <drm/virtgpu_drm.h>
#include "virtgpu_drv.h"
+struct virtio_gpu_submit_post_dep {
+ struct drm_syncobj *syncobj;
+ struct dma_fence_chain *chain;
+ uint64_t point;
+};
+
struct virtio_gpu_submit {
+ struct virtio_gpu_submit_post_dep *post_deps;
+ unsigned int num_out_syncobjs;
+
+ struct drm_syncobj **in_syncobjs;
+ unsigned int num_in_syncobjs;
+ uint64_t *in_fence_ids;
+ unsigned int num_in_fence_ids;
+
struct virtio_gpu_object_array *buflist;
struct drm_virtgpu_execbuffer *exbuf;
struct virtio_gpu_fence *out_fence;
@@ -58,6 +73,186 @@ static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
return 0;
}
+static void virtio_gpu_free_syncobjs(struct drm_syncobj **syncobjs,
+ uint32_t nr_syncobjs)
+{
+ uint32_t i = nr_syncobjs;
+
+ while (syncobjs && i--) {
+ if (syncobjs[i])
+ drm_syncobj_put(syncobjs[i]);
+ }
+
+ kfree(syncobjs);
+}
+
+static int
+virtio_gpu_parse_deps(struct virtio_gpu_submit *submit)
+{
+ struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
+ struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
+ size_t syncobj_stride = exbuf->syncobj_stride;
+ struct drm_syncobj **syncobjs;
+ int ret = 0, i;
+
+ if (!submit->num_in_syncobjs)
+ return 0;
+
+ syncobjs = kcalloc(submit->num_in_syncobjs, sizeof(*syncobjs),
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!syncobjs)
+ return -ENOMEM;
+
+ for (i = 0; i < submit->num_in_syncobjs; i++) {
+ uint64_t address = exbuf->in_syncobjs + i * syncobj_stride;
+ struct dma_fence *fence;
+
+ if (copy_from_user(&syncobj_desc,
+ u64_to_user_ptr(address),
+ min(syncobj_stride, sizeof(syncobj_desc)))) {
+ ret = -EFAULT;
+ break;
+ }
+
+ if (syncobj_desc.flags & ~VIRTGPU_EXECBUF_SYNCOBJ_FLAGS) {
+ ret = -EINVAL;
+ break;
+ }
+
+ ret = drm_syncobj_find_fence(submit->file, syncobj_desc.handle,
+ syncobj_desc.point, 0, &fence);
+ if (ret)
+ break;
+
+ ret = virtio_gpu_dma_fence_wait(submit, fence);
+
+ dma_fence_put(fence);
+ if (ret)
+ break;
+
+ if (syncobj_desc.flags & VIRTGPU_EXECBUF_SYNCOBJ_RESET) {
+ syncobjs[i] =
+ drm_syncobj_find(submit->file, syncobj_desc.handle);
+ if (!syncobjs[i]) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+ }
+
+ if (ret) {
+ virtio_gpu_free_syncobjs(syncobjs, i);
+ return ret;
+ }
+
+ submit->in_syncobjs = syncobjs;
+
+ return ret;
+}
+
+static void virtio_gpu_reset_syncobjs(struct drm_syncobj **syncobjs,
+ uint32_t nr_syncobjs)
+{
+ uint32_t i;
+
+ for (i = 0; syncobjs && i < nr_syncobjs; i++) {
+ if (syncobjs[i])
+ drm_syncobj_replace_fence(syncobjs[i], NULL);
+ }
+}
+
+static void
+virtio_gpu_free_post_deps(struct virtio_gpu_submit_post_dep *post_deps,
+ uint32_t nr_syncobjs)
+{
+ uint32_t i = nr_syncobjs;
+
+ while (post_deps && i--) {
+ kfree(post_deps[i].chain);
+ drm_syncobj_put(post_deps[i].syncobj);
+ }
+
+ kfree(post_deps);
+}
+
+static int virtio_gpu_parse_post_deps(struct virtio_gpu_submit *submit)
+{
+ struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
+ struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
+ struct virtio_gpu_submit_post_dep *post_deps;
+ size_t syncobj_stride = exbuf->syncobj_stride;
+ int ret = 0, i;
+
+ if (!submit->num_out_syncobjs)
+ return 0;
+
+ post_deps = kcalloc(submit->num_out_syncobjs, sizeof(*post_deps),
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!post_deps)
+ return -ENOMEM;
+
+ for (i = 0; i < submit->num_out_syncobjs; i++) {
+ uint64_t address = exbuf->out_syncobjs + i * syncobj_stride;
+
+ if (copy_from_user(&syncobj_desc,
+ u64_to_user_ptr(address),
+ min(syncobj_stride, sizeof(syncobj_desc)))) {
+ ret = -EFAULT;
+ break;
+ }
+
+ post_deps[i].point = syncobj_desc.point;
+
+ if (syncobj_desc.flags) {
+ ret = -EINVAL;
+ break;
+ }
+
+ if (syncobj_desc.point) {
+ post_deps[i].chain = dma_fence_chain_alloc();
+ if (!post_deps[i].chain) {
+ ret = -ENOMEM;
+ break;
+ }
+ }
+
+ post_deps[i].syncobj =
+ drm_syncobj_find(submit->file, syncobj_desc.handle);
+ if (!post_deps[i].syncobj) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+
+ if (ret) {
+ virtio_gpu_free_post_deps(post_deps, i);
+ return ret;
+ }
+
+ submit->post_deps = post_deps;
+
+ return 0;
+}
+
+static void
+virtio_gpu_process_post_deps(struct virtio_gpu_submit *submit)
+{
+ struct virtio_gpu_submit_post_dep *post_deps = submit->post_deps;
+ struct dma_fence *fence = &submit->out_fence->f;
+ uint32_t i;
+
+ for (i = 0; post_deps && i < submit->num_out_syncobjs; i++) {
+ if (post_deps[i].chain) {
+ drm_syncobj_add_point(post_deps[i].syncobj,
+ post_deps[i].chain,
+ fence, post_deps[i].point);
+ post_deps[i].chain = NULL;
+ } else {
+ drm_syncobj_replace_fence(post_deps[i].syncobj, fence);
+ }
+ }
+}
+
static int virtio_gpu_fence_event_create(struct drm_device *dev,
struct drm_file *file,
struct virtio_gpu_fence *fence,
@@ -121,6 +316,11 @@ static int virtio_gpu_init_submit_buflist(struct virtio_gpu_submit *submit)
static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
{
+ virtio_gpu_reset_syncobjs(submit->in_syncobjs, submit->num_in_syncobjs);
+ virtio_gpu_free_syncobjs(submit->in_syncobjs, submit->num_in_syncobjs);
+ virtio_gpu_free_post_deps(submit->post_deps, submit->num_out_syncobjs);
+ kfree(submit->in_fence_ids);
+
if (!IS_ERR(submit->buf))
kvfree(submit->buf);
@@ -173,6 +373,8 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
return err;
}
+ submit->num_out_syncobjs = exbuf->num_out_syncobjs;
+ submit->num_in_syncobjs = exbuf->num_in_syncobjs;
submit->out_fence = out_fence;
submit->fence_ctx = fence_ctx;
submit->ring_idx = ring_idx;
@@ -285,6 +487,14 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
if (ret)
goto cleanup;
+ ret = virtio_gpu_parse_deps(&submit);
+ if (ret)
+ goto cleanup;
+
+ ret = virtio_gpu_parse_post_deps(&submit);
+ if (ret)
+ goto cleanup;
+
ret = virtio_gpu_install_out_fence_fd(&submit);
if (ret)
goto cleanup;
@@ -294,6 +504,7 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
goto cleanup;
virtio_gpu_submit(&submit);
+ virtio_gpu_process_post_deps(&submit);
virtio_gpu_complete_submit(&submit);
cleanup:
virtio_gpu_cleanup_submit(&submit);
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 7b158fcb02b4..ce4948aacafd 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -64,6 +64,16 @@ struct drm_virtgpu_map {
__u32 pad;
};
+#define VIRTGPU_EXECBUF_SYNCOBJ_RESET 0x01
+#define VIRTGPU_EXECBUF_SYNCOBJ_FLAGS ( \
+ VIRTGPU_EXECBUF_SYNCOBJ_RESET | \
+ 0)
+struct drm_virtgpu_execbuffer_syncobj {
+ __u32 handle;
+ __u32 flags;
+ __u64 point;
+};
+
/* fence_fd is modified on success if VIRTGPU_EXECBUF_FENCE_FD_OUT flag is set. */
struct drm_virtgpu_execbuffer {
__u32 flags;
@@ -73,7 +83,11 @@ struct drm_virtgpu_execbuffer {
__u32 num_bo_handles;
__s32 fence_fd; /* in/out fence fd (see VIRTGPU_EXECBUF_FENCE_FD_IN/OUT) */
__u32 ring_idx; /* command ring index (see VIRTGPU_EXECBUF_RING_IDX) */
- __u32 pad;
+ __u32 syncobj_stride;
+ __u32 num_in_syncobjs;
+ __u32 num_out_syncobjs;
+ __u64 in_syncobjs;
+ __u64 out_syncobjs;
};
#define VIRTGPU_PARAM_3D_FEATURES 1 /* do we have 3D features in the hw */
--
2.39.2
On Thu, Mar 23, 2023 at 12:05 PM Dmitry Osipenko
<[email protected]> wrote:
>
> Move virtio_gpu_execbuffer_ioctl() into separate virtgpu_submit.c file
> and refactor the code along the way to ease addition of new features to
> the ioctl.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
Reviewed-by: Rob Clark <[email protected]>
> ---
> drivers/gpu/drm/virtio/Makefile | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 4 +
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 182 --------------
> drivers/gpu/drm/virtio/virtgpu_submit.c | 302 ++++++++++++++++++++++++
> 4 files changed, 307 insertions(+), 183 deletions(-)
> create mode 100644 drivers/gpu/drm/virtio/virtgpu_submit.c
>
> diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
> index b99fa4a73b68..d2e1788a8227 100644
> --- a/drivers/gpu/drm/virtio/Makefile
> +++ b/drivers/gpu/drm/virtio/Makefile
> @@ -6,6 +6,6 @@
> virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
> virtgpu_display.o virtgpu_vq.o \
> virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
> - virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
> + virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o virtgpu_submit.o
>
> obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index af6ffb696086..4126c384286b 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -486,4 +486,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
> struct sg_table *sgt,
> enum dma_data_direction dir);
>
> +/* virtgpu_submit.c */
> +int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
> +
> #endif
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index da45215a933d..b24b11f25197 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -38,36 +38,6 @@
> VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
> VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
>
> -static int virtio_gpu_fence_event_create(struct drm_device *dev,
> - struct drm_file *file,
> - struct virtio_gpu_fence *fence,
> - uint32_t ring_idx)
> -{
> - struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> - struct virtio_gpu_fence_event *e = NULL;
> - int ret;
> -
> - if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
> - return 0;
> -
> - e = kzalloc(sizeof(*e), GFP_KERNEL);
> - if (!e)
> - return -ENOMEM;
> -
> - e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
> - e->event.length = sizeof(e->event);
> -
> - ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
> - if (ret)
> - goto free;
> -
> - fence->e = e;
> - return 0;
> -free:
> - kfree(e);
> - return ret;
> -}
> -
> /* Must be called with &virtio_gpu_fpriv.struct_mutex held. */
> static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_fpriv *vfpriv)
> @@ -108,158 +78,6 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
> &virtio_gpu_map->offset);
> }
>
> -/*
> - * Usage of execbuffer:
> - * Relocations need to take into account the full VIRTIO_GPUDrawable size.
> - * However, the command as passed from user space must *not* contain the initial
> - * VIRTIO_GPUReleaseInfo struct (first XXX bytes)
> - */
> -static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> - struct drm_file *file)
> -{
> - struct drm_virtgpu_execbuffer *exbuf = data;
> - struct virtio_gpu_device *vgdev = dev->dev_private;
> - struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> - struct virtio_gpu_fence *out_fence;
> - int ret;
> - uint32_t *bo_handles = NULL;
> - void __user *user_bo_handles = NULL;
> - struct virtio_gpu_object_array *buflist = NULL;
> - struct sync_file *sync_file;
> - int out_fence_fd = -1;
> - void *buf;
> - uint64_t fence_ctx;
> - uint32_t ring_idx;
> -
> - fence_ctx = vgdev->fence_drv.context;
> - ring_idx = 0;
> -
> - if (vgdev->has_virgl_3d == false)
> - return -ENOSYS;
> -
> - if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
> - return -EINVAL;
> -
> - if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX)) {
> - if (exbuf->ring_idx >= vfpriv->num_rings)
> - return -EINVAL;
> -
> - if (!vfpriv->base_fence_ctx)
> - return -EINVAL;
> -
> - fence_ctx = vfpriv->base_fence_ctx;
> - ring_idx = exbuf->ring_idx;
> - }
> -
> - virtio_gpu_create_context(dev, file);
> - if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
> - struct dma_fence *in_fence;
> -
> - in_fence = sync_file_get_fence(exbuf->fence_fd);
> -
> - if (!in_fence)
> - return -EINVAL;
> -
> - /*
> - * Wait if the fence is from a foreign context, or if the fence
> - * array contains any fence from a foreign context.
> - */
> - ret = 0;
> - if (!dma_fence_match_context(in_fence, fence_ctx + ring_idx))
> - ret = dma_fence_wait(in_fence, true);
> -
> - dma_fence_put(in_fence);
> - if (ret)
> - return ret;
> - }
> -
> - if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
> - out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> - if (out_fence_fd < 0)
> - return out_fence_fd;
> - }
> -
> - if (exbuf->num_bo_handles) {
> - bo_handles = kvmalloc_array(exbuf->num_bo_handles,
> - sizeof(uint32_t), GFP_KERNEL);
> - if (!bo_handles) {
> - ret = -ENOMEM;
> - goto out_unused_fd;
> - }
> -
> - user_bo_handles = u64_to_user_ptr(exbuf->bo_handles);
> - if (copy_from_user(bo_handles, user_bo_handles,
> - exbuf->num_bo_handles * sizeof(uint32_t))) {
> - ret = -EFAULT;
> - goto out_unused_fd;
> - }
> -
> - buflist = virtio_gpu_array_from_handles(file, bo_handles,
> - exbuf->num_bo_handles);
> - if (!buflist) {
> - ret = -ENOENT;
> - goto out_unused_fd;
> - }
> - kvfree(bo_handles);
> - bo_handles = NULL;
> - }
> -
> - buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
> - if (IS_ERR(buf)) {
> - ret = PTR_ERR(buf);
> - goto out_unused_fd;
> - }
> -
> - if (buflist) {
> - ret = virtio_gpu_array_lock_resv(buflist);
> - if (ret)
> - goto out_memdup;
> - }
> -
> - out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> - if(!out_fence) {
> - ret = -ENOMEM;
> - goto out_unresv;
> - }
> -
> - ret = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> - if (ret)
> - goto out_unresv;
> -
> - if (out_fence_fd >= 0) {
> - sync_file = sync_file_create(&out_fence->f);
> - if (!sync_file) {
> - dma_fence_put(&out_fence->f);
> - ret = -ENOMEM;
> - goto out_unresv;
> - }
> -
> - exbuf->fence_fd = out_fence_fd;
> - fd_install(out_fence_fd, sync_file->file);
> - }
> -
> - virtio_gpu_cmd_submit(vgdev, buf, exbuf->size,
> - vfpriv->ctx_id, buflist, out_fence);
> - dma_fence_put(&out_fence->f);
> - virtio_gpu_notify(vgdev);
> - return 0;
> -
> -out_unresv:
> - if (buflist)
> - virtio_gpu_array_unlock_resv(buflist);
> -out_memdup:
> - kvfree(buf);
> -out_unused_fd:
> - kvfree(bo_handles);
> - if (buflist)
> - virtio_gpu_array_put_free(buflist);
> -
> - if (out_fence_fd >= 0)
> - put_unused_fd(out_fence_fd);
> -
> - return ret;
> -}
> -
> static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file)
> {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> new file mode 100644
> index 000000000000..2ce2459c6bc2
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2015 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * Authors:
> + * Dave Airlie
> + * Alon Levy
> + */
> +
> +#include <linux/dma-fence-unwrap.h>
> +#include <linux/file.h>
> +#include <linux/sync_file.h>
> +#include <linux/uaccess.h>
> +
> +#include <drm/drm_file.h>
> +#include <drm/virtgpu_drm.h>
> +
> +#include "virtgpu_drv.h"
> +
> +struct virtio_gpu_submit {
> + struct virtio_gpu_object_array *buflist;
> + struct drm_virtgpu_execbuffer *exbuf;
> + struct virtio_gpu_fence *out_fence;
> + struct virtio_gpu_fpriv *vfpriv;
> + struct virtio_gpu_device *vgdev;
> + struct drm_file *file;
> + uint64_t fence_ctx;
> + uint32_t ring_idx;
> + int out_fence_fd;
> + void *buf;
> +};
> +
> +static int virtio_gpu_do_fence_wait(struct virtio_gpu_submit *submit,
> + struct dma_fence *in_fence)
> +{
> + uint32_t context = submit->fence_ctx + submit->ring_idx;
> +
> + if (dma_fence_match_context(in_fence, context))
> + return 0;
> +
> + return dma_fence_wait(in_fence, true);
> +}
> +
> +static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
> + struct dma_fence *fence)
> +{
> + struct dma_fence_unwrap itr;
> + struct dma_fence *f;
> + int err;
> +
> + dma_fence_unwrap_for_each(f, &itr, fence) {
> + err = virtio_gpu_do_fence_wait(submit, f);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_fence_event_create(struct drm_device *dev,
> + struct drm_file *file,
> + struct virtio_gpu_fence *fence,
> + uint32_t ring_idx)
> +{
> + struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> + struct virtio_gpu_fence_event *e = NULL;
> + int ret;
> +
> + if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
> + return 0;
> +
> + e = kzalloc(sizeof(*e), GFP_KERNEL);
> + if (!e)
> + return -ENOMEM;
> +
> + e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
> + e->event.length = sizeof(e->event);
> +
> + ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
> + if (ret) {
> + kfree(e);
> + return ret;
> + }
> +
> + fence->e = e;
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_init_submit_buflist(struct virtio_gpu_submit *submit)
> +{
> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
> + uint32_t *bo_handles;
> +
> + if (!exbuf->num_bo_handles)
> + return 0;
> +
> + bo_handles = kvmalloc_array(exbuf->num_bo_handles, sizeof(uint32_t),
> + GFP_KERNEL);
> + if (!bo_handles)
> + return -ENOMEM;
> +
> + if (copy_from_user(bo_handles, u64_to_user_ptr(exbuf->bo_handles),
> + exbuf->num_bo_handles * sizeof(uint32_t))) {
> + kvfree(bo_handles);
> + return -EFAULT;
> + }
> +
> + submit->buflist = virtio_gpu_array_from_handles(submit->file, bo_handles,
> + exbuf->num_bo_handles);
> + if (!submit->buflist) {
> + kvfree(bo_handles);
> + return -ENOENT;
> + }
> +
> + kvfree(bo_handles);
> +
> + return 0;
> +}
> +
> +static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
> +{
> + if (!IS_ERR(submit->buf))
> + kvfree(submit->buf);
> +
> + if (submit->buflist)
> + virtio_gpu_array_put_free(submit->buflist);
> +
> + if (submit->out_fence_fd >= 0)
> + put_unused_fd(submit->out_fence_fd);
> +
> + if (submit->out_fence)
> + dma_fence_put(&submit->out_fence->f);
> +}
> +
> +static void virtio_gpu_submit(struct virtio_gpu_submit *submit)
> +{
> + virtio_gpu_cmd_submit(submit->vgdev, submit->buf, submit->exbuf->size,
> + submit->vfpriv->ctx_id, submit->buflist,
> + submit->out_fence);
> + virtio_gpu_notify(submit->vgdev);
> +}
> +
> +static void virtio_gpu_complete_submit(struct virtio_gpu_submit *submit)
> +{
> + submit->buf = NULL;
> + submit->buflist = NULL;
> + submit->out_fence = NULL;
> + submit->out_fence_fd = -1;
> +}
> +
> +static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
> + struct drm_virtgpu_execbuffer *exbuf,
> + struct drm_device *dev,
> + struct drm_file *file,
> + uint64_t fence_ctx, uint32_t ring_idx)
> +{
> + struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> + struct virtio_gpu_device *vgdev = dev->dev_private;
> + struct virtio_gpu_fence *out_fence;
> + int err;
> +
> + memset(submit, 0, sizeof(*submit));
> +
> + out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> + if (!out_fence)
> + return -ENOMEM;
> +
> + err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> + if (err) {
> + dma_fence_put(&out_fence->f);
> + return err;
> + }
> +
> + submit->out_fence = out_fence;
> + submit->fence_ctx = fence_ctx;
> + submit->ring_idx = ring_idx;
> + submit->out_fence_fd = -1;
> + submit->vfpriv = vfpriv;
> + submit->vgdev = vgdev;
> + submit->exbuf = exbuf;
> + submit->file = file;
> +
> + err = virtio_gpu_init_submit_buflist(submit);
> + if (err)
> + return err;
> +
> + submit->buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
> + if (IS_ERR(submit->buf))
> + return PTR_ERR(submit->buf);
> +
> + if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
> + err = get_unused_fd_flags(O_CLOEXEC);
> + if (err < 0)
> + return err;
> +
> + submit->out_fence_fd = err;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_wait_in_fence(struct virtio_gpu_submit *submit)
> +{
> + int ret = 0;
> +
> + if (submit->exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
> + struct dma_fence *in_fence =
> + sync_file_get_fence(submit->exbuf->fence_fd);
> + if (!in_fence)
> + return -EINVAL;
> +
> + /*
> + * Wait if the fence is from a foreign context, or if the fence
> + * array contains any fence from a foreign context.
> + */
> + ret = virtio_gpu_dma_fence_wait(submit, in_fence);
> +
> + dma_fence_put(in_fence);
> + }
> +
> + return ret;
> +}
> +
> +static int virtio_gpu_install_out_fence_fd(struct virtio_gpu_submit *submit)
> +{
> + if (submit->out_fence_fd >= 0) {
> + struct sync_file *sync_file =
> + sync_file_create(&submit->out_fence->f);
> + if (!sync_file)
> + return -ENOMEM;
> +
> + submit->exbuf->fence_fd = submit->out_fence_fd;
> + fd_install(submit->out_fence_fd, sync_file->file);
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_lock_buflist(struct virtio_gpu_submit *submit)
> +{
> + if (submit->buflist)
> + return virtio_gpu_array_lock_resv(submit->buflist);
> +
> + return 0;
> +}
> +
> +int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> + struct virtio_gpu_device *vgdev = dev->dev_private;
> + struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> + uint64_t fence_ctx = vgdev->fence_drv.context;
> + struct drm_virtgpu_execbuffer *exbuf = data;
> + struct virtio_gpu_submit submit;
> + uint32_t ring_idx = 0;
> + int ret = -EINVAL;
> +
> + if (vgdev->has_virgl_3d == false)
> + return -ENOSYS;
> +
> + if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
> + return ret;
> +
> + if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX)) {
> + if (exbuf->ring_idx >= vfpriv->num_rings)
> + return ret;
> +
> + if (!vfpriv->base_fence_ctx)
> + return ret;
> +
> + fence_ctx = vfpriv->base_fence_ctx;
> + ring_idx = exbuf->ring_idx;
> + }
> +
> + virtio_gpu_create_context(dev, file);
> +
> + ret = virtio_gpu_init_submit(&submit, exbuf, dev, file,
> + fence_ctx, ring_idx);
> + if (ret)
> + goto cleanup;
> +
> + ret = virtio_gpu_wait_in_fence(&submit);
> + if (ret)
> + goto cleanup;
> +
> + ret = virtio_gpu_install_out_fence_fd(&submit);
> + if (ret)
> + goto cleanup;
> +
> + ret = virtio_gpu_lock_buflist(&submit);
> + if (ret)
> + goto cleanup;
> +
> + virtio_gpu_submit(&submit);
> + virtio_gpu_complete_submit(&submit);
> +cleanup:
> + virtio_gpu_cleanup_submit(&submit);
> +
> + return ret;
> +}
> --
> 2.39.2
>
On Thu, Mar 23, 2023 at 12:05 PM Dmitry Osipenko
<[email protected]> wrote:
>
> Add sync object DRM UAPI support to VirtIO-GPU driver. It's required
> for enabling a full-featured Vulkan fencing by Venus and native context
> VirtIO-GPU Mesa drivers.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 3 +-
> drivers/gpu/drm/virtio/virtgpu_submit.c | 211 ++++++++++++++++++++++++
> include/uapi/drm/virtgpu_drm.h | 16 +-
> 3 files changed, 228 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index add075681e18..a22155577152 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -176,7 +176,8 @@ static const struct drm_driver driver = {
> * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
> * out via drm_device::driver_features:
> */
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC |
> + DRIVER_SYNCOBJ | DRIVER_SYNCOBJ_TIMELINE,
> .open = virtio_gpu_driver_open,
> .postclose = virtio_gpu_driver_postclose,
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index 2ce2459c6bc2..9ea4390948bf 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -14,11 +14,26 @@
> #include <linux/uaccess.h>
>
> #include <drm/drm_file.h>
> +#include <drm/drm_syncobj.h>
> #include <drm/virtgpu_drm.h>
>
> #include "virtgpu_drv.h"
>
> +struct virtio_gpu_submit_post_dep {
> + struct drm_syncobj *syncobj;
> + struct dma_fence_chain *chain;
> + uint64_t point;
> +};
> +
> struct virtio_gpu_submit {
> + struct virtio_gpu_submit_post_dep *post_deps;
> + unsigned int num_out_syncobjs;
> +
> + struct drm_syncobj **in_syncobjs;
> + unsigned int num_in_syncobjs;
> + uint64_t *in_fence_ids;
> + unsigned int num_in_fence_ids;
> +
> struct virtio_gpu_object_array *buflist;
> struct drm_virtgpu_execbuffer *exbuf;
> struct virtio_gpu_fence *out_fence;
> @@ -58,6 +73,186 @@ static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
> return 0;
> }
>
> +static void virtio_gpu_free_syncobjs(struct drm_syncobj **syncobjs,
> + uint32_t nr_syncobjs)
> +{
> + uint32_t i = nr_syncobjs;
> +
> + while (syncobjs && i--) {
Checking syncobjs!=NULL here does at least look a bit funny, as the
condition doesn't change in the loop body. It is not incorrect, it
protects you against the cleanup path where submit->in_syncobjs is
NULL. But if (!syncobjs)\nreturn seems a bit more clear
> + if (syncobjs[i])
> + drm_syncobj_put(syncobjs[i]);
> + }
> +
> + kfree(syncobjs);
> +}
> +
> +static int
> +virtio_gpu_parse_deps(struct virtio_gpu_submit *submit)
> +{
> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
> + size_t syncobj_stride = exbuf->syncobj_stride;
> + struct drm_syncobj **syncobjs;
> + int ret = 0, i;
> +
> + if (!submit->num_in_syncobjs)
> + return 0;
> +
> + syncobjs = kcalloc(submit->num_in_syncobjs, sizeof(*syncobjs),
> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
I *think*, assuming I'm reading where this is called correctly (kinda
wish git would show more lines of context by default) that these don't
need to be NOWARN|NORETRY (same for post_deps). I guess you inherited
this from drm/msm, where I appear to have forgotten to update the
syncobj path in commit f0de40a131d9 ("drm/msm: Reorder lock vs submit
alloc"). I don't see anything obvious that would require NORETRY, but
lockdep should be able to tell you otherwise if needed..
BR,
-R
> + if (!syncobjs)
> + return -ENOMEM;
> +
> + for (i = 0; i < submit->num_in_syncobjs; i++) {
> + uint64_t address = exbuf->in_syncobjs + i * syncobj_stride;
> + struct dma_fence *fence;
> +
> + if (copy_from_user(&syncobj_desc,
> + u64_to_user_ptr(address),
> + min(syncobj_stride, sizeof(syncobj_desc)))) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + if (syncobj_desc.flags & ~VIRTGPU_EXECBUF_SYNCOBJ_FLAGS) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + ret = drm_syncobj_find_fence(submit->file, syncobj_desc.handle,
> + syncobj_desc.point, 0, &fence);
> + if (ret)
> + break;
> +
> + ret = virtio_gpu_dma_fence_wait(submit, fence);
> +
> + dma_fence_put(fence);
> + if (ret)
> + break;
> +
> + if (syncobj_desc.flags & VIRTGPU_EXECBUF_SYNCOBJ_RESET) {
> + syncobjs[i] =
> + drm_syncobj_find(submit->file, syncobj_desc.handle);
> + if (!syncobjs[i]) {
> + ret = -EINVAL;
> + break;
> + }
> + }
> + }
> +
> + if (ret) {
> + virtio_gpu_free_syncobjs(syncobjs, i);
> + return ret;
> + }
> +
> + submit->in_syncobjs = syncobjs;
> +
> + return ret;
> +}
> +
> +static void virtio_gpu_reset_syncobjs(struct drm_syncobj **syncobjs,
> + uint32_t nr_syncobjs)
> +{
> + uint32_t i;
> +
> + for (i = 0; syncobjs && i < nr_syncobjs; i++) {
> + if (syncobjs[i])
> + drm_syncobj_replace_fence(syncobjs[i], NULL);
> + }
> +}
> +
> +static void
> +virtio_gpu_free_post_deps(struct virtio_gpu_submit_post_dep *post_deps,
> + uint32_t nr_syncobjs)
> +{
> + uint32_t i = nr_syncobjs;
> +
> + while (post_deps && i--) {
> + kfree(post_deps[i].chain);
> + drm_syncobj_put(post_deps[i].syncobj);
> + }
> +
> + kfree(post_deps);
> +}
> +
> +static int virtio_gpu_parse_post_deps(struct virtio_gpu_submit *submit)
> +{
> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
> + struct virtio_gpu_submit_post_dep *post_deps;
> + size_t syncobj_stride = exbuf->syncobj_stride;
> + int ret = 0, i;
> +
> + if (!submit->num_out_syncobjs)
> + return 0;
> +
> + post_deps = kcalloc(submit->num_out_syncobjs, sizeof(*post_deps),
> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> + if (!post_deps)
> + return -ENOMEM;
> +
> + for (i = 0; i < submit->num_out_syncobjs; i++) {
> + uint64_t address = exbuf->out_syncobjs + i * syncobj_stride;
> +
> + if (copy_from_user(&syncobj_desc,
> + u64_to_user_ptr(address),
> + min(syncobj_stride, sizeof(syncobj_desc)))) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + post_deps[i].point = syncobj_desc.point;
> +
> + if (syncobj_desc.flags) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (syncobj_desc.point) {
> + post_deps[i].chain = dma_fence_chain_alloc();
> + if (!post_deps[i].chain) {
> + ret = -ENOMEM;
> + break;
> + }
> + }
> +
> + post_deps[i].syncobj =
> + drm_syncobj_find(submit->file, syncobj_desc.handle);
> + if (!post_deps[i].syncobj) {
> + ret = -EINVAL;
> + break;
> + }
> + }
> +
> + if (ret) {
> + virtio_gpu_free_post_deps(post_deps, i);
> + return ret;
> + }
> +
> + submit->post_deps = post_deps;
> +
> + return 0;
> +}
> +
> +static void
> +virtio_gpu_process_post_deps(struct virtio_gpu_submit *submit)
> +{
> + struct virtio_gpu_submit_post_dep *post_deps = submit->post_deps;
> + struct dma_fence *fence = &submit->out_fence->f;
> + uint32_t i;
> +
> + for (i = 0; post_deps && i < submit->num_out_syncobjs; i++) {
> + if (post_deps[i].chain) {
> + drm_syncobj_add_point(post_deps[i].syncobj,
> + post_deps[i].chain,
> + fence, post_deps[i].point);
> + post_deps[i].chain = NULL;
> + } else {
> + drm_syncobj_replace_fence(post_deps[i].syncobj, fence);
> + }
> + }
> +}
> +
> static int virtio_gpu_fence_event_create(struct drm_device *dev,
> struct drm_file *file,
> struct virtio_gpu_fence *fence,
> @@ -121,6 +316,11 @@ static int virtio_gpu_init_submit_buflist(struct virtio_gpu_submit *submit)
>
> static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
> {
> + virtio_gpu_reset_syncobjs(submit->in_syncobjs, submit->num_in_syncobjs);
> + virtio_gpu_free_syncobjs(submit->in_syncobjs, submit->num_in_syncobjs);
> + virtio_gpu_free_post_deps(submit->post_deps, submit->num_out_syncobjs);
> + kfree(submit->in_fence_ids);
> +
> if (!IS_ERR(submit->buf))
> kvfree(submit->buf);
>
> @@ -173,6 +373,8 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
> return err;
> }
>
> + submit->num_out_syncobjs = exbuf->num_out_syncobjs;
> + submit->num_in_syncobjs = exbuf->num_in_syncobjs;
> submit->out_fence = out_fence;
> submit->fence_ctx = fence_ctx;
> submit->ring_idx = ring_idx;
> @@ -285,6 +487,14 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> if (ret)
> goto cleanup;
>
> + ret = virtio_gpu_parse_deps(&submit);
> + if (ret)
> + goto cleanup;
> +
> + ret = virtio_gpu_parse_post_deps(&submit);
> + if (ret)
> + goto cleanup;
> +
> ret = virtio_gpu_install_out_fence_fd(&submit);
> if (ret)
> goto cleanup;
> @@ -294,6 +504,7 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> goto cleanup;
>
> virtio_gpu_submit(&submit);
> + virtio_gpu_process_post_deps(&submit);
> virtio_gpu_complete_submit(&submit);
> cleanup:
> virtio_gpu_cleanup_submit(&submit);
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index 7b158fcb02b4..ce4948aacafd 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -64,6 +64,16 @@ struct drm_virtgpu_map {
> __u32 pad;
> };
>
> +#define VIRTGPU_EXECBUF_SYNCOBJ_RESET 0x01
> +#define VIRTGPU_EXECBUF_SYNCOBJ_FLAGS ( \
> + VIRTGPU_EXECBUF_SYNCOBJ_RESET | \
> + 0)
> +struct drm_virtgpu_execbuffer_syncobj {
> + __u32 handle;
> + __u32 flags;
> + __u64 point;
> +};
> +
> /* fence_fd is modified on success if VIRTGPU_EXECBUF_FENCE_FD_OUT flag is set. */
> struct drm_virtgpu_execbuffer {
> __u32 flags;
> @@ -73,7 +83,11 @@ struct drm_virtgpu_execbuffer {
> __u32 num_bo_handles;
> __s32 fence_fd; /* in/out fence fd (see VIRTGPU_EXECBUF_FENCE_FD_IN/OUT) */
> __u32 ring_idx; /* command ring index (see VIRTGPU_EXECBUF_RING_IDX) */
> - __u32 pad;
> + __u32 syncobj_stride;
> + __u32 num_in_syncobjs;
> + __u32 num_out_syncobjs;
> + __u64 in_syncobjs;
> + __u64 out_syncobjs;
> };
>
> #define VIRTGPU_PARAM_3D_FEATURES 1 /* do we have 3D features in the hw */
> --
> 2.39.2
>
On 3/24/23 00:18, Rob Clark wrote:
...
>> +static int
>> +virtio_gpu_parse_deps(struct virtio_gpu_submit *submit)
>> +{
>> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
>> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
>> + size_t syncobj_stride = exbuf->syncobj_stride;
>> + struct drm_syncobj **syncobjs;
>> + int ret = 0, i;
>> +
>> + if (!submit->num_in_syncobjs)
>> + return 0;
>> +
>> + syncobjs = kcalloc(submit->num_in_syncobjs, sizeof(*syncobjs),
>> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> I *think*, assuming I'm reading where this is called correctly (kinda
> wish git would show more lines of context by default) that these don't
> need to be NOWARN|NORETRY (same for post_deps). I guess you inherited
> this from drm/msm, where I appear to have forgotten to update the
> syncobj path in commit f0de40a131d9 ("drm/msm: Reorder lock vs submit
> alloc"). I don't see anything obvious that would require NORETRY, but
> lockdep should be able to tell you otherwise if needed.
The NORETRY should prevent waking up OOM killer, it shouldn't help with
lockdep. Nothing prevents userspace from giving a big number of
num_in_syncobjs. But perhaps indeed not very practical to care about
this case, given that other similar memalloc paces of execbuffer_ioctl()
aren't using NORETRY. Alright, let's drop it in v4.
--
Best regards,
Dmitry
On 3/23/23 22:03, Dmitry Osipenko wrote:
> struct virtio_gpu_submit {
> + struct virtio_gpu_submit_post_dep *post_deps;
> + unsigned int num_out_syncobjs;
> +
> + struct drm_syncobj **in_syncobjs;
> + unsigned int num_in_syncobjs;
> + uint64_t *in_fence_ids;
> + unsigned int num_in_fence_ids;
The in_fence_ids actually doesn't belong to this patch. Missed to remove
it on a rebase, will fix in v4.
--
Best regards,
Dmitry
On 3/24/23 00:51, Dmitry Osipenko wrote:
> On 3/24/23 00:18, Rob Clark wrote:
> ...
>>> +static int
>>> +virtio_gpu_parse_deps(struct virtio_gpu_submit *submit)
>>> +{
>>> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
>>> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
>>> + size_t syncobj_stride = exbuf->syncobj_stride;
>>> + struct drm_syncobj **syncobjs;
>>> + int ret = 0, i;
>>> +
>>> + if (!submit->num_in_syncobjs)
>>> + return 0;
>>> +
>>> + syncobjs = kcalloc(submit->num_in_syncobjs, sizeof(*syncobjs),
>>> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
>> I *think*, assuming I'm reading where this is called correctly (kinda
>> wish git would show more lines of context by default) that these don't
>> need to be NOWARN|NORETRY (same for post_deps). I guess you inherited
>> this from drm/msm, where I appear to have forgotten to update the
>> syncobj path in commit f0de40a131d9 ("drm/msm: Reorder lock vs submit
>> alloc"). I don't see anything obvious that would require NORETRY, but
>> lockdep should be able to tell you otherwise if needed.
>
> The NORETRY should prevent waking up OOM killer, it shouldn't help with
> lockdep. Nothing prevents userspace from giving a big number of
> num_in_syncobjs. But perhaps indeed not very practical to care about
> this case, given that other similar memalloc paces of execbuffer_ioctl()
> aren't using NORETRY. Alright, let's drop it in v4.
>
Although no, there is only a kvmalloc_array() in the code and vmalloc
uses NOWARN and NORETRY flags implicitly. May be better switch to use
kvmalloc everywhere, for consistency. Technically, vmalloc shouldn't
ever be needed for a submit code path and kmalloc should be enough. On
the other hand, vmalloc acts like kmalloc until there is no enough
contig memory.
--
Best regards,
Dmitry