Re: [RFC v2 8/8] drm/fence: add out-fences support

On Mon, Apr 25, 2016 at 07:33:28PM -0300, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

Support DRM out-fences creating a sync_file with a fence for each crtc update with the DRM_MODE_ATOMIC_OUT_FENCE flag.

We then send an struct drm_out_fences array with the out-fences fds back in the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.

struct drm_out_fences { __u32 crtc_id; __u32 fd; };

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

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/drm_atomic.c | 163 +++++++++++++++++++++++++++++++++++++++++-- include/drm/drm_crtc.h | 10 +++ include/uapi/drm/drm_mode.h | 11 ++- 3 files changed, 179 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5f9d434..06c6007 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1566,6 +1566,133 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+static struct drm_out_fence_state *get_out_fence(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 drm_out_fence_state *fence_state;
• int num_fences = 0;
• int i, ret;
• if (count_out_fences > dev->mode_config.num_crtc)

• return ERR_PTR(-EINVAL);
  

• out_fences = kcalloc(count_out_fences, sizeof(*out_fences),

• 	     GFP_KERNEL);
  

• if (!out_fences)

• return ERR_PTR(-ENOMEM);
  

A bit tricky, but the above kcalloc is the only thing that catches integer overflows in count_out_fences. Needs a comment imo since this could be a security exploit if we accidentally screw it up.

Also needs a testcase imo.

• fence_state = kcalloc(count_out_fences, sizeof(*fence_state),

• 	     GFP_KERNEL);
  

• if (!fence_state) {

• kfree(out_fences);
  

• return ERR_PTR(-ENOMEM);
  

• }
• for (i = 0 ; i < count_out_fences ; i++)

• fence_state[i].fd = -1;
  

• for_each_crtc_in_state(state, crtc, crtc_state, i) {

• struct drm_pending_vblank_event *e;
  

• struct fence *fence;
  

• char name[32];
  

• fence = kzalloc(sizeof(*fence), GFP_KERNEL);
  

• if (!fence) {
  

• 	ret = -ENOMEM;
  

• 	goto out;
  

• }
  

• fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
  

• 	   crtc->fence_context, crtc->fence_seqno);
  

• snprintf(name, sizeof(name), "crtc-%d_%lu",
  

• 	 drm_crtc_index(crtc), crtc->fence_seqno++);
  

Hm ... fence_init_with_name? I'm kinda confused why we only name fences that are exported though, and why not all of them. Debugging fence deadlocks is real hard, so giving them all names might be a good idea.

Anyway, seems like more room for a bit more sync_file/struct fence merging.

• fence_state[i].fd = get_unused_fd_flags(O_CLOEXEC);
  

• if (fence_state[i].fd < 0) {
  

• 	fence_put(fence);
  

• 	ret = fence_state[i].fd;
  

• 	goto out;
  

• }
  

• fence_state[i].sync_file = sync_file_create(name, fence);
  

• if(!fence_state[i].sync_file) {
  

• 	fence_put(fence);
  

• 	ret = -ENOMEM;
  

• 	goto out;
  

• }
  

• if (crtc_state->event) {
  

• 	crtc_state->event->base.fence = fence;
  

• } else {
  

This looks a bit funny - I'd change the create event logic to create an event either if we have the either drm event or out-fence flag set.

• 	e = create_vblank_event(dev, NULL, fence, user_data);
  

• 	if (!e) {
  

• 		ret = -ENOMEM;
  

• 		goto out;
  

• 	}
  

• 	crtc_state->event = e;
  

• }
  

• out_fences[num_fences].crtc_id = crtc->base.id;
  

• out_fences[num_fences].fd = fence_state[i].fd;
  

• num_fences++;
  

• }
• if (copy_to_user(out_fences_ptr, out_fences,

• 	 num_fences * sizeof(*out_fences))) {
  

• ret = -EFAULT;
  

• goto out;
  

• }
• kfree(out_fences);
• return fence_state;

+out:

• for (i = 0 ; i < count_out_fences ; i++) {

• if (fence_state[i].sync_file)
  

• 	sync_file_put(fence_state[i].sync_file);
  

• if (fence_state[i].fd >= 0)
  

• 	put_unused_fd(fence_state[i].fd);
  

• }
• kfree(fence_state);
• kfree(out_fences);
• return ERR_PTR(ret);

+}

+static void install_out_fence(uint64_t count_out_fences,

• 	      struct drm_out_fence_state *state)
  

+{

• int i;
• for (i = 0 ; i < count_out_fences ; i++) {

• if (state[i].sync_file)
  

• 	sync_file_install(state[i].sync_file, state[i].fd);
  

Is sync_file_install anything more than fd_install? Imo a wrapper for just that function is overkill and just hides stuff. I'd nuke it (another sync_file patch though). In dma-buf we also don't wrap it, we only have a convenience wrapper for users who want to combine the get_unused_flags+fd_install in one go. And maybe even that is silly.

Ok, I unlazied and it's indeed just a silly wrapper. Please nuke it.

• }

+}

+static void release_out_fence(uint64_t count_out_fences,

• 	      struct drm_out_fence_state *state)
  

+{

• int i;
• for (i = 0 ; i < count_out_fences ; i++) {

• if (state->sync_file)
  

• 	sync_file_put(state->sync_file);
  

• if (state->fd >= 0)
  

• 	put_unused_fd(state->fd);
  

• }

+}

int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1574,12 +1701,14 @@ 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; struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
• struct drm_out_fence_state *fence_state = NULL; unsigned plane_mask; int ret = 0; unsigned int i, j;

@@ -1605,9 +1734,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, !dev->mode_config.async_page_flip) return -EINVAL;

• if ((arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) && !arg->count_out_fences)

• return -EINVAL;
  

We need testcases which check that arg->count_out_fences and arg->out_fences are 0 when the OUT_FENCE flag is not set.

Definitely needs an igt testcase for this invalid input case. Ofc we also need tests that give the kernel nonsens in count_out_fences and out_fences with the flag set.

• /* 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)))
  

If you go with my suggestion above to create the event if either is set, maybe a DRM_MODE_ATOMIC_EVENT_MASK with both? Would read easier.

return -EINVAL;

drm_modeset_acquire_init(&ctx, 0); @@ -1699,16 +1832,30 @@ retry: } }

• if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {

• fence_state = get_out_fence(dev, state, out_fences_ptr,
  

• 			    arg->count_out_fences,
  

• 			    arg->user_data);
  

• if (IS_ERR(fence_state)) {
  

• 	ret = PTR_ERR(fence_state);
  

• 	goto out;
  

• }
  

• }
• if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { /*
  • Unlike commit, check_only does not clean up state.
  • Below we call drm_atomic_state_free for it.
  */ ret = drm_atomic_check_only(state);

• } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {

• ret = drm_atomic_async_commit(state);
  

  } else {

• ret = drm_atomic_commit(state);
  

• if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE)
  

• 	install_out_fence(arg->count_out_fences, fence_state);
  

• if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
  

• 	ret = drm_atomic_async_commit(state);
  

• else
  

• 	ret = drm_atomic_commit(state);
  

  }

out: @@ -1729,6 +1876,14 @@ out: } }

• if ((arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) &&

•    !IS_ERR_OR_NULL(fence_state)) {
  

• if (ret)
  

• 	release_out_fence(arg->count_out_fences, fence_state);
  

• kfree(fence_state);
  

• }
• if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d8c32c8..5f7c272 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -798,6 +798,16 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) }

/**

• • struct drm_out_fence_state - store out_fence data for clean up
• • @sync_file: sync_file related to the out_fence
• • @fd: file descriptor to installed on the sync_file.
• */

+struct drm_out_fence_state {

• struct sync_file *sync_file;
• int fd;

+};

+/**

• struct drm_connector_state - mutable connector state
• @connector: backpointer to the connector
• @crtc: CRTC to connect connector to, NULL if disabled

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 7a7856e..4cdcd22 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -582,13 +582,20 @@ struct drm_mode_destroy_dumb { #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100 #define DRM_MODE_ATOMIC_NONBLOCK 0x0200 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400 +#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800

#define DRM_MODE_ATOMIC_FLAGS (\ DRM_MODE_PAGE_FLIP_EVENT |\ DRM_MODE_PAGE_FLIP_ASYNC |\ DRM_MODE_ATOMIC_TEST_ONLY |\ DRM_MODE_ATOMIC_NONBLOCK |\

• DRM_MODE_ATOMIC_ALLOW_MODESET)
  

• DRM_MODE_ATOMIC_ALLOW_MODESET |\
  

• DRM_MODE_ATOMIC_OUT_FENCE)
  

+struct drm_out_fences {

• __u32 crtc_id;
• __u32 fd;

+};

struct drm_mode_atomic { __u32 flags; @@ -599,6 +606,8 @@ struct drm_mode_atomic { __u64 prop_values_ptr; __u64 reserved; __u64 user_data;

• __u64 count_out_fences;
• __u64 out_fences_ptr;

};

/**

2.5.5