Re: [RFC 8/8] drm/fence: add out-fences support

From: Gustavo Padovan
Date: Fri Apr 15 2016 - 15:15:54 EST

2016-04-15 Daniel Vetter <daniel@xxxxxxxx>:

> On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> > Support DRM out-fences creating a sync_file with a fence for each crtc
> > update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> >
> > We then send an struct drm_out_fences array with the out-fences fds back in
> > the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> >
> > struct drm_out_fences {
> > __u32 crtc_id;
> > __u32 fd;
> > };
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 109 +++++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/drm_atomic_helper.c | 1 +
> > include/drm/drm_crtc.h | 3 +
> > include/uapi/drm/drm_mode.h | 7 +++
> > 4 files changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 0b95526..af6e051 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> >
> > +static int drm_atomic_get_out_fences(struct drm_device *dev,
> > + struct drm_atomic_state *state,
> > + uint32_t __user *out_fences_ptr,
> > + uint64_t count_out_fences,
> > + uint64_t user_data)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_out_fences *out_fences;
> > + struct sync_file **sync_file;
> > + int num_fences = 0;
> > + int i, ret;
> > +
> > + out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
> > + GFP_KERNEL);
> > + if (!out_fences)
> > + return -ENOMEM;
> > +
> > + sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
> > + GFP_KERNEL);
> > + if (!sync_file) {
> > + kfree(out_fences);
> > + return -ENOMEM;
> > + }
> > +
> > + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > + struct drm_pending_vblank_event *e;
> > + struct fence *fence;
> > + char name[32];
> > + int fd;
> > +
> > + fence = sync_timeline_create_fence(crtc->timeline,
> > + crtc->fence_seqno);
> > + if (!fence) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + snprintf(name, sizeof(name), "crtc-%d_%lu",
> > + drm_crtc_index(crtc), crtc->fence_seqno++);
> > +
> > + sync_file[i] = sync_file_create(name, fence);
> > + if(!sync_file[i]) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + fd = get_unused_fd_flags(O_CLOEXEC);
> > + if (fd < 0) {
> > + ret = fd;
> > + goto out;
> > + }
> > +
> > + sync_file_install(sync_file[i], fd);
> > +
> > + if (crtc_state->event) {
> > + crtc_state->event->base.fence = fence;
> > + } else {
> > + e = create_vblank_event(dev, NULL, fence, user_data);
> > + if (!e) {
> > + put_unused_fd(fd);
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + crtc_state->event = e;
> > + }
> > + if (num_fences > count_out_fences) {
> > + put_unused_fd(fd);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + fence_get(fence);
> > +
> > + out_fences[num_fences].crtc_id = crtc->base.id;
> > + out_fences[num_fences].fd = fd;
> > + num_fences++;
> > + }
> > +
> > + if (copy_to_user(out_fences_ptr, out_fences,
> > + num_fences * sizeof(*out_fences))) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + return 0;
> > +
> > +out:
> > + for (i = 0 ; i < count_out_fences ; i++) {
> > + if (sync_file[i])
> > + sync_file_put(sync_file[i]);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > int drm_mode_atomic_ioctl(struct drm_device *dev,
> > void *data, struct drm_file *file_priv)
> > {
> > @@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> > uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> > uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> > uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> > + uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
> > unsigned int copied_objs, copied_props;
> > struct drm_atomic_state *state;
> > struct drm_modeset_acquire_ctx ctx;
> > @@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >
> > /* can't test and expect an event at the same time. */
> > if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> > - (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> > + (arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
> > + | DRM_MODE_ATOMIC_OUT_FENCE)))
> > return -EINVAL;
> >
> > drm_modeset_acquire_init(&ctx, 0);
> > @@ -1693,6 +1792,14 @@ retry:
> > }
> > }
> >
> > + if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
>
> OUT_FENCE and TEST_ONLY probably don't make sense, and need to be
> rejected. Needs a testcase, too.

I've added the check for this above. But a test case is still missing.

>
> > + ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
> > + arg->count_out_fences,
> > + arg->user_data);
> > + if (ret < 0)
> > + goto out;
> > + }
> > +
> > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>
> If anything fails below this point we need to clean up the sync_file/fd
> mess. Might be easier to first create sync_file objects only, and only
> install the fd once atomic has succeeded. You probably want to reserve the
> fd slots beforehand though.
>
> That means a bunch more per-crtc state in drm_atomic_state. We should
> probably take all the per-crtc pointers and throw them into a small
> struct, to avoid allocating individual arrays for everything. So
>
> struct drm_atomic_state_per_crtc {
> struct drm_crtc *crtc;
> struct drm_crtc_state *state;
> struct sync_file *sync_file;
> int fd;
> };

That is good idea. I've left the clean up out for this RFC because I
didn't had any good approach on how to do it.

Thanks for this suggestion and all the other comments in the patches.
They were really helpful to improve this work.

Gustavo