Re: [PATCH 1/3] drm/atomic: move waiting for hw_done to a helper

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 09, 2017 at 06:06:56PM +0100, Maarten Lankhorst wrote:
> This will allow i915 to perform a wait on pending modeset using the
> same code.

I don't like commit messages that refer to the subject like this. I
can't even see the subject when I'm replying so I have approximately
zero idea what's being said here.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++----------
>  include/drm/drm_atomic_helper.h     |  1 +
>  2 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7b71ac48b8a4..3a8a46510c36 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1936,6 +1936,41 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
>  
>  /**
> + * drm_atomic_helper_wait_for_hw_done - wait for all previous hw updates to complete.
> + * @crtc: crtc to wait on.
> + *
> + * This function waits for all previous hardware updates queued on @crtc
> + * to complete.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc)
> +{
> +	struct drm_crtc_commit *commit;
> +	long ret;
> +
> +	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);
> +
> +	if (!commit)
> +		return 0;
> +
> +	ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
> +	drm_crtc_commit_put(commit);
> +
> +	if (ret == 0)
> +		return -EBUSY;
> +
> +	return ret > 0 ? 0 : ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_wait_for_hw_done);

Seems sane enough. I was pondering if we could share the code with the
other hw_done wait as well. But it grabs a different commit from the
list, so not sure if we'd end up with less code or not.

Hmm. maybe pass the commit index from the caller all the way from the
caller to preceeding_commit() (which would obviously need a rename)?

> +
> +/**
>   * drm_atomic_helper_swap_state - store atomic state into current sw state
>   * @state: atomic state
>   * @stall: stall for proceeding commits
> @@ -1976,26 +2011,14 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> -	struct drm_crtc_commit *commit;
>  
>  	if (stall) {
>  		for_each_crtc_in_state(state, crtc, 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);
> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
>  
> -			if (!commit)
> -				continue;
> -
> -			ret = wait_for_completion_timeout(&commit->hw_done,
> -							  10*HZ);
> -			if (ret == 0)
> +			if (ret < 0)
>  				DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
>  					  crtc->base.id, crtc->name);
> -			drm_crtc_commit_put(commit);
>  		}
>  	}
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4b2353dc34ba..e15d2fe7b274 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -77,6 +77,7 @@ void
>  drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state,
>  					 bool atomic);
>  
> +int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc);
>  void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  				  bool stall);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx