| Message ID | 1460683781-22535-9-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > 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@collabora.co.uk> > --- > 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. > + 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; }; Sorry if this means a bit a sprawling upfront refactor :( Wrt testcase: Need evil ones that pass invalid pointer to out_fences_ptr or too small array size in count_out_fences and all that ofc. -Daniel > /* > * Unlike commit, check_only does not clean up state. > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 6ed8339..15ba3a8 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -31,6 +31,7 @@ > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> > #include <linux/fence.h> > +#include <linux/sync_file.h> > > /** > * DOC: overview > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 7934178..53e4e71 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -717,6 +717,7 @@ struct drm_crtc_funcs { > * @properties: property tracking for this CRTC > * @state: current atomic state for this CRTC > * @timeline: sync timeline for fence sigalling > + * @fence_seqno: seqno variable to create fences > * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for > * legacy IOCTLs > * > @@ -773,7 +774,9 @@ struct drm_crtc { > > struct drm_crtc_state *state; > > + /* for out-fences */ > struct sync_timeline *timeline; > + unsigned long fence_seqno; > > /* > * For legacy crtc IOCTLs so that atomic drivers can get at the locking > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 39905cc..4cdcd22 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb { > DRM_MODE_ATOMIC_ALLOW_MODESET |\ > DRM_MODE_ATOMIC_OUT_FENCE) > > +struct drm_out_fences { > + __u32 crtc_id; > + __u32 fd; > +}; > + > struct drm_mode_atomic { > __u32 flags; > __u32 count_objs; > @@ -601,6 +606,8 @@ struct drm_mode_atomic { > __u64 prop_values_ptr; > __u64 reserved; > __u64 user_data; > + __u64 count_out_fences; > + __u64 out_fences_ptr; > }; > > /** > -- > 2.5.5 >
2016-04-15 Daniel Vetter <daniel@ffwll.ch>: > On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > 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@collabora.co.uk> > > --- > > 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
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) { + 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) { /* * Unlike commit, check_only does not clean up state. diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6ed8339..15ba3a8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <linux/fence.h> +#include <linux/sync_file.h> /** * DOC: overview diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 7934178..53e4e71 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -717,6 +717,7 @@ struct drm_crtc_funcs { * @properties: property tracking for this CRTC * @state: current atomic state for this CRTC * @timeline: sync timeline for fence sigalling + * @fence_seqno: seqno variable to create fences * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for * legacy IOCTLs * @@ -773,7 +774,9 @@ struct drm_crtc { struct drm_crtc_state *state; + /* for out-fences */ struct sync_timeline *timeline; + unsigned long fence_seqno; /* * For legacy crtc IOCTLs so that atomic drivers can get at the locking diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 39905cc..4cdcd22 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb { DRM_MODE_ATOMIC_ALLOW_MODESET |\ DRM_MODE_ATOMIC_OUT_FENCE) +struct drm_out_fences { + __u32 crtc_id; + __u32 fd; +}; + struct drm_mode_atomic { __u32 flags; __u32 count_objs; @@ -601,6 +606,8 @@ struct drm_mode_atomic { __u64 prop_values_ptr; __u64 reserved; __u64 user_data; + __u64 count_out_fences; + __u64 out_fences_ptr; }; /**