Message-ID: <20161108131549.lvxuqvabb43esnvx@phenom.ffwll.local>
Date:   Tue, 8 Nov 2016 14:15:49 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Gustavo Padovan <gustavo@...ovan.org>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Daniel Stone <daniels@...labora.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Rob Clark <robdclark@...il.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        John Harrison <John.C.Harrison@...el.com>,
        laurent.pinchart@...asonboard.com, seanpaul@...gle.com,
        marcheu@...gle.com, m.chehab@...sung.com,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Brian Starkey <brian.starkey@....com>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>
Subject: Re: [PATCH v7 3/3] drm/fence: add out-fences support

On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@...labora.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.
> 
> v2: Comment by Rob Clark:
> 	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> 
>     Comment by Daniel Vetter:
> 	- Add clean up code for out_fences
> 
> v3: Comments by Daniel Vetter:
> 	- create DRM_MODE_ATOMIC_EVENT_MASK
> 	- userspace should fill out_fences_ptr with the crtc_ids for which
> 	it wants fences back.
> 
> v4: Create OUT_FENCE_PTR properties and remove old approach.
> 
> v5: Comments by Brian Starkey:
> 	- Remove extra fence_get() in atomic_ioctl()
> 	- Check ret before iterating on the crtc_state
> 	- check ret before fd_install
> 	- set fence_state to NULL at the beginning
> 	- check fence_state->out_fence_ptr before put_user()
> 	- change order of fput() and put_unused_fd() on failure
> 
>      - Add access_ok() check to the out_fence_ptr received
>      - Rebase after fence -> dma_fence rename
>      - Store out_fence_ptr in the drm_atomic_state
>      - Split crtc_setup_out_fence()
>      - return -1 as out_fence with TEST_ONLY flag
> 
> v6: Comments by Daniel Vetter
> 	- Add prepare/unprepare_crtc_signaling()
> 	- move struct drm_out_fence_state to drm_atomic.c
> 	- mark get_crtc_fence() as static
> 
>     Comments by Brian Starkey
> 	- proper set fence_ptr fence_state array
> 	- isolate fence_idx increment
> 
>     - improve error handling
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>

Found a bunch more nitpicks, and wishlist items for igt coverage.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 233 +++++++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_crtc.c   |   8 ++
>  include/drm/drm_atomic.h     |   1 +
>  include/drm/drm_crtc.h       |   7 +-
>  4 files changed, 206 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d1ae463..9a7d84c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -289,6 +289,25 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
> +static void
> +drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
> +				  struct drm_crtc *crtc, u64 __user *fence_ptr)
> +{
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> +}
> +
> +static u64 __user *
> +drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
> +			     struct drm_crtc *crtc)

Since these two are static I'd drop the long prefix to make the code
easier to read.

