| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160415191548.GF23954@joana>
Date: Fri, 15 Apr 2016 12:15:48 -0700
From: Gustavo Padovan <gustavo@...ovan.org>
To: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Daniel Stone <daniels@...labora.com>,
Arve Hjønnevåg <arve@...roid.com>,
Riley Andrews <riandrews@...roid.com>,
Rob Clark <robdclark@...il.com>,
Greg Hackmann <ghackmann@...gle.com>,
John Harrison <John.C.Harrison@...el.com>,
laurent.pinchart@...asonboard.com, seanpaul@...gle.com,
marcheu@...gle.com, m.chehab@...sung.com,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Gustavo Padovan <gustavo.padovan@...labora.co.uk>
Subject: Re: [RFC 8/8] drm/fence: add out-fences support
2016-04-15 Daniel Vetter <daniel@...ll.ch>:
> On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@...labora.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@...labora.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
Powered by blists - more mailing lists