Re: [PATCH v12 3/3] drm/fence: add out-fences support

[Chris Wilson]

On Tue, Nov 15, 2016 at 10:06:41PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> 
> 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
> 
> v7: Comments by Daniel Vetter
> 	- remove prefix from internal functions
> 	- make out_fence_ptr an s64 pointer
> 	- degrade DRM_INFO to DRM_DEBUG_ATOMIC when put_user fail
> 	- fix doc issues
> 	- filter out OUT_FENCE_PTR == NULL and do not fail in this case
> 	- add complete_crtc_signalling()
> 	- krealloc fence_state on demand
> 
>     Comment by Brian Starkey
> 	- remove unused crtc_state arg from get_out_fence()
> 
> v8: Comment by Brian Starkey
> 	- cancel events before check for !fence_state
> 	- convert a few lefovers u64 types for out_fence_ptr
> 	- fix memleak by assign fence_state earlier after realloc
> 	- proper accout num_fences in case of error
> 
> v9: Comment by Brian Starkey
> 	- memset last position of fence_state after krealloc
>     Comments by Sean Paul
> 	- pass install_fds in complete_crtc_signaling() instead of ret
> 
>      - put_user(-1, fence_ptr) when decoding props
> 
> v10: Comment by Brian Starkey
> 	- remove unneeded num_fences increment on error path
> 	- kfree fence_state after installing fences fd
> 
> v11: rebase against latest drm-misc
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> Reviewed-by: Brian Starkey <brian.starkey@xxxxxxx>
> Tested-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_atomic.c | 241 +++++++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_crtc.c   |   8 ++
>  include/drm/drm_atomic.h     |   1 +
>  include/drm/drm_crtc.h       |   6 ++
>  4 files changed, 211 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3ad780a..d4a92a9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -290,6 +290,23 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
> +static void set_out_fence_for_crtc(struct drm_atomic_state *state,
> +				   struct drm_crtc *crtc, s64 __user *fence_ptr)
> +{
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> +}
> +
> +static s64 __user * get_out_fence_for_crtc(struct drm_atomic_state *state,
> +					   struct drm_crtc *crtc)
> +{
> +	s64 __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
> @@ -494,6 +511,16 @@ 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) {
> +		s64 __user *fence_ptr = u64_to_user_ptr(val);
> +
> +		if (!fence_ptr)
> +			return 0;
> +
> +		if (put_user(-1, fence_ptr))
> +			return -EFAULT;
> +
> +		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
> @@ -536,6 +563,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
> @@ -1664,11 +1693,9 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
>   */
>  
>  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)
> @@ -1678,17 +1705,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;
>  }
>  
> @@ -1793,6 +1809,165 @@ 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)
> +{

return drm_crtc_fence_create(crtc);

(or possibly, drm_crtc_fence_get(), drm_crtc_timeline_advance() or
somesuch if we need finer control over fence_seqno)

Or if you want to embed it,

	struct our_fence *fence;

	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
	if (!fence)
		return NULL;
	
	drm_crtc_fence_init(crtc, &fence->base);

	return &fence->base;


Looks tidier than dumping all the fence construction here

> +	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> +		       crtc->fence_context, ++crtc->fence_seqno);

-- 
Chris Wilson, Intel Open Source Technology Centre