Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754586AbcKPJLT (ORCPT ); Wed, 16 Nov 2016 04:11:19 -0500 Received: from mail.fireflyinternet.com ([109.228.58.192]:56125 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752360AbcKPJLL (ORCPT ); Wed, 16 Nov 2016 04:11:11 -0500 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Date: Wed, 16 Nov 2016 09:10:53 +0000 From: Chris Wilson To: Gustavo Padovan Cc: dri-devel@lists.freedesktop.org, marcheu@google.com, Daniel Stone , seanpaul@google.com, Daniel Vetter , linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com, Gustavo Padovan , John Harrison , m.chehab@samsung.com Subject: Re: [PATCH v12 3/3] drm/fence: add out-fences support Message-ID: <20161116091053.GD24981@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Gustavo Padovan , dri-devel@lists.freedesktop.org, marcheu@google.com, Daniel Stone , seanpaul@google.com, Daniel Vetter , linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com, Gustavo Padovan , John Harrison , m.chehab@samsung.com References: <1479215201-2672-1-git-send-email-gustavo@padovan.org> <1479215201-2672-4-git-send-email-gustavo@padovan.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1479215201-2672-4-git-send-email-gustavo@padovan.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6924 Lines: 208 On Tue, Nov 15, 2016 at 10:06:41PM +0900, Gustavo Padovan wrote: > From: Gustavo Padovan > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > that sets the OUT_FENCE_PTR property. > > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send > the sync_file fd back to userspace. > > The sync_file and fd are allocated/created before commit, but the > fd_install operation only happens after we know that commit succeed. > > v2: Comment by Rob Clark: > - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here. > > Comment by Daniel Vetter: > - Add clean up code for out_fences > > v3: Comments by Daniel Vetter: > - create DRM_MODE_ATOMIC_EVENT_MASK > - userspace should fill out_fences_ptr with the crtc_ids for which > it wants fences back. > > v4: Create OUT_FENCE_PTR properties and remove old approach. > > v5: Comments by Brian Starkey: > - Remove extra fence_get() in atomic_ioctl() > - Check ret before iterating on the crtc_state > - check ret before fd_install > - set fence_state to NULL at the beginning > - check fence_state->out_fence_ptr before put_user() > - change order of fput() and put_unused_fd() on failure > > - Add access_ok() check to the out_fence_ptr received > - Rebase after fence -> dma_fence rename > - Store out_fence_ptr in the drm_atomic_state > - Split crtc_setup_out_fence() > - return -1 as out_fence with TEST_ONLY flag > > v6: Comments by Daniel Vetter > - Add prepare/unprepare_crtc_signaling() > - move struct drm_out_fence_state to drm_atomic.c > - mark get_crtc_fence() as static > > Comments by Brian Starkey > - proper set fence_ptr fence_state array > - isolate fence_idx increment > > - improve error handling > > v7: Comments by Daniel Vetter > - remove prefix from internal functions > - make out_fence_ptr an s64 pointer > - degrade DRM_INFO to DRM_DEBUG_ATOMIC when put_user fail > - fix doc issues > - filter out OUT_FENCE_PTR == NULL and do not fail in this case > - add complete_crtc_signalling() > - krealloc fence_state on demand > > Comment by Brian Starkey > - remove unused crtc_state arg from get_out_fence() > > v8: Comment by Brian Starkey > - cancel events before check for !fence_state > - convert a few lefovers u64 types for out_fence_ptr > - fix memleak by assign fence_state earlier after realloc > - proper accout num_fences in case of error > > v9: Comment by Brian Starkey > - memset last position of fence_state after krealloc > Comments by Sean Paul > - pass install_fds in complete_crtc_signaling() instead of ret > > - put_user(-1, fence_ptr) when decoding props > > v10: Comment by Brian Starkey > - remove unneeded num_fences increment on error path > - kfree fence_state after installing fences fd > > v11: rebase against latest drm-misc > > Signed-off-by: Gustavo Padovan > Reviewed-by: Brian Starkey > Tested-by: Robert Foss > --- > drivers/gpu/drm/drm_atomic.c | 241 +++++++++++++++++++++++++++++++++++-------- > drivers/gpu/drm/drm_crtc.c | 8 ++ > include/drm/drm_atomic.h | 1 + > include/drm/drm_crtc.h | 6 ++ > 4 files changed, 211 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 3ad780a..d4a92a9 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -290,6 +290,23 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, > } > EXPORT_SYMBOL(drm_atomic_get_crtc_state); > > +static void set_out_fence_for_crtc(struct drm_atomic_state *state, > + struct drm_crtc *crtc, s64 __user *fence_ptr) > +{ > + state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr; > +} > + > +static s64 __user * get_out_fence_for_crtc(struct drm_atomic_state *state, > + struct drm_crtc *crtc) > +{ > + s64 __user *fence_ptr; > + > + fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr; > + state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL; > + > + return fence_ptr; > +} > + > /** > * drm_atomic_set_mode_for_crtc - set mode for CRTC > * @state: the CRTC whose incoming state to update > @@ -494,6 +511,16 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > &replaced); > state->color_mgmt_changed |= replaced; > return ret; > + } else if (property == config->prop_out_fence_ptr) { > + s64 __user *fence_ptr = u64_to_user_ptr(val); > + > + if (!fence_ptr) > + return 0; > + > + if (put_user(-1, fence_ptr)) > + return -EFAULT; > + > + set_out_fence_for_crtc(state->state, crtc, fence_ptr); > } else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else > @@ -536,6 +563,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > *val = (state->ctm) ? state->ctm->base.id : 0; > else if (property == config->gamma_lut_property) > *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0; > + else if (property == config->prop_out_fence_ptr) > + *val = 0; > else if (crtc->funcs->atomic_get_property) > return crtc->funcs->atomic_get_property(crtc, state, property, val); > else > @@ -1664,11 +1693,9 @@ int drm_atomic_debugfs_init(struct drm_minor *minor) > */ > > static struct drm_pending_vblank_event *create_vblank_event( > - struct drm_device *dev, struct drm_file *file_priv, > - struct dma_fence *fence, uint64_t user_data) > + struct drm_device *dev, uint64_t user_data) > { > struct drm_pending_vblank_event *e = NULL; > - int ret; > > e = kzalloc(sizeof *e, GFP_KERNEL); > if (!e) > @@ -1678,17 +1705,6 @@ static struct drm_pending_vblank_event *create_vblank_event( > e->event.base.length = sizeof(e->event); > e->event.user_data = user_data; > > - if (file_priv) { > - ret = drm_event_reserve_init(dev, file_priv, &e->base, > - &e->event.base); > - if (ret) { > - kfree(e); > - return NULL; > - } > - } > - > - e->base.fence = fence; > - > return e; > } > > @@ -1793,6 +1809,165 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_clean_old_fb); > > +static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc) > +{ return drm_crtc_fence_create(crtc); (or possibly, drm_crtc_fence_get(), drm_crtc_timeline_advance() or somesuch if we need finer control over fence_seqno) Or if you want to embed it, struct our_fence *fence; fence = kzalloc(sizeof(*fence), GFP_KERNEL); if (!fence) return NULL; drm_crtc_fence_init(crtc, &fence->base); return &fence->base; Looks tidier than dumping all the fence construction here > + dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, > + crtc->fence_context, ++crtc->fence_seqno); -- Chris Wilson, Intel Open Source Technology Centre