Re: [PATCH 1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tuesday, 29 August 2017 17:02:02 EEST Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortunately we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
> 
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  7 ---
>  drivers/gpu/drm/drm_atomic_helper.c | 92 ++++++++--------------------------
>  include/drm/drm_atomic.h            |  1 -
>  include/drm/drm_crtc.h              |  9 ++++
>  4 files changed, 32 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct
> drm_atomic_state *state) crtc->funcs->atomic_destroy_state(crtc,
>  						  state->crtcs[i].state);
> 
> -		if (state->crtcs[i].commit) {
> -			kfree(state->crtcs[i].commit->event);
> -			state->crtcs[i].commit->event = NULL;
> -			drm_crtc_commit_put(state->crtcs[i].commit);
> -		}
> -
> -		state->crtcs[i].commit = NULL;
>  		state->crtcs[i].ptr = NULL;
>  		state->crtcs[i].state = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 4e53aae9a1fb..9c2888cb4081
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  					  struct drm_atomic_state *old_state)
>  {
> -	struct drm_crtc_state *unused;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	int i;
> 
> -	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> -		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> +	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		struct drm_crtc_commit *commit = new_crtc_state->commit;
>  		int ret;
> 
>  		if (!commit)
> @@ -1388,11 +1388,10 @@ int drm_atomic_helper_async_check(struct drm_device
> *dev, {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
>  	struct drm_plane *__plane, *plane = NULL;
>  	struct drm_plane_state *__plane_state, *plane_state = NULL;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
> 
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
> @@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device
> *dev, return -EINVAL;
> 
>  	/*
> -	 * Don't do an async update if there is an outstanding commit modifying
> +	 * XXX: Don't do an async update if there is an outstanding commit
> modifying
> 	 * the plane.  This prevents our async update's changes from getting
> 	 * overridden by a previous synchronous update's state.
>  	 */

This looks like work in progress. The code is gone and the comment left. I'm 
not sure what 'XXX' means. If it's a TODO, patch 2/2 doesn't address it. And 
if you really meant to drop this check permanently, the reason should be 
explained in the commit message.

> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> -
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {
> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
> 
>  	return funcs->atomic_async_check(plane, plane_state);
>  }
> @@ -1731,7 +1707,7 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state, kref_init(&commit->ref);
>  		commit->crtc = crtc;
> 
> -		state->crtcs[i].commit = commit;
> +		new_crtc_state->commit = commit;
> 
>  		ret = stall_checks(crtc, nonblock);
>  		if (ret)
> @@ -1769,22 +1745,6 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state, }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> 
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> -	struct drm_crtc_commit *commit;
> -	int i = 0;
> -
> -	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> -		/* skip the first entry, that's the current commit */
> -		if (i == 1)
> -			return commit;
> -		i++;
> -	}
> -
> -	return NULL;
> -}
> -
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding
> commits * @old_state: atomic state object with old state structures
> @@ -1800,17 +1760,13 @@ static struct drm_crtc_commit
> *preceeding_commit(struct drm_crtc *crtc) void
> drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
> 
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		spin_lock(&crtc->commit_lock);
> -		commit = preceeding_commit(crtc);
> -		if (commit)
> -			drm_crtc_commit_get(commit);
> -		spin_unlock(&crtc->commit_lock);
> +	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		commit = old_crtc_state->commit;
> 
>  		if (!commit)
>  			continue;
> @@ -1828,8 +1784,6 @@ void drm_atomic_helper_wait_for_dependencies(struct
> drm_atomic_state *old_state) if (ret == 0)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
> -
> -		drm_crtc_commit_put(commit);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> @@ -1857,7 +1811,7 @@ void drm_atomic_helper_commit_hw_done(struct
> drm_atomic_state *old_state) int i;
> 
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (!commit)
>  			continue;
> 
> @@ -1888,7 +1842,7 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *old_state) long ret;
> 
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (WARN_ON(!commit))
>  			continue;
> 
> @@ -2294,20 +2248,13 @@ int drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, struct drm_private_state *old_obj_state,
> *new_obj_state;
> 
>  	if (stall) {
> -		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -			spin_lock(&crtc->commit_lock);
> -			commit = list_first_entry_or_null(&crtc->commit_list,
> -					struct drm_crtc_commit, commit_entry);
> -			if (commit)
> -				drm_crtc_commit_get(commit);
> -			spin_unlock(&crtc->commit_lock);
> +		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			commit = old_crtc_state->commit;
> 
>  			if (!commit)
>  				continue;
> 
>  			ret = wait_for_completion_interruptible(&commit->hw_done);
> -			drm_crtc_commit_put(commit);
> -
>  			if (ret)
>  				return ret;
>  		}
> @@ -2332,13 +2279,13 @@ int drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, state->crtcs[i].state = old_crtc_state;
>  		crtc->state = new_crtc_state;
> 
> -		if (state->crtcs[i].commit) {
> +		if (new_crtc_state->commit) {
>  			spin_lock(&crtc->commit_lock);
> -			list_add(&state->crtcs[i].commit->commit_entry,
> +			list_add(&new_crtc_state->commit->commit_entry,
>  				 &crtc->commit_list);
>  			spin_unlock(&crtc->commit_lock);
> 
> -			state->crtcs[i].commit->event = NULL;
> +			new_crtc_state->commit->event = NULL;
>  		}
>  	}
> 
> @@ -3186,6 +3133,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct
> drm_crtc *crtc, state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
>  	state->zpos_changed = false;
> +	state->commit = NULL;
>  	state->event = NULL;
>  	state->pageflip_flags = 0;
>  }
> @@ -3224,6 +3172,12 @@
> EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); */
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>  {
> +	if (state->commit) {
> +		kfree(state->commit->event);
> +		state->commit->event = NULL;
> +		drm_crtc_commit_put(state->commit);
> +	}
> +
>  	drm_property_blob_put(state->mode_blob);
>  	drm_property_blob_put(state->degamma_lut);
>  	drm_property_blob_put(state->ctm);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8a5808eb5628..e76d9f95c191 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,7 +144,6 @@ struct __drm_planes_state {
>  struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state, *old_state, *new_state;
> -	struct drm_crtc_commit *commit;
>  	s32 __user *out_fence_ptr;
>  	unsigned last_vblank_count;
>  };
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a642020e306..4cc7c9cdb6ca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -253,6 +253,15 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_pending_vblank_event *event;
> 
> +	/**
> +	 * @commit:
> +	 *
> +	 * This tracks how the commit for this update proceeds through the
> +	 * various phases. This is never cleared, except when we destroy the
> +	 * state, so that subsequent commits can synchronize with previous ones.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_atomic_state *state;
>  };


-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx