[PATCH 2/2] drm/lima: driver for ARM Mali4xx GPUs
Eric Anholt
eric at anholt.net
Wed Feb 6 19:17:18 UTC 2019
Qiang Yu <yuq825 at gmail.com> writes:
> From: Lima Project Developers <lima at lists.freedesktop.org>
>
> Signed-off-by: Andreas Baierl <ichgeh at imkreisrum.de>
> Signed-off-by: Erico Nunes <nunes.erico at gmail.com>
> Signed-off-by: Heiko Stuebner <heiko at sntech.de>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Signed-off-by: Neil Armstrong <narmstrong at baylibre.com>
> Signed-off-by: Qiang Yu <yuq825 at gmail.com>
> Signed-off-by: Simon Shields <simon at lineageos.org>
> Signed-off-by: Vasily Khoruzhick <anarsoul at gmail.com>
> ---
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 <yuq825 at gmail.com> */
> +
> +#include <linux/slab.h>
> +
> +#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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190206/5ca539ff/attachment-0001.sig>
More information about the dri-devel
mailing list