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

Gustavo Padovan gustavo.padovan at collabora.com
Fri Nov 11 13:27:38 UTC 2016


Hi Brian,

2016-11-11 Brian Starkey <brian.starkey at arm.com>:

> Hi Gustavo,
> 
> On Fri, Nov 11, 2016 at 02:16:09PM +0900, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan at collabora.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
> > 
> > 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 fail in this case
> 
> 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()
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> 
> From an integration with writeback point of view, this looks fine to
> me - I can add writeback to this easily.
> 
> A few comments below though:
> 
> > ---
> > drivers/gpu/drm/drm_atomic.c | 242 +++++++++++++++++++++++++++++++++++--------
> > drivers/gpu/drm/drm_crtc.c   |   8 ++
> > include/drm/drm_atomic.h     |   1 +
> > include/drm/drm_crtc.h       |   6 ++
> > 4 files changed, 212 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 8e26ad1..cd39f13 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, u64 __user *fence_ptr)
> > +{
> > +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> > +}
> > +
> > +static u64 __user * get_out_fence_for_crtc(struct drm_atomic_state *state,
> > +					   struct drm_crtc *crtc)
> > +{
> > +	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;
> > +}
> > +
> 
> You missed a couple of s/u64/s64/ in the two functions above.
> 
> > /**
> >  * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >  * @state: the CRTC whose incoming state to update
> > @@ -491,6 +508,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 (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*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
> > @@ -533,6 +560,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
> > @@ -1659,11 +1688,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)
> > @@ -1673,17 +1700,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;
> > }
> > 
> > @@ -1788,6 +1804,166 @@ 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 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 {
> > +	s64 __user *out_fence_ptr;
> > +	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,
> > +				  unsigned int *num_fences)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int i, ret;
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		u64 __user *fence_ptr;
> > +
> > +		fence_ptr = 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;
> > +			struct drm_out_fence_state *f;
> > +
> > +			f = krealloc(*fence_state, sizeof(**fence_state) *
> > +				     (*num_fences + 1), GFP_KERNEL);
> > +			if (!f)
> > +				return -ENOMEM;
> > +
> > +			f[*num_fences].out_fence_ptr = fence_ptr;
> > +
> > +			fence = get_crtc_fence(crtc);
> > +			if (!fence)
> > +				return -ENOMEM;
> > +
> > +			ret = setup_out_fence(&f[*num_fences], fence);
> > +			if (ret) {
> > +				dma_fence_put(fence);
> > +				return ret;
> > +			}
> 
> Looks to me that the cleanup path doesn't work here. If
> setup_out_fence fails, you bail out before incrementing num_fences,
> and so the cleanup loop will be an iteration short.
> 
> > +
> > +			(*num_fences)++;
> > +
> > +			*fence_state = f;
> 
> I think this is problematic too. If I understand krealloc right, when
> you bail out before here and f != *fence_state, then the old
> *fence_state will have been already freed, and f is going to leak.

Yes, it is broken, I missed that. I think for fence_state I can just
move the assignment a few lines above and for num_fences one way is to
increment it before returning on error.

> 
> > +			crtc_state->event->base.fence = fence;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void complete_crtc_signaling(struct drm_device *dev,
> > +				     struct drm_atomic_state *state,
> > +				     struct drm_out_fence_state *fence_state,
> > +				     unsigned int num_fences, int ret)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int i;
> > +
> > +	if (!ret) {
> > +		for (i = 0; i < num_fences; i++)
> > +			fd_install(fence_state[i].fd,
> > +				   fence_state[i].sync_file->file);
> > +		return;
> > +	}
> > +
> > +	if (!fence_state)
> > +		return;
> > +
> > +	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);
> > +	}
> 
> I think this event cleanup needs to be before the !fence_state check;
> userspace can ask for events but no fences, and if that fails then the
> events need to be freed.

Yes. In the previous version this was the correct flow, but with v8 the
clean up actually needs to happen before.

Gustavo



More information about the dri-devel mailing list