Message-ID: <20161020153518.GX4329@intel.com>
Date:   Thu, 20 Oct 2016 18:35:18 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Gustavo Padovan <gustavo@...ovan.org>
Cc:     dri-devel@...ts.freedesktop.org, marcheu@...gle.com,
        Daniel Stone <daniels@...labora.com>, seanpaul@...gle.com,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        linux-kernel@...r.kernel.org, laurent.pinchart@...asonboard.com,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
        John Harrison <John.C.Harrison@...el.com>, m.chehab@...sung.com
Subject: Re: [PATCH v5 4/4] drm/fence: add out-fences support

On Thu, Oct 20, 2016 at 12:50:05PM -0200, 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.

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@...labora.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@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Powered by blists - more mailing lists