[PATCH 2/2] drm/lima: driver for ARM Mali4xx GPUs

[Qiang Yu]

On Thu, Feb 7, 2019 at 3:17 AM Eric Anholt <eric at anholt.net> wrote:
>
> 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.
Thanks for your kind and valuable suggestion, I'll fix the args check and
left some of suggestions as future improvement.

> > +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.)
timeout_ns in lima is already an absolute one which will be converted to
relative one in lima_timeout_to_jiffies, is this what you want or I miss
understand?

>
> > 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.
Sounds good, I'll consider about this way.

>
> > +#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?
Just for old apps when there's no user space modifier sharing method
like the DRI3 modifiers, like old xserver.

Regards,
Qiang