Qiang Yu writes: > From: Lima Project Developers > > Signed-off-by: Andreas Baierl > Signed-off-by: Erico Nunes > Signed-off-by: Heiko Stuebner > Signed-off-by: Marek Vasut > Signed-off-by: Neil Armstrong > Signed-off-by: Qiang Yu > Signed-off-by: Simon Shields > Signed-off-by: Vasily Khoruzhick > --- Some comments to follow. Of them, the integer overflow and flags checks definitely need fixing, I strongly recommend changing your timeout handling, and would not block on any of my other suggestions. > diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c > new file mode 100644 > index 000000000000..724ac4051f7a > --- /dev/null > +++ b/drivers/gpu/drm/lima/lima_ctx.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > +/* Copyright 2018 Qiang Yu */ > + > +#include > + > +#include "lima_device.h" > +#include "lima_ctx.h" > + > +int lima_ctx_create(struct lima_device *dev, struct lima_ctx_mgr *mgr, u32 *id) > +{ > + struct lima_ctx *ctx; > + int i, err; > + > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + ctx->dev = dev; > + kref_init(&ctx->refcnt); > + > + for (i = 0; i < lima_pipe_num; i++) { > + err = lima_sched_context_init(dev->pipe + i, ctx->context + i, &ctx->guilty); > + if (err) > + goto err_out0; > + } > + > + idr_preload(GFP_KERNEL); > + spin_lock(&mgr->lock); > + err = idr_alloc(&mgr->handles, ctx, 1, 0, GFP_ATOMIC); > + spin_unlock(&mgr->lock); > + idr_preload_end(); > + if (err < 0) > + goto err_out0; You might enjoy using the new xa_alloc() api instead of idrs. > +static int lima_ioctl_gem_submit(struct drm_device *dev, void *data, struct drm_file *file) > +{ > + struct drm_lima_gem_submit_in *args = data; > + struct lima_device *ldev = to_lima_dev(dev); > + struct lima_drm_priv *priv = file->driver_priv; > + struct drm_lima_gem_submit_bo *bos; > + struct ttm_validate_buffer *vbs; > + union drm_lima_gem_submit_dep *deps = NULL; > + struct lima_sched_pipe *pipe; > + struct lima_sched_task *task; > + struct lima_ctx *ctx; > + struct lima_submit submit = {0}; > + int err = 0, size; > + > + if (args->pipe >= lima_pipe_num || args->nr_bos == 0) > + return -EINVAL; > + > + if (args->flags & ~(LIMA_SUBMIT_FLAG_EXPLICIT_FENCE | > + LIMA_SUBMIT_FLAG_SYNC_FD_OUT)) > + return -EINVAL; > + > + pipe = ldev->pipe + args->pipe; > + if (args->frame_size != pipe->frame_size) > + return -EINVAL; > + > + size = args->nr_bos * (sizeof(*submit.bos) + sizeof(*submit.vbs)) + > + args->nr_deps * sizeof(*submit.deps); Needs checking for integer overflow with user-submitted args here. (Having done overflow math for the equivalent in vc4, I'd say: don't bother, do more kmallocs.) > + bos = kzalloc(size, GFP_KERNEL); > + if (!bos) > + return -ENOMEM; > + > + size = args->nr_bos * sizeof(*submit.bos); > + if (copy_from_user(bos, u64_to_user_ptr(args->bos), size)) { > + err = -EFAULT; > + goto out0; > + } > + > + vbs = (void *)bos + size; > + > + if (args->nr_deps) { > + deps = (void *)vbs + args->nr_bos * sizeof(*submit.vbs); > + size = args->nr_deps * sizeof(*submit.deps); > + if (copy_from_user(deps, u64_to_user_ptr(args->deps), size)) { > + err = -EFAULT; > + goto out0; > + } > + } > +static int lima_ioctl_gem_wait(struct drm_device *dev, void *data, struct drm_file *file) > +{ > + struct drm_lima_gem_wait *args = data; > + > + if (!(args->op & (LIMA_GEM_WAIT_READ|LIMA_GEM_WAIT_WRITE))) > + return -EINVAL; I think you want if (args->op & ~(LIMA_GEM_WAIT_READ|LIMA_GEM_WAIT_WRITE)) so that you can be sure userspace is doing the right thing today so that you can extend with other flags in the future. > +static int lima_ioctl_ctx(struct drm_device *dev, void *data, struct drm_file *file) > +{ > + struct drm_lima_ctx *args = data; > + struct lima_drm_priv *priv = file->driver_priv; > + struct lima_device *ldev = to_lima_dev(dev); > + > + if (args->op == LIMA_CTX_OP_CREATE) > + return lima_ctx_create(ldev, &priv->ctx_mgr, &args->id); > + else if (args->op == LIMA_CTX_OP_FREE) > + return lima_ctx_free(&priv->ctx_mgr, args->id); > + > + return -EINVAL; > +} Overall UAPI suggestion: Having these muxing ioctls means that your debug logs are harder to parse. ioctls already are already a mux based on the command number, so just have separate ctx_create/ctx_free, gem_get/gem_set ioctls, etc. > +static int lima_drm_driver_open(struct drm_device *dev, struct drm_file *file) > +{ > + int err; > + struct lima_drm_priv *priv; > + struct lima_device *ldev = to_lima_dev(dev); > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->vm = lima_vm_create(ldev); > + if (!priv->vm) { > + err = -ENOMEM; > + goto err_out0; > + } > + > + lima_ctx_mgr_init(&priv->ctx_mgr); whitespace issue > +int lima_gem_wait(struct drm_file *file, u32 handle, u32 op, u64 timeout_ns) > +{ > + bool write = op & LIMA_GEM_WAIT_WRITE; > + struct drm_gem_object *obj; > + struct lima_bo *bo; > + signed long ret; > + unsigned long timeout; > + > + obj = drm_gem_object_lookup(file, handle); > + if (!obj) > + return -ENOENT; > + > + bo = to_lima_bo(obj); > + > + timeout = timeout_ns ? lima_timeout_to_jiffies(timeout_ns) : 0; > + > + ret = lima_bo_reserve(bo, true); > + if (ret) > + goto out; > + > + /* must use long for result check because in 64bit arch int > + * will overflow if timeout is too large and get <0 result > + */ > + ret = reservation_object_wait_timeout_rcu(bo->tbo.resv, write, true, timeout); > + if (ret == 0) > + ret = timeout ? -ETIMEDOUT : -EBUSY; > + else if (ret > 0) > + ret = 0; > + > + lima_bo_unreserve(bo); > +out: > + drm_gem_object_put_unlocked(obj); > + return ret; > +} From Documentation/botching-up-ioctls.txt: * For timeouts, use absolute times. If you're a good fellow and made your ioctl restartable relative timeouts tend to be too coarse and can indefinitely extend your wait time due to rounding on each restart. Especially if your reference clock is something really slow like the display frame counter. With a spec lawyer hat on this isn't a bug since timeouts can always be extended - but users will surely hate you if their neat animations starts to stutter due to this. (I made v3d's timeouts relative, but decrement the timeout value the user passed by how much I waited so that the timeout probably gets reduced after a restartable signal. I should have done absolute.) > diff --git a/include/uapi/drm/lima_drm.h b/include/uapi/drm/lima_drm.h > new file mode 100644 > index 000000000000..c44757b4be39 > --- /dev/null > +++ b/include/uapi/drm/lima_drm.h > + > +#define LIMA_SUBMIT_DEP_FENCE 0x00 > +#define LIMA_SUBMIT_DEP_SYNC_FD 0x01 > + > +struct drm_lima_gem_submit_dep_fence { > + __u32 type; > + __u32 ctx; > + __u32 pipe; > + __u32 seq; > +}; > + > +struct drm_lima_gem_submit_dep_sync_fd { > + __u32 type; > + __u32 fd; > +}; > + > +union drm_lima_gem_submit_dep { > + __u32 type; > + struct drm_lima_gem_submit_dep_fence fence; > + struct drm_lima_gem_submit_dep_sync_fd sync_fd; > +}; I've been using gem sync objects for exposing my fences in v3d. You can import/export fences from sync files into syncobjs, and then you don't need a custom driver fence type in the uapi or your own ioctls for it if the submit just takes syncobjs in and out. > +#define LIMA_GEM_MOD_OP_GET 0 > +#define LIMA_GEM_MOD_OP_SET 1 > + > +struct drm_lima_gem_mod { > + __u32 handle; /* in */ > + __u32 op; /* in */ > + __u64 modifier; /* in/out */ > +}; I thought the whole idea with the DRI3 modifiers stuff was that the kernel didn't need to store modifier metadata on buffers? (And this gets in the way of Vulkan modifiers support, from what I understand). Do you actually need this ABI?