On Thu, Feb 7, 2019 at 3:17 AM Eric Anholt <eric@xxxxxxxxxx> wrote:
>
> Qiang Yu <yuq825@xxxxxxxxx> writes:
>
> > From: Lima Project Developers <lima@xxxxxxxxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Andreas Baierl <ichgeh@xxxxxxxxxxxxx>
> > Signed-off-by: Erico Nunes <nunes.erico@xxxxxxxxx>
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > Signed-off-by: Qiang Yu <yuq825@xxxxxxxxx>
> > Signed-off-by: Simon Shields <simon@xxxxxxxxxxxxx>
> > Signed-off-by: Vasily Khoruzhick <anarsoul@xxxxxxxxx>
> > ---
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
