| Message ID | 1476975005-30441-5-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > that sets the OUT_FENCE_PTR property. I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty. > > 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. > > In case of error userspace should receive a fd number of -1. > > 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. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- > drivers/gpu/drm/drm_crtc.c | 8 +++ > include/drm/drm_crtc.h | 19 +++++++ > 3 files changed, 119 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index b3284b2..07775fc 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -490,6 +490,8 @@ 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) { > + state->out_fence_ptr = u64_to_user_ptr(val); > } else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else > @@ -532,6 +534,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 > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > */ > > static struct drm_pending_vblank_event *create_vblank_event( > - struct drm_device *dev, struct drm_file *file_priv, > - struct 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) > @@ -1488,17 +1490,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; > } > > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_clean_old_fb); > > +static int crtc_setup_out_fence(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state, > + struct drm_out_fence_state *fence_state) > +{ > + struct fence *fence; > + > + fence_state->fd = get_unused_fd_flags(O_CLOEXEC); > + if (fence_state->fd < 0) { > + return fence_state->fd; > + } > + > + fence_state->out_fence_ptr = crtc_state->out_fence_ptr; > + crtc_state->out_fence_ptr = NULL; > + > + if (put_user(fence_state->fd, fence_state->out_fence_ptr)) > + return -EFAULT; > + > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (!fence) > + return -ENOMEM; > + > + fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, > + crtc->fence_context, ++crtc->fence_seqno); > + > + fence_state->sync_file = sync_file_create(fence); > + if(!fence_state->sync_file) { > + fence_put(fence); > + return -ENOMEM; > + } > + > + crtc_state->event->base.fence = fence_get(fence); > + return 0; > +} > + > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { > @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > struct drm_plane *plane; > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > + struct drm_out_fence_state *fence_state; > unsigned plane_mask; > int ret = 0; > - unsigned int i, j; > + unsigned int i, j, fence_idx = 0; > > /* disallow for drivers not supporting atomic: */ > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) > @@ -1734,12 +1760,19 @@ retry: > drm_mode_object_unreference(obj); > } > > - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > + fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), > + GFP_KERNEL); > + if (!fence_state) { > + ret = -ENOMEM; > + goto out; > + } > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || > + crtc_state->out_fence_ptr) { > struct drm_pending_vblank_event *e; > > - e = create_vblank_event(dev, file_priv, NULL, > - arg->user_data); > + e = create_vblank_event(dev, arg->user_data); > if (!e) { > ret = -ENOMEM; > goto out; > @@ -1747,6 +1780,28 @@ retry: > > crtc_state->event = e; > } > + > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > + struct drm_pending_vblank_event *e = crtc_state->event; > + > + if (!file_priv) > + continue; > + > + ret = drm_event_reserve_init(dev, file_priv, &e->base, > + &e->event.base); > + if (ret) { > + kfree(e); > + crtc_state->event = NULL; > + goto out; > + } > + } > + > + if (crtc_state->out_fence_ptr) { > + ret = crtc_setup_out_fence(crtc, crtc_state, > + &fence_state[fence_idx++]); > + if (ret) > + goto out; > + } > } > > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { > @@ -1761,24 +1816,37 @@ retry: > ret = drm_atomic_commit(state); > } > > + for (i = 0; i < fence_idx; i++) > + fd_install(fence_state[i].fd, fence_state[i].sync_file->file); > + > out: > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > /* > * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, > * if they weren't, this code should be called on success > * for TEST_ONLY too. > */ > + if (ret && crtc_state->event) > + drm_event_cancel_free(dev, &crtc_state->event->base); > + } > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > - if (!crtc_state->event) > - continue; > + if (ret && fence_state) { > + for (i = 0; i < fence_idx; i++) { > + if (fence_state[i].fd >= 0) > + put_unused_fd(fence_state[i].fd); > + if (fence_state[i].sync_file) > + fput(fence_state[i].sync_file->file); > > - drm_event_cancel_free(dev, &crtc_state->event->base); > + /* If this fails, there's not much we can do about it */ > + if (put_user(-1, fence_state->out_fence_ptr)) > + DRM_ERROR("Couldn't clear out_fence_ptr\n"); > } > } > > + kfree(fence_state); > + > if (ret == -EDEADLK) { > drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b99090f..40bce97 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { > drm_object_attach_property(&crtc->base, config->prop_active, 0); > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); > + drm_object_attach_property(&crtc->base, > + config->prop_out_fence_ptr, 0); > } > > return 0; > @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_in_fence_fd = prop; > > + prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, > + "OUT_FENCE_PTR", 0, U64_MAX); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_out_fence_ptr = prop; > + > prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, > "CRTC_ID", DRM_MODE_OBJECT_CRTC); > if (!prop) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 657a33a..b898604 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -165,6 +165,8 @@ struct drm_crtc_state { > struct drm_property_blob *ctm; > struct drm_property_blob *gamma_lut; > > + u64 __user *out_fence_ptr; > + > /** > * @event: > * > @@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) > } > > /** > + * struct drm_out_fence_state - store out_fence data for install and clean up > + * @out_fence_ptr: user pointer to store the fence fd in. > + * @sync_file: sync_file related to the out_fence > + * @fd: file descriptor to installed on the sync_file. > + */ > +struct drm_out_fence_state { > + u64 __user *out_fence_ptr; > + struct sync_file *sync_file; > + int fd; > +}; > + > +/* > * struct drm_mode_set - new values for a CRTC config change > * @fb: framebuffer to use for new config > * @crtc: CRTC whose configuration we're about to change > @@ -1230,6 +1244,11 @@ struct drm_mode_config { > */ > struct drm_property *prop_in_fence_fd; > /** > + * @prop_out_fence_ptr: Sync File fd pointer representing the > + * outgoing fences for a CRTC. > + */ > + struct drm_property *prop_out_fence_ptr; > + /** > * @prop_crtc_id: Default atomic plane property to specify the > * &drm_crtc. > */ > -- > 2.5.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > > that sets the OUT_FENCE_PTR property. > > I still maintain the out fence should also be per fb (well, per plane > since we can't add it to fbs). Otherwise it's not going to be at all > useful if you do fps>vrefresh and don't include the same set of planes > in every atomic ioctl, eg. if you only ever include ones that are > somehow dirty. How would the kernel signal these dirty planes then? Right now we signal at the vblank. Gustavo
On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote: > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > > > that sets the OUT_FENCE_PTR property. > > > > I still maintain the out fence should also be per fb (well, per plane > > since we can't add it to fbs). Otherwise it's not going to be at all > > useful if you do fps>vrefresh and don't include the same set of planes > > in every atomic ioctl, eg. if you only ever include ones that are > > somehow dirty. > > How would the kernel signal these dirty planes then? Right now we signal > at the vblank. So if I think about it in terms of the previous fbs something like this comes to mind: starting point: plane a, fb 0 plane b, fb 1 ioctl: plane a, fb 2, fence A plane b, fb 3, fence B ioctl: plane a, fb 4, fence C -> fb 2 can be reused, so fence C should signal immediately? vblank: -> fb 0,1 can be reused, so fence A,B signal? It feels a bit wonky since the fence is effectively associated with the previous fb after the previous fb was already submitted to the kernel. One might assume fence A to be the one signalled early, but that would give the impression that fb 0 would be free for reuse when it's not.
Hi Gustavo, I notice your branch has the sync_file refcount change in, but this doesn't seem to take account for that. Will you be dropping that change to match the semantics of fence_array? Couple more comments below. On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >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. > >In case of error userspace should receive a fd number of -1. > >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. > >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >--- > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- > drivers/gpu/drm/drm_crtc.c | 8 +++ > include/drm/drm_crtc.h | 19 +++++++ > 3 files changed, 119 insertions(+), 24 deletions(-) > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >index b3284b2..07775fc 100644 >--- a/drivers/gpu/drm/drm_atomic.c >+++ b/drivers/gpu/drm/drm_atomic.c >@@ -490,6 +490,8 @@ 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) { >+ state->out_fence_ptr = u64_to_user_ptr(val); > } else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else >@@ -532,6 +534,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 >@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > */ > > static struct drm_pending_vblank_event *create_vblank_event( >- struct drm_device *dev, struct drm_file *file_priv, >- struct 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) >@@ -1488,17 +1490,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; > } > >@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_clean_old_fb); > >+static int crtc_setup_out_fence(struct drm_crtc *crtc, >+ struct drm_crtc_state *crtc_state, >+ struct drm_out_fence_state *fence_state) >+{ >+ struct fence *fence; >+ >+ fence_state->fd = get_unused_fd_flags(O_CLOEXEC); >+ if (fence_state->fd < 0) { >+ return fence_state->fd; >+ } >+ >+ fence_state->out_fence_ptr = crtc_state->out_fence_ptr; >+ crtc_state->out_fence_ptr = NULL; >+ >+ if (put_user(fence_state->fd, fence_state->out_fence_ptr)) >+ return -EFAULT; >+ >+ fence = kzalloc(sizeof(*fence), GFP_KERNEL); >+ if (!fence) >+ return -ENOMEM; >+ >+ fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, >+ crtc->fence_context, ++crtc->fence_seqno); >+ >+ fence_state->sync_file = sync_file_create(fence); >+ if(!fence_state->sync_file) { >+ fence_put(fence); >+ return -ENOMEM; >+ } >+ >+ crtc_state->event->base.fence = fence_get(fence); >+ return 0; >+} >+ > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { >@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > struct drm_plane *plane; > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; >+ struct drm_out_fence_state *fence_state; > unsigned plane_mask; > int ret = 0; >- unsigned int i, j; >+ unsigned int i, j, fence_idx = 0; > > /* disallow for drivers not supporting atomic: */ > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) >@@ -1734,12 +1760,19 @@ retry: > drm_mode_object_unreference(obj); > } > >- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >- for_each_crtc_in_state(state, crtc, crtc_state, i) { >+ fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), >+ GFP_KERNEL); >+ if (!fence_state) { >+ ret = -ENOMEM; >+ goto out; >+ } >+ >+ for_each_crtc_in_state(state, crtc, crtc_state, i) { >+ if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || >+ crtc_state->out_fence_ptr) { > struct drm_pending_vblank_event *e; > >- e = create_vblank_event(dev, file_priv, NULL, >- arg->user_data); >+ e = create_vblank_event(dev, arg->user_data); > if (!e) { > ret = -ENOMEM; > goto out; >@@ -1747,6 +1780,28 @@ retry: > > crtc_state->event = e; > } >+ >+ if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >+ struct drm_pending_vblank_event *e = crtc_state->event; >+ >+ if (!file_priv) >+ continue; >+ >+ ret = drm_event_reserve_init(dev, file_priv, &e->base, >+ &e->event.base); >+ if (ret) { >+ kfree(e); >+ crtc_state->event = NULL; >+ goto out; >+ } >+ } >+ >+ if (crtc_state->out_fence_ptr) { >+ ret = crtc_setup_out_fence(crtc, crtc_state, >+ &fence_state[fence_idx++]); >+ if (ret) >+ goto out; >+ } > } > > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { >@@ -1761,24 +1816,37 @@ retry: > ret = drm_atomic_commit(state); > } > >+ for (i = 0; i < fence_idx; i++) >+ fd_install(fence_state[i].fd, fence_state[i].sync_file->file); >+ > out: > drm_atomic_clean_old_fb(dev, plane_mask, ret); > >- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >+ for_each_crtc_in_state(state, crtc, crtc_state, i) { I think a check on ret is still needed before you iterate the CRTCs. If the commit is successful then state has already been freed by this point, and I get a splat. > /* > * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, > * if they weren't, this code should be called on success > * for TEST_ONLY too. > */ >+ if (ret && crtc_state->event) >+ drm_event_cancel_free(dev, &crtc_state->event->base); >+ } > >- for_each_crtc_in_state(state, crtc, crtc_state, i) { >- if (!crtc_state->event) >- continue; >+ if (ret && fence_state) { >+ for (i = 0; i < fence_idx; i++) { >+ if (fence_state[i].fd >= 0) >+ put_unused_fd(fence_state[i].fd); >+ if (fence_state[i].sync_file) >+ fput(fence_state[i].sync_file->file); > >- drm_event_cancel_free(dev, &crtc_state->event->base); >+ /* If this fails, there's not much we can do about it */ >+ if (put_user(-1, fence_state->out_fence_ptr)) >+ DRM_ERROR("Couldn't clear out_fence_ptr\n"); > } > } > >+ kfree(fence_state); >+ > if (ret == -EDEADLK) { > drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); >diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >index b99090f..40bce97 100644 >--- a/drivers/gpu/drm/drm_crtc.c >+++ b/drivers/gpu/drm/drm_crtc.c >@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { > drm_object_attach_property(&crtc->base, config->prop_active, 0); > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); >+ drm_object_attach_property(&crtc->base, >+ config->prop_out_fence_ptr, 0); > } > > return 0; >@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_in_fence_fd = prop; > >+ prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, >+ "OUT_FENCE_PTR", 0, U64_MAX); >+ if (!prop) >+ return -ENOMEM; >+ dev->mode_config.prop_out_fence_ptr = prop; >+ > prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, > "CRTC_ID", DRM_MODE_OBJECT_CRTC); > if (!prop) >diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >index 657a33a..b898604 100644 >--- a/include/drm/drm_crtc.h >+++ b/include/drm/drm_crtc.h >@@ -165,6 +165,8 @@ struct drm_crtc_state { > struct drm_property_blob *ctm; > struct drm_property_blob *gamma_lut; > >+ u64 __user *out_fence_ptr; >+ I'm somewhat not convinced about stashing a __user pointer in the CRTC atomic state. I know it gets cleared before the driver sees it, but if anything I think that hints that this isn't the right place to store it. I've been trying to think of other ways to get from a property to something drm_mode_atomic_ioctl can use - maybe we can store some stuff in drm_atomic_state which only lives for the length of the ioctl call and put this pointer in there. Thanks, Brian > /** > * @event: > * >@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) > } > > /** >+ * struct drm_out_fence_state - store out_fence data for install and clean up >+ * @out_fence_ptr: user pointer to store the fence fd in. >+ * @sync_file: sync_file related to the out_fence >+ * @fd: file descriptor to installed on the sync_file. >+ */ >+struct drm_out_fence_state { >+ u64 __user *out_fence_ptr; >+ struct sync_file *sync_file; >+ int fd; >+}; >+ >+/* > * struct drm_mode_set - new values for a CRTC config change > * @fb: framebuffer to use for new config > * @crtc: CRTC whose configuration we're about to change >@@ -1230,6 +1244,11 @@ struct drm_mode_config { > */ > struct drm_property *prop_in_fence_fd; > /** >+ * @prop_out_fence_ptr: Sync File fd pointer representing the >+ * outgoing fences for a CRTC. >+ */ >+ struct drm_property *prop_out_fence_ptr; >+ /** > * @prop_crtc_id: Default atomic plane property to specify the > * &drm_crtc. > */ >-- >2.5.5 > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote: > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote: > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > > > > that sets the OUT_FENCE_PTR property. > > > > > > I still maintain the out fence should also be per fb (well, per plane > > > since we can't add it to fbs). Otherwise it's not going to be at all > > > useful if you do fps>vrefresh and don't include the same set of planes > > > in every atomic ioctl, eg. if you only ever include ones that are > > > somehow dirty. > > > > How would the kernel signal these dirty planes then? Right now we signal > > at the vblank. > > So if I think about it in terms of the previous fbs something like this > comes to mind: > > starting point: > plane a, fb 0 > plane b, fb 1 > > ioctl: > plane a, fb 2, fence A > plane b, fb 3, fence B > > ioctl: > plane a, fb 4, fence C > -> fb 2 can be reused, so fence C should signal immediately? > > vblank: > -> fb 0,1 can be reused, so fence A,B signal? > > It feels a bit wonky since the fence is effectively associated with the > previous fb after the previous fb was already submitted to the kernel. > One might assume fence A to be the one signalled early, but that would > give the impression that fb 0 would be free for reuse when it's not. OK, so after hashing this out on irc with Gustavo and Brian, we came to the conclusion that the per-crtc out fence should be sufficient after all. The way it can work is that the first ioctl will return the fence, doesn't really matter how many of the planes are involved in this ioctl. Subsequent ioctls that manage to get in before the next vblank will get back a -1 as the fence, even if the set if planes involved is not the same as in the first ioctl. From the -1 userspace can tell what happened, and can then consult its own records to see which still pending flips were overwritten, and thus knows which buffers can be reused immediately. If userspace plans on sending out the now free buffers to someone else accompanied by a fence, it can just create an already signalled fence to send instead of sending the fence it got back from the atomic ioctl since that one is still associated with the original scanout buffers. The fence returned by the atomic ioctl will only signal after the vblank, at which point userspace will obviously know that the orginal scanout buffers are now also ready for reuse, and that the last buffer submitted for each plane is now being actively scanned out. So if userspace wants to send out some of the original buffers to someone else, it can send along the fence returned from the atomic ioctl. So requires a bit more work from userspace perhaps. But obviously it only has to do this if it plans on submitting multiple operations per frame. So a slightly expanded version of my previous example might look like: starting point: plane a, fb 0 plane b, fb 1 ioctl: crtc: fence A plane a, fb 2 plane b, fb 3 ioctl: crtc: fence -1 plane a, fb 4 -> the previously submitted fb for plane a (fb 2) can be reused ioctl: crtc: fence -1 plane a, fb 5 -> the previously submitted fb for plane a (fb 4) can be reused vblank: -> fence A signals, fb 0,1 can be reused fb 3 and 5 being scanned out now We also had a quick side track w.r.t. variable refresh rate displays, and I think the conclusion was that there the vblank may just happen sooner or later. Nothing else should change. What's unclear is how the refresh rate would be controlled. The two obvious options are explicit control, and automagic based on the submit rate. But that's a separate topic entirely.
Hi Brian, 2016-10-20 Brian Starkey <brian.starkey@arm.com>: > Hi Gustavo, > > I notice your branch has the sync_file refcount change in, but this > doesn't seem to take account for that. Will you be dropping that > change to match the semantics of fence_array? I will drop the fence_get() in the out-fence patch because we already hold the ref when we create the fence. The sync_file refcount patch will remain. > > Couple more comments below. > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > 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. > > > > In case of error userspace should receive a fd number of -1. > > > > 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. > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- > > drivers/gpu/drm/drm_crtc.c | 8 +++ > > include/drm/drm_crtc.h | 19 +++++++ > > 3 files changed, 119 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index b3284b2..07775fc 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -490,6 +490,8 @@ 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) { > > + state->out_fence_ptr = u64_to_user_ptr(val); > > } else if (crtc->funcs->atomic_set_property) > > return crtc->funcs->atomic_set_property(crtc, state, property, val); > > else > > @@ -532,6 +534,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 > > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > > */ > > > > static struct drm_pending_vblank_event *create_vblank_event( > > - struct drm_device *dev, struct drm_file *file_priv, > > - struct 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) > > @@ -1488,17 +1490,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; > > } > > > > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_atomic_clean_old_fb); > > > > +static int crtc_setup_out_fence(struct drm_crtc *crtc, > > + struct drm_crtc_state *crtc_state, > > + struct drm_out_fence_state *fence_state) > > +{ > > + struct fence *fence; > > + > > + fence_state->fd = get_unused_fd_flags(O_CLOEXEC); > > + if (fence_state->fd < 0) { > > + return fence_state->fd; > > + } > > + > > + fence_state->out_fence_ptr = crtc_state->out_fence_ptr; > > + crtc_state->out_fence_ptr = NULL; > > + > > + if (put_user(fence_state->fd, fence_state->out_fence_ptr)) > > + return -EFAULT; > > + > > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > > + if (!fence) > > + return -ENOMEM; > > + > > + fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, > > + crtc->fence_context, ++crtc->fence_seqno); > > + > > + fence_state->sync_file = sync_file_create(fence); > > + if(!fence_state->sync_file) { > > + fence_put(fence); > > + return -ENOMEM; > > + } > > + > > + crtc_state->event->base.fence = fence_get(fence); > > + return 0; > > +} > > + > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > void *data, struct drm_file *file_priv) > > { > > @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > struct drm_plane *plane; > > struct drm_crtc *crtc; > > struct drm_crtc_state *crtc_state; > > + struct drm_out_fence_state *fence_state; > > unsigned plane_mask; > > int ret = 0; > > - unsigned int i, j; > > + unsigned int i, j, fence_idx = 0; > > > > /* disallow for drivers not supporting atomic: */ > > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) > > @@ -1734,12 +1760,19 @@ retry: > > drm_mode_object_unreference(obj); > > } > > > > - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), > > + GFP_KERNEL); > > + if (!fence_state) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || > > + crtc_state->out_fence_ptr) { > > struct drm_pending_vblank_event *e; > > > > - e = create_vblank_event(dev, file_priv, NULL, > > - arg->user_data); > > + e = create_vblank_event(dev, arg->user_data); > > if (!e) { > > ret = -ENOMEM; > > goto out; > > @@ -1747,6 +1780,28 @@ retry: > > > > crtc_state->event = e; > > } > > + > > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > > + struct drm_pending_vblank_event *e = crtc_state->event; > > + > > + if (!file_priv) > > + continue; > > + > > + ret = drm_event_reserve_init(dev, file_priv, &e->base, > > + &e->event.base); > > + if (ret) { > > + kfree(e); > > + crtc_state->event = NULL; > > + goto out; > > + } > > + } > > + > > + if (crtc_state->out_fence_ptr) { > > + ret = crtc_setup_out_fence(crtc, crtc_state, > > + &fence_state[fence_idx++]); > > + if (ret) > > + goto out; > > + } > > } > > > > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { > > @@ -1761,24 +1816,37 @@ retry: > > ret = drm_atomic_commit(state); > > } > > > > + for (i = 0; i < fence_idx; i++) > > + fd_install(fence_state[i].fd, fence_state[i].sync_file->file); > > + > > out: > > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > > > - if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > I think a check on ret is still needed before you iterate the CRTCs. > If the commit is successful then state has already been freed by this > point, and I get a splat. Yes, I believe I only tested with non-blocking requests. > > > /* > > * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, > > * if they weren't, this code should be called on success > > * for TEST_ONLY too. > > */ > > + if (ret && crtc_state->event) > > + drm_event_cancel_free(dev, &crtc_state->event->base); > > + } > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > - if (!crtc_state->event) > > - continue; > > + if (ret && fence_state) { > > + for (i = 0; i < fence_idx; i++) { > > + if (fence_state[i].fd >= 0) > > + put_unused_fd(fence_state[i].fd); > > + if (fence_state[i].sync_file) > > + fput(fence_state[i].sync_file->file); > > > > - drm_event_cancel_free(dev, &crtc_state->event->base); > > + /* If this fails, there's not much we can do about it */ > > + if (put_user(-1, fence_state->out_fence_ptr)) > > + DRM_ERROR("Couldn't clear out_fence_ptr\n"); > > } > > } > > > > + kfree(fence_state); > > + > > if (ret == -EDEADLK) { > > drm_atomic_state_clear(state); > > drm_modeset_backoff(&ctx); > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index b99090f..40bce97 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { > > drm_object_attach_property(&crtc->base, config->prop_active, 0); > > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); > > + drm_object_attach_property(&crtc->base, > > + config->prop_out_fence_ptr, 0); > > } > > > > return 0; > > @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > return -ENOMEM; > > dev->mode_config.prop_in_fence_fd = prop; > > > > + prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, > > + "OUT_FENCE_PTR", 0, U64_MAX); > > + if (!prop) > > + return -ENOMEM; > > + dev->mode_config.prop_out_fence_ptr = prop; > > + > > prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, > > "CRTC_ID", DRM_MODE_OBJECT_CRTC); > > if (!prop) > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 657a33a..b898604 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -165,6 +165,8 @@ struct drm_crtc_state { > > struct drm_property_blob *ctm; > > struct drm_property_blob *gamma_lut; > > > > + u64 __user *out_fence_ptr; > > + > > I'm somewhat not convinced about stashing a __user pointer in the > CRTC atomic state. I know it gets cleared before the driver sees it, > but if anything I think that hints that this isn't the right place to > store it. > > I've been trying to think of other ways to get from a property to > something drm_mode_atomic_ioctl can use - maybe we can store some > stuff in drm_atomic_state which only lives for the length of the ioctl > call and put this pointer in there. The drm_atomic_state is still visible by the drivers. Between there and crtc_state, I would keep in the crtc_state for now. Gustavo
Hi Gustavo, On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote: >Hi Brian, > >2016-10-20 Brian Starkey <brian.starkey@arm.com>: > >> Hi Gustavo, >> >> I notice your branch has the sync_file refcount change in, but this >> doesn't seem to take account for that. Will you be dropping that >> change to match the semantics of fence_array? > >I will drop the fence_get() in the out-fence patch because we already >hold the ref when we create the fence. The sync_file refcount patch will >remain. > OK, I'll work on that basis, thanks. >> >> Couple more comments below. >> >> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: >> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > >> > 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. >> > >> > In case of error userspace should receive a fd number of -1. >> > >> > 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. >> > >> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > --- >> > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- >> > drivers/gpu/drm/drm_crtc.c | 8 +++ >> > include/drm/drm_crtc.h | 19 +++++++ >> > 3 files changed, 119 insertions(+), 24 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> > index b3284b2..07775fc 100644 >> > --- a/drivers/gpu/drm/drm_atomic.c >> > +++ b/drivers/gpu/drm/drm_atomic.c >> > @@ -490,6 +490,8 @@ 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) { >> > + state->out_fence_ptr = u64_to_user_ptr(val); >> > } else if (crtc->funcs->atomic_set_property) >> > return crtc->funcs->atomic_set_property(crtc, state, property, val); >> > else >> > @@ -532,6 +534,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 >> > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); >> > */ >> > >> > static struct drm_pending_vblank_event *create_vblank_event( >> > - struct drm_device *dev, struct drm_file *file_priv, >> > - struct 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) >> > @@ -1488,17 +1490,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; >> > } >> > >> > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, >> > } >> > EXPORT_SYMBOL(drm_atomic_clean_old_fb); >> > >> > +static int crtc_setup_out_fence(struct drm_crtc *crtc, >> > + struct drm_crtc_state *crtc_state, >> > + struct drm_out_fence_state *fence_state) >> > +{ >> > + struct fence *fence; >> > + >> > + fence_state->fd = get_unused_fd_flags(O_CLOEXEC); >> > + if (fence_state->fd < 0) { >> > + return fence_state->fd; >> > + } >> > + >> > + fence_state->out_fence_ptr = crtc_state->out_fence_ptr; >> > + crtc_state->out_fence_ptr = NULL; >> > + >> > + if (put_user(fence_state->fd, fence_state->out_fence_ptr)) >> > + return -EFAULT; >> > + >> > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); >> > + if (!fence) >> > + return -ENOMEM; >> > + >> > + fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, >> > + crtc->fence_context, ++crtc->fence_seqno); >> > + >> > + fence_state->sync_file = sync_file_create(fence); >> > + if(!fence_state->sync_file) { >> > + fence_put(fence); >> > + return -ENOMEM; >> > + } >> > + >> > + crtc_state->event->base.fence = fence_get(fence); >> > + return 0; >> > +} >> > + >> > int drm_mode_atomic_ioctl(struct drm_device *dev, >> > void *data, struct drm_file *file_priv) >> > { >> > @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, >> > struct drm_plane *plane; >> > struct drm_crtc *crtc; >> > struct drm_crtc_state *crtc_state; >> > + struct drm_out_fence_state *fence_state; I just hit another crash - looks like fence_state needs to be initialised to NULL here, otherwise if property lookup/set fails we kfree() a bad pointer. >> > unsigned plane_mask; >> > int ret = 0; >> > - unsigned int i, j; >> > + unsigned int i, j, fence_idx = 0; >> > >> > /* disallow for drivers not supporting atomic: */ >> > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) >> > @@ -1734,12 +1760,19 @@ retry: >> > drm_mode_object_unreference(obj); >> > } >> > >> > - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >> > - for_each_crtc_in_state(state, crtc, crtc_state, i) { >> > + fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), >> > + GFP_KERNEL); >> > + if (!fence_state) { >> > + ret = -ENOMEM; >> > + goto out; >> > + } >> > + >> > + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || >> > + crtc_state->out_fence_ptr) { >> > struct drm_pending_vblank_event *e; >> > >> > - e = create_vblank_event(dev, file_priv, NULL, >> > - arg->user_data); >> > + e = create_vblank_event(dev, arg->user_data); >> > if (!e) { >> > ret = -ENOMEM; >> > goto out; >> > @@ -1747,6 +1780,28 @@ retry: >> > >> > crtc_state->event = e; >> > } >> > + >> > + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >> > + struct drm_pending_vblank_event *e = crtc_state->event; >> > + >> > + if (!file_priv) >> > + continue; >> > + >> > + ret = drm_event_reserve_init(dev, file_priv, &e->base, >> > + &e->event.base); >> > + if (ret) { >> > + kfree(e); >> > + crtc_state->event = NULL; >> > + goto out; >> > + } >> > + } >> > + >> > + if (crtc_state->out_fence_ptr) { >> > + ret = crtc_setup_out_fence(crtc, crtc_state, >> > + &fence_state[fence_idx++]); >> > + if (ret) >> > + goto out; >> > + } >> > } >> > >> > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { >> > @@ -1761,24 +1816,37 @@ retry: >> > ret = drm_atomic_commit(state); >> > } >> > >> > + for (i = 0; i < fence_idx; i++) >> > + fd_install(fence_state[i].fd, fence_state[i].sync_file->file); >> > + >> > out: >> > drm_atomic_clean_old_fb(dev, plane_mask, ret); >> > >> > - if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >> > + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> >> I think a check on ret is still needed before you iterate the CRTCs. >> If the commit is successful then state has already been freed by this >> point, and I get a splat. > >Yes, I believe I only tested with non-blocking requests. > Right, I suppose you should get here before the free in that case. >> >> > /* >> > * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, >> > * if they weren't, this code should be called on success >> > * for TEST_ONLY too. >> > */ >> > + if (ret && crtc_state->event) >> > + drm_event_cancel_free(dev, &crtc_state->event->base); >> > + } >> > >> > - for_each_crtc_in_state(state, crtc, crtc_state, i) { >> > - if (!crtc_state->event) >> > - continue; >> > + if (ret && fence_state) { >> > + for (i = 0; i < fence_idx; i++) { >> > + if (fence_state[i].fd >= 0) >> > + put_unused_fd(fence_state[i].fd); >> > + if (fence_state[i].sync_file) >> > + fput(fence_state[i].sync_file->file); >> > >> > - drm_event_cancel_free(dev, &crtc_state->event->base); >> > + /* If this fails, there's not much we can do about it */ >> > + if (put_user(-1, fence_state->out_fence_ptr)) >> > + DRM_ERROR("Couldn't clear out_fence_ptr\n"); >> > } >> > } >> > >> > + kfree(fence_state); >> > + >> > if (ret == -EDEADLK) { >> > drm_atomic_state_clear(state); >> > drm_modeset_backoff(&ctx); >> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> > index b99090f..40bce97 100644 >> > --- a/drivers/gpu/drm/drm_crtc.c >> > +++ b/drivers/gpu/drm/drm_crtc.c >> > @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >> > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { >> > drm_object_attach_property(&crtc->base, config->prop_active, 0); >> > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); >> > + drm_object_attach_property(&crtc->base, >> > + config->prop_out_fence_ptr, 0); >> > } >> > >> > return 0; >> > @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) >> > return -ENOMEM; >> > dev->mode_config.prop_in_fence_fd = prop; >> > >> > + prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, >> > + "OUT_FENCE_PTR", 0, U64_MAX); >> > + if (!prop) >> > + return -ENOMEM; >> > + dev->mode_config.prop_out_fence_ptr = prop; >> > + >> > prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, >> > "CRTC_ID", DRM_MODE_OBJECT_CRTC); >> > if (!prop) >> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> > index 657a33a..b898604 100644 >> > --- a/include/drm/drm_crtc.h >> > +++ b/include/drm/drm_crtc.h >> > @@ -165,6 +165,8 @@ struct drm_crtc_state { >> > struct drm_property_blob *ctm; >> > struct drm_property_blob *gamma_lut; >> > >> > + u64 __user *out_fence_ptr; >> > + >> >> I'm somewhat not convinced about stashing a __user pointer in the >> CRTC atomic state. I know it gets cleared before the driver sees it, >> but if anything I think that hints that this isn't the right place to >> store it. >> >> I've been trying to think of other ways to get from a property to >> something drm_mode_atomic_ioctl can use - maybe we can store some >> stuff in drm_atomic_state which only lives for the length of the ioctl >> call and put this pointer in there. > >The drm_atomic_state is still visible by the drivers. Between there and >crtc_state, I would keep in the crtc_state for now. > Mm, yeah I suppose they could get to it if they went looking for it in ->atomic_set_property or something, but I was thinking of freeing it before the drm_atomic_commit. Anyway, this way is certainly simpler, and setting it to NULL should be enough to limit the damage a driver can do :-) Thanks, Brian >Gustavo >
On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote: > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote: > > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > > > > > that sets the OUT_FENCE_PTR property. > > > > > > > > I still maintain the out fence should also be per fb (well, per plane > > > > since we can't add it to fbs). Otherwise it's not going to be at all > > > > useful if you do fps>vrefresh and don't include the same set of planes > > > > in every atomic ioctl, eg. if you only ever include ones that are > > > > somehow dirty. > > > > > > How would the kernel signal these dirty planes then? Right now we signal > > > at the vblank. > > > > So if I think about it in terms of the previous fbs something like this > > comes to mind: > > > > starting point: > > plane a, fb 0 > > plane b, fb 1 > > > > ioctl: > > plane a, fb 2, fence A > > plane b, fb 3, fence B > > > > ioctl: > > plane a, fb 4, fence C > > -> fb 2 can be reused, so fence C should signal immediately? > > > > vblank: > > -> fb 0,1 can be reused, so fence A,B signal? > > > > It feels a bit wonky since the fence is effectively associated with the > > previous fb after the previous fb was already submitted to the kernel. > > One might assume fence A to be the one signalled early, but that would > > give the impression that fb 0 would be free for reuse when it's not. > > OK, so after hashing this out on irc with Gustavo and Brian, we came > to the conclusion that the per-crtc out fence should be sufficient > after all. > > The way it can work is that the first ioctl will return the fence, > doesn't really matter how many of the planes are involved in this > ioctl. Subsequent ioctls that manage to get in before the next > vblank will get back a -1 as the fence, even if the set if planes > involved is not the same as in the first ioctl. From the -1 > userspace can tell what happened, and can then consult its own > records to see which still pending flips were overwritten, and > thus knows which buffers can be reused immediately. > > If userspace plans on sending out the now free buffers to someone > else accompanied by a fence, it can just create an already signalled > fence to send instead of sending the fence it got back from the > atomic ioctl since that one is still associated with the original > scanout buffers. > > The fence returned by the atomic ioctl will only signal after the > vblank, at which point userspace will obviously know that the > orginal scanout buffers are now also ready for reuse, and that > the last buffer submitted for each plane is now being actively > scanned out. So if userspace wants to send out some of the > original buffers to someone else, it can send along the fence > returned from the atomic ioctl. > > So requires a bit more work from userspace perhaps. But obviously > it only has to do this if it plans on submitting multiple operations > per frame. > > So a slightly expanded version of my previous example might look like: > starting point: > plane a, fb 0 > plane b, fb 1 > > ioctl: > crtc: fence A > plane a, fb 2 > plane b, fb 3 > > ioctl: > crtc: fence -1 > plane a, fb 4 > -> the previously submitted fb for plane a (fb 2) can be reused > > ioctl: > crtc: fence -1 > plane a, fb 5 > -> the previously submitted fb for plane a (fb 4) can be reused > > vblank: > -> fence A signals, fb 0,1 can be reused > fb 3 and 5 being scanned out now > > > We also had a quick side track w.r.t. variable refresh rate displays, > and I think the conclusion was that there the vblank may just happen > sooner or later. Nothing else should change. What's unclear is how > the refresh rate would be controlled. The two obvious options are > explicit control, and automagic based on the submit rate. But that's > a separate topic entirely. Thanks a lot for doing this discussion and the detailed write-up. Imo we should bake this into the kerneldoc, as uabi documentation examples. Gustavo, can you pls include this? I'd put it into the kerneldoc for @out_fence, with inline struct comments we can be rather excessive in their lenght ;-) -Daniel
On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote: > On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote: > > 2016-10-20 Brian Starkey <brian.starkey@arm.com>: > > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > > > index 657a33a..b898604 100644 > > > > --- a/include/drm/drm_crtc.h > > > > +++ b/include/drm/drm_crtc.h > > > > @@ -165,6 +165,8 @@ struct drm_crtc_state { > > > > struct drm_property_blob *ctm; > > > > struct drm_property_blob *gamma_lut; > > > > > > > > + u64 __user *out_fence_ptr; > > > > + > > > > > > I'm somewhat not convinced about stashing a __user pointer in the > > > CRTC atomic state. I know it gets cleared before the driver sees it, > > > but if anything I think that hints that this isn't the right place to > > > store it. > > > > > > I've been trying to think of other ways to get from a property to > > > something drm_mode_atomic_ioctl can use - maybe we can store some > > > stuff in drm_atomic_state which only lives for the length of the ioctl > > > call and put this pointer in there. > > > > The drm_atomic_state is still visible by the drivers. Between there and > > crtc_state, I would keep in the crtc_state for now. > > > > Mm, yeah I suppose they could get to it if they went looking for it > in ->atomic_set_property or something, but I was thinking of freeing > it before the drm_atomic_commit. > > Anyway, this way is certainly simpler, and setting it to NULL should > be enough to limit the damage a driver can do :-) +1 on moving this out of drm_crtc_state. drm_atomic_state already has per-crtc structs, so easy to extend them with this. And yes drivers can still see it, but mostly they're not supposed to touch drm_atomic_state internals - the book-keeping is all done by the core. The per-object state structs otoh are meant to be massively mangled by drivers. -Daniel
2016-10-21 Daniel Vetter <daniel@ffwll.ch>: > On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote: > > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote: > > > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > > > > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: > > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > > > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC > > > > > > that sets the OUT_FENCE_PTR property. > > > > > > > > > > I still maintain the out fence should also be per fb (well, per plane > > > > > since we can't add it to fbs). Otherwise it's not going to be at all > > > > > useful if you do fps>vrefresh and don't include the same set of planes > > > > > in every atomic ioctl, eg. if you only ever include ones that are > > > > > somehow dirty. > > > > > > > > How would the kernel signal these dirty planes then? Right now we signal > > > > at the vblank. > > > > > > So if I think about it in terms of the previous fbs something like this > > > comes to mind: > > > > > > starting point: > > > plane a, fb 0 > > > plane b, fb 1 > > > > > > ioctl: > > > plane a, fb 2, fence A > > > plane b, fb 3, fence B > > > > > > ioctl: > > > plane a, fb 4, fence C > > > -> fb 2 can be reused, so fence C should signal immediately? > > > > > > vblank: > > > -> fb 0,1 can be reused, so fence A,B signal? > > > > > > It feels a bit wonky since the fence is effectively associated with the > > > previous fb after the previous fb was already submitted to the kernel. > > > One might assume fence A to be the one signalled early, but that would > > > give the impression that fb 0 would be free for reuse when it's not. > > > > OK, so after hashing this out on irc with Gustavo and Brian, we came > > to the conclusion that the per-crtc out fence should be sufficient > > after all. > > > > The way it can work is that the first ioctl will return the fence, > > doesn't really matter how many of the planes are involved in this > > ioctl. Subsequent ioctls that manage to get in before the next > > vblank will get back a -1 as the fence, even if the set if planes > > involved is not the same as in the first ioctl. From the -1 > > userspace can tell what happened, and can then consult its own > > records to see which still pending flips were overwritten, and > > thus knows which buffers can be reused immediately. > > > > If userspace plans on sending out the now free buffers to someone > > else accompanied by a fence, it can just create an already signalled > > fence to send instead of sending the fence it got back from the > > atomic ioctl since that one is still associated with the original > > scanout buffers. > > > > The fence returned by the atomic ioctl will only signal after the > > vblank, at which point userspace will obviously know that the > > orginal scanout buffers are now also ready for reuse, and that > > the last buffer submitted for each plane is now being actively > > scanned out. So if userspace wants to send out some of the > > original buffers to someone else, it can send along the fence > > returned from the atomic ioctl. > > > > So requires a bit more work from userspace perhaps. But obviously > > it only has to do this if it plans on submitting multiple operations > > per frame. > > > > So a slightly expanded version of my previous example might look like: > > starting point: > > plane a, fb 0 > > plane b, fb 1 > > > > ioctl: > > crtc: fence A > > plane a, fb 2 > > plane b, fb 3 > > > > ioctl: > > crtc: fence -1 > > plane a, fb 4 > > -> the previously submitted fb for plane a (fb 2) can be reused > > > > ioctl: > > crtc: fence -1 > > plane a, fb 5 > > -> the previously submitted fb for plane a (fb 4) can be reused > > > > vblank: > > -> fence A signals, fb 0,1 can be reused > > fb 3 and 5 being scanned out now > > > > > > We also had a quick side track w.r.t. variable refresh rate displays, > > and I think the conclusion was that there the vblank may just happen > > sooner or later. Nothing else should change. What's unclear is how > > the refresh rate would be controlled. The two obvious options are > > explicit control, and automagic based on the submit rate. But that's > > a separate topic entirely. > > Thanks a lot for doing this discussion and the detailed write-up. Imo we > should bake this into the kerneldoc, as uabi documentation examples. > Gustavo, can you pls include this? I'd put it into the kerneldoc for > @out_fence, with inline struct comments we can be rather excessive in > their lenght ;-) Sure, I'll work on kernel doc for this. Gustavo
Hi, Sorry, I hit another couple of bugs that originated from my hastebin patch - see below. On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote: >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >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. > >In case of error userspace should receive a fd number of -1. > >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. > >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >--- > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- > drivers/gpu/drm/drm_crtc.c | 8 +++ > include/drm/drm_crtc.h | 19 +++++++ > 3 files changed, 119 insertions(+), 24 deletions(-) > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >index b3284b2..07775fc 100644 >--- a/drivers/gpu/drm/drm_atomic.c >+++ b/drivers/gpu/drm/drm_atomic.c >@@ -490,6 +490,8 @@ 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) { >+ state->out_fence_ptr = u64_to_user_ptr(val); > } else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else >@@ -532,6 +534,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 >@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > */ > > static struct drm_pending_vblank_event *create_vblank_event( >- struct drm_device *dev, struct drm_file *file_priv, >- struct 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) >@@ -1488,17 +1490,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; > } > >@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_clean_old_fb); > >+static int crtc_setup_out_fence(struct drm_crtc *crtc, >+ struct drm_crtc_state *crtc_state, >+ struct drm_out_fence_state *fence_state) >+{ >+ struct fence *fence; >+ >+ fence_state->fd = get_unused_fd_flags(O_CLOEXEC); >+ if (fence_state->fd < 0) { >+ return fence_state->fd; >+ } >+ >+ fence_state->out_fence_ptr = crtc_state->out_fence_ptr; >+ crtc_state->out_fence_ptr = NULL; >+ >+ if (put_user(fence_state->fd, fence_state->out_fence_ptr)) >+ return -EFAULT; >+ >+ fence = kzalloc(sizeof(*fence), GFP_KERNEL); >+ if (!fence) >+ return -ENOMEM; >+ >+ fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, >+ crtc->fence_context, ++crtc->fence_seqno); >+ >+ fence_state->sync_file = sync_file_create(fence); >+ if(!fence_state->sync_file) { >+ fence_put(fence); >+ return -ENOMEM; >+ } >+ >+ crtc_state->event->base.fence = fence_get(fence); >+ return 0; >+} >+ > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { >@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > struct drm_plane *plane; > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; >+ struct drm_out_fence_state *fence_state; > unsigned plane_mask; > int ret = 0; >- unsigned int i, j; >+ unsigned int i, j, fence_idx = 0; > > /* disallow for drivers not supporting atomic: */ > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) >@@ -1734,12 +1760,19 @@ retry: > drm_mode_object_unreference(obj); > } > >- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >- for_each_crtc_in_state(state, crtc, crtc_state, i) { >+ fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), >+ GFP_KERNEL); >+ if (!fence_state) { >+ ret = -ENOMEM; >+ goto out; >+ } >+ >+ for_each_crtc_in_state(state, crtc, crtc_state, i) { >+ if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || >+ crtc_state->out_fence_ptr) { > struct drm_pending_vblank_event *e; > >- e = create_vblank_event(dev, file_priv, NULL, >- arg->user_data); >+ e = create_vblank_event(dev, arg->user_data); > if (!e) { > ret = -ENOMEM; > goto out; >@@ -1747,6 +1780,28 @@ retry: > > crtc_state->event = e; > } >+ >+ if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >+ struct drm_pending_vblank_event *e = crtc_state->event; >+ >+ if (!file_priv) >+ continue; >+ >+ ret = drm_event_reserve_init(dev, file_priv, &e->base, >+ &e->event.base); >+ if (ret) { >+ kfree(e); >+ crtc_state->event = NULL; >+ goto out; >+ } >+ } >+ >+ if (crtc_state->out_fence_ptr) { >+ ret = crtc_setup_out_fence(crtc, crtc_state, >+ &fence_state[fence_idx++]); >+ if (ret) >+ goto out; >+ } > } > > if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { >@@ -1761,24 +1816,37 @@ retry: > ret = drm_atomic_commit(state); > } > >+ for (i = 0; i < fence_idx; i++) >+ fd_install(fence_state[i].fd, fence_state[i].sync_file->file); This loop should be conditional on ret. If drm_atomic_commit fails it doesn't jump over this, it falls into it. >+ > out: > drm_atomic_clean_old_fb(dev, plane_mask, ret); > >- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { >+ for_each_crtc_in_state(state, crtc, crtc_state, i) { > /* > * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, > * if they weren't, this code should be called on success > * for TEST_ONLY too. > */ >+ if (ret && crtc_state->event) >+ drm_event_cancel_free(dev, &crtc_state->event->base); >+ } > >- for_each_crtc_in_state(state, crtc, crtc_state, i) { >- if (!crtc_state->event) >- continue; >+ if (ret && fence_state) { >+ for (i = 0; i < fence_idx; i++) { >+ if (fence_state[i].fd >= 0) >+ put_unused_fd(fence_state[i].fd); >+ if (fence_state[i].sync_file) >+ fput(fence_state[i].sync_file->file); > >- drm_event_cancel_free(dev, &crtc_state->event->base); >+ /* If this fails, there's not much we can do about it */ >+ if (put_user(-1, fence_state->out_fence_ptr)) >+ DRM_ERROR("Couldn't clear out_fence_ptr\n"); This should be conditional on fence_state->out_fence_ptr, otherwise it's needlessly noisy for an incompletely filled-in fence_state. I also wonder if the fput should be first, for symmetry. Cheers, Brian > } > } > >+ kfree(fence_state); >+ > if (ret == -EDEADLK) { > drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); >diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >index b99090f..40bce97 100644 >--- a/drivers/gpu/drm/drm_crtc.c >+++ b/drivers/gpu/drm/drm_crtc.c >@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { > drm_object_attach_property(&crtc->base, config->prop_active, 0); > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); >+ drm_object_attach_property(&crtc->base, >+ config->prop_out_fence_ptr, 0); > } > > return 0; >@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_in_fence_fd = prop; > >+ prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, >+ "OUT_FENCE_PTR", 0, U64_MAX); >+ if (!prop) >+ return -ENOMEM; >+ dev->mode_config.prop_out_fence_ptr = prop; >+ > prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, > "CRTC_ID", DRM_MODE_OBJECT_CRTC); > if (!prop) >diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >index 657a33a..b898604 100644 >--- a/include/drm/drm_crtc.h >+++ b/include/drm/drm_crtc.h >@@ -165,6 +165,8 @@ struct drm_crtc_state { > struct drm_property_blob *ctm; > struct drm_property_blob *gamma_lut; > >+ u64 __user *out_fence_ptr; >+ > /** > * @event: > * >@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) > } > > /** >+ * struct drm_out_fence_state - store out_fence data for install and clean up >+ * @out_fence_ptr: user pointer to store the fence fd in. >+ * @sync_file: sync_file related to the out_fence >+ * @fd: file descriptor to installed on the sync_file. >+ */ >+struct drm_out_fence_state { >+ u64 __user *out_fence_ptr; >+ struct sync_file *sync_file; >+ int fd; >+}; >+ >+/* > * struct drm_mode_set - new values for a CRTC config change > * @fb: framebuffer to use for new config > * @crtc: CRTC whose configuration we're about to change >@@ -1230,6 +1244,11 @@ struct drm_mode_config { > */ > struct drm_property *prop_in_fence_fd; > /** >+ * @prop_out_fence_ptr: Sync File fd pointer representing the >+ * outgoing fences for a CRTC. >+ */ >+ struct drm_property *prop_out_fence_ptr; >+ /** > * @prop_crtc_id: Default atomic plane property to specify the > * &drm_crtc. > */ >-- >2.5.5 > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ 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) { + state->out_fence_ptr = u64_to_user_ptr(val); } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else @@ -532,6 +534,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 @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */ static struct drm_pending_vblank_event *create_vblank_event( - struct drm_device *dev, struct drm_file *file_priv, - struct 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) @@ -1488,17 +1490,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; } @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb); +static int crtc_setup_out_fence(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state, + struct drm_out_fence_state *fence_state) +{ + struct fence *fence; + + fence_state->fd = get_unused_fd_flags(O_CLOEXEC); + if (fence_state->fd < 0) { + return fence_state->fd; + } + + fence_state->out_fence_ptr = crtc_state->out_fence_ptr; + crtc_state->out_fence_ptr = NULL; + + if (put_user(fence_state->fd, fence_state->out_fence_ptr)) + return -EFAULT; + + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (!fence) + return -ENOMEM; + + fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, + crtc->fence_context, ++crtc->fence_seqno); + + fence_state->sync_file = sync_file_create(fence); + if(!fence_state->sync_file) { + fence_put(fence); + return -ENOMEM; + } + + crtc_state->event->base.fence = fence_get(fence); + return 0; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; + struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0; - unsigned int i, j; + unsigned int i, j, fence_idx = 0; /* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); } - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { + fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), + GFP_KERNEL); + if (!fence_state) { + ret = -ENOMEM; + goto out; + } + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || + crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e; - e = create_vblank_event(dev, file_priv, NULL, - arg->user_data); + e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out; @@ -1747,6 +1780,28 @@ retry: crtc_state->event = e; } + + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + struct drm_pending_vblank_event *e = crtc_state->event; + + if (!file_priv) + continue; + + ret = drm_event_reserve_init(dev, file_priv, &e->base, + &e->event.base); + if (ret) { + kfree(e); + crtc_state->event = NULL; + goto out; + } + } + + if (crtc_state->out_fence_ptr) { + ret = crtc_setup_out_fence(crtc, crtc_state, + &fence_state[fence_idx++]); + if (ret) + goto out; + } } if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { @@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); } + for (i = 0; i < fence_idx; i++) + fd_install(fence_state[i].fd, fence_state[i].sync_file->file); + out: drm_atomic_clean_old_fb(dev, plane_mask, ret); - if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { /* * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, * if they weren't, this code should be called on success * for TEST_ONLY too. */ + if (ret && crtc_state->event) + drm_event_cancel_free(dev, &crtc_state->event->base); + } - for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc_state->event) - continue; + if (ret && fence_state) { + for (i = 0; i < fence_idx; i++) { + if (fence_state[i].fd >= 0) + put_unused_fd(fence_state[i].fd); + if (fence_state[i].sync_file) + fput(fence_state[i].sync_file->file); - drm_event_cancel_free(dev, &crtc_state->event->base); + /* If this fails, there's not much we can do about it */ + if (put_user(-1, fence_state->out_fence_ptr)) + DRM_ERROR("Couldn't clear out_fence_ptr\n"); } } + kfree(fence_state); + if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); + drm_object_attach_property(&crtc->base, + config->prop_out_fence_ptr, 0); } return 0; @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop; + prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, + "OUT_FENCE_PTR", 0, U64_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_out_fence_ptr = prop; + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut; + u64 __user *out_fence_ptr; + /** * @event: * @@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) } /** + * struct drm_out_fence_state - store out_fence data for install and clean up + * @out_fence_ptr: user pointer to store the fence fd in. + * @sync_file: sync_file related to the out_fence + * @fd: file descriptor to installed on the sync_file. + */ +struct drm_out_fence_state { + u64 __user *out_fence_ptr; + struct sync_file *sync_file; + int fd; +}; + +/* * struct drm_mode_set - new values for a CRTC config change * @fb: framebuffer to use for new config * @crtc: CRTC whose configuration we're about to change @@ -1230,6 +1244,11 @@ struct drm_mode_config { */ struct drm_property *prop_in_fence_fd; /** + * @prop_out_fence_ptr: Sync File fd pointer representing the + * outgoing fences for a CRTC. + */ + struct drm_property *prop_out_fence_ptr; + /** * @prop_crtc_id: Default atomic plane property to specify the * &drm_crtc. */