Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752218AbcDOTPw (ORCPT ); Fri, 15 Apr 2016 15:15:52 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:35895 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751562AbcDOTPu (ORCPT ); Fri, 15 Apr 2016 15:15:50 -0400 Date: Fri, 15 Apr 2016 12:15:48 -0700 From: Gustavo Padovan To: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Maarten Lankhorst , Gustavo Padovan Subject: Re: [RFC 8/8] drm/fence: add out-fences support Message-ID: <20160415191548.GF23954@joana> Mail-Followup-To: Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Maarten Lankhorst , Gustavo Padovan References: <1460683781-22535-1-git-send-email-gustavo@padovan.org> <1460683781-22535-9-git-send-email-gustavo@padovan.org> <20160415081808.GU2510@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160415081808.GU2510@phenom.ffwll.local> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6165 Lines: 194 2016-04-15 Daniel Vetter : > On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote: > > From: Gustavo Padovan > > > > 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 > > --- > > 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