> +{
> +	u64 __user *fence_ptr;
> +
> +	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
> +
> +	return fence_ptr;
> +}
> +
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
> @@ -490,6 +509,12 @@ 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) {
> +		uint64_t __user *fence_ptr = u64_to_user_ptr(val);

Don't we need to filter out NULL/0 here like we filter out -1 for the
in-fences? Just in case some userspace samples/restores all properties.

Sounds like an igt is missing for this ...

Maybe we even need a special igt which samples _all_ current atomic
properties, and then does an atomic commit which changes none of them
(without setting the ALLOW_MODESET flag ofc). That /should/ work, and
would catch such bugs in the future.

> +		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
> +			return -EFAULT;

Same comment about igt coverage I made for patch 1, but with
s/in-fence/out-fence/, and s/~0ULL/8/. I picked 8 as an invalid address !=
NULL.

And the testcase need to cover all possible combinations of output event
generation, i.e. out-fence, event and out-fence+event. So 3x3=9 testcases
for this I think.

> +
> +		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>  	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
> @@ -532,6 +557,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
> @@ -1506,11 +1533,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 dma_fence *fence, uint64_t user_data)
> +		struct drm_device *dev, uint64_t user_data)
>  {
>  	struct drm_pending_vblank_event *e = NULL;
> -	int ret;
>  
>  	e = kzalloc(sizeof *e, GFP_KERNEL);
>  	if (!e)
> @@ -1520,17 +1545,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;
>  }
>  
> @@ -1635,6 +1649,148 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
> +					struct drm_crtc_state *crtc_state)
> +{
> +	struct dma_fence *fence;
> +
> +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence)
> +		return NULL;
> +
> +	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> +		       crtc->fence_context, ++crtc->fence_seqno);
> +
> +	return fence;
> +}
> +
> +struct drm_out_fence_state {
> +	u64 __user *out_fence_ptr;

You have a type mess here. The pointer is to an u64, but the value you
store there is an int. Since we want to avoid fund with 32bit userspace
vs. 64bit kernel I think it should be an s64 everywhere.
> +	struct sync_file *sync_file;
> +	int fd;
> +};
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> +			   struct dma_fence *fence)
> +{
> +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fence_state->fd < 0)
> +		return fence_state->fd;
> +
> +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +		return -EFAULT;
> +
> +	fence_state->sync_file = sync_file_create(fence);
> +	if(!fence_state->sync_file)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static int prepare_crtc_signaling(struct drm_device *dev,
> +				  struct drm_atomic_state *state,
> +				  struct drm_mode_atomic *arg,
> +				  struct drm_file *file_priv,
> +				  struct drm_out_fence_state *fence_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int i, ret, fence_idx = 0;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		u64 __user *fence_ptr;
> +
> +		fence_ptr = drm_atomic_get_out_fence_for_crtc(crtc_state->state,
> +							      crtc);
> +
> +		if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY && fence_ptr) {
> +			if (put_user(-1, fence_ptr))
> +				return -EFAULT;
> +
> +			continue;
> +		}
> +
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> +			struct drm_pending_vblank_event *e;
> +
> +			e = create_vblank_event(dev, arg->user_data);
> +			if (!e)
> +				return -ENOMEM;
> +
> +			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;
> +				return ret;
> +			}
> +		}
> +
> +		if (fence_ptr) {
> +			struct dma_fence *fence;
> +
> +			fence_state[fence_idx].out_fence_ptr = fence_ptr;
> +
> +			fence = get_crtc_fence(crtc, crtc_state);
> +			if (!fence)
> +				return -ENOMEM;
> +
> +			ret = setup_out_fence(&fence_state[fence_idx], fence);
> +			if (ret) {
> +				dma_fence_put(fence);
> +				return ret;
> +			}
> +
> +			fence_idx++;
> +
> +			crtc_state->event->base.fence = fence;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void unprepare_crtc_signaling(struct drm_device *dev,
> +				     struct drm_atomic_state *state,
> +				     struct drm_out_fence_state *fence_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	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 (crtc_state->event)
> +			drm_event_cancel_free(dev,
> +					      &crtc_state->event->base);
> +	}
> +
> +	for (i = 0; fence_state[i].out_fence_ptr; i++) {

This goes boom if you have fences set for every crtc, because then this
check will walk past the end of the array and do something undefined. You
need to manually count how many of these slots are set (and might want to
switch to a krealloc pattern while at it). Sounds like it needs an igt.

> +		if (fence_state[i].sync_file)
> +			fput(fence_state[i].sync_file->file);
> +		if (fence_state[i].fd >= 0)
> +			put_unused_fd(fence_state[i].fd);
> +
> +		/* If this fails log error to the user */
> +		if (fence_state[i].out_fence_ptr &&
> +		    put_user(-1, fence_state[i].out_fence_ptr))
> +			DRM_INFO("Couldn't clear out_fence_ptr\n");

Userspace can provoke this (race an munmap against an atomic ioctl), and
userspace is not allowed to spam dmesg. Please degrade to
DRM_DEBUG_ATOMIC.

> +	}
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1647,8 +1803,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	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;
> @@ -1766,21 +1921,18 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		drm_mode_object_unreference(obj);
>  	}
>  
> -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			struct drm_pending_vblank_event *e;
> -
> -			e = create_vblank_event(dev, file_priv, NULL,
> -						arg->user_data);
> -			if (!e) {
> -				ret = -ENOMEM;
> -				goto out;
> -			}
> -
> -			crtc_state->event = e;
> -		}
> +	fence_state = kzalloc(dev->mode_config.num_crtc * sizeof(*fence_state),
> +			      GFP_KERNEL);
> +	if (!fence_state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
>  
> +	ret = prepare_crtc_signaling(dev, state, arg, file_priv,
> +					  fence_state);

Personally I'd pass &fence_state here and only krealloc within prepare if
needed. And as mentioned you need an &num_fences here too.

> +	if (ret)
> +		goto out;
> +
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>  		/*
>  		 * Unlike commit, check_only does not clean up state.
> @@ -1793,23 +1945,20 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		ret = drm_atomic_commit(state);
>  	}
>  
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; fence_state[i].sync_file && i < dev->mode_config.num_crtc;
> +	     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) {
> -		/*
> -		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> -		 * if they weren't, this code should be called on success
> -		 * for TEST_ONLY too.
> -		 */
> -
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			if (!crtc_state->event)
> -				continue;
> +	if (ret && fence_state)
> +		unprepare_crtc_signaling(dev, state, fence_state);

I'm tempted to rework this to:

	complete_crtc_signalling(dev, state, fence_state, ret)

and push the check for fence_state == NULL into it. And also use ret to
decide whether we need to clean up, or fd_install the fence. That way
everything is in one place. That would also tidy up the control flow a
bit. And s/unprepare/complete since it wouldn't just be the error path
cleanup any more.

>  
> -			drm_event_cancel_free(dev, &crtc_state->event->base);
> -		}
> -	}
> +	kfree(fence_state);
>  
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e2a06c8..a18d81b 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_atomic.h b/include/drm/drm_atomic.h
> index 2d1e9c9..ebde60e 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,6 +144,7 @@ struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state;
>  	struct drm_crtc_commit *commit;
> +	u64 __user *out_fence_ptr;

Same confusion about pointer types here as in fence_state.

>  };
>  
>  struct __drm_connnectors_state {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 30f2401..a5a05e8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -773,7 +773,7 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
>  	return container_of(fence->lock, struct drm_crtc, fence_lock);
>  }
>  
> -/**
> +/*

Bogus hunk I think here, this breaks the kerneldoc.

>   * 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
> @@ -1251,6 +1251,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. Userspace should provide a u64 pointer.

"... should provide a pointer to a value of type s64, and then cast that
pointer to u64." The type of the value pointed to and the pointer itself
aren't the same!

> +	 */
> +	struct drm_property *prop_out_fence_ptr;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> -- 
> 2.5.5
> 

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

Powered by blists - more mailing lists