Message-ID: <20160426145305.GY8291@phenom.ffwll.local>
Date:	Tue, 26 Apr 2016 16:53:05 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Gustavo Padovan <gustavo@...ovan.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
	dri-devel@...ts.freedesktop.org,
	Daniel Stone <daniels@...labora.com>,
	Arve Hjønnevåg <arve@...roid.com>,
	Riley Andrews <riandrews@...roid.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	Greg Hackmann <ghackmann@...gle.com>,
	John Harrison <John.C.Harrison@...el.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>
Subject: 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@...labora.co.uk>
> 
> Support DRM out-fences creating a sync_file with a fence for each crtc
> update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> 
> We then send an struct drm_out_fences array with the out-fences fds back in
> the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> 
> struct drm_out_fences {
> 	__u32   crtc_id;
> 	__u32   fd;
> };
> 
> 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@...labora.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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists