[PATCH v2] drm/exynos: use atomic helper commit

[Tobias Jakobi]

What about Laurent's comment stating that drm_atomic_helper_commit() is
broken at the moment? Shouldn't there be some kind of warning in the
commit message that this patch is only safe to apply once the fixes for
drm_atomic_helper_commit() have landed? I'm already seeing this getting
merged by accident, without Maarten's series even being reviewed.

The commit message looks much better now. Still some spelling issues remain:
implemention -> implementation


With best wishes,
Tobias


Inki Dae wrote:
> This patch replaces specific atomic commit function
> with atomic helper commit one.
> 
> For this, it removes existing atomic commit function
> and relevant code specific to Exynos DRM and makes
> atomic helper commit to be used instead.
> 
> Below are changes for the use of atomic helper commit:
> - add atomic_commit_tail callback specific to Exynos DRM
>   . default implemention of atomic helper doesn't mesh well
>     with runtime PM so the device driver which supports runtime
>     PM should call drm_atomic_helper_commit_modeset_enables function
>     prior to drm_atomic_helper_commit_planes function call.
>     atomic_commit_tail callback implements this call ordering.
> - allow plane commit only in case that CRTC device is enabled.
>   . for this, it calls atomic_helper_commit_planes function
>     with DRM_PLANE_COMMIT_ACTIVE_ONLY flag in atomic_commit_tail callback.
> 
> Changelog v1:
> - fix comment
> - fix trivial things
> - revive existing comment which explains why plane commit should be
>   called after crtc and encoder device are enabled.
> 
> Signed-off-by: Inki Dae <inki.dae at samsung.com>
> Reviewed-by: Gustavo Padovan <gustavo.padovan at collabora.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |  10 ++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 109 -------------------------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  32 ++++++++-
>  3 files changed, 40 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 2530bf5..8f13bce 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  
>  	if (exynos_crtc->ops->disable)
>  		exynos_crtc->ops->disable(exynos_crtc);
> +
> +	if (crtc->state->event && !crtc->state->active) {
> +		spin_lock_irq(&crtc->dev->event_lock);
> +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +		spin_unlock_irq(&crtc->dev->event_lock);
> +
> +		crtc->state->event = NULL;
> +	}
>  }
>  
>  static void
> @@ -94,9 +102,9 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
>  			drm_crtc_send_vblank_event(crtc, event);
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  	}
> -
>  }
>  
> +
>  static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>  	.enable		= exynos_drm_crtc_enable,
>  	.disable	= exynos_drm_crtc_disable,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 3ec0535..c8f3eeb 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -38,56 +38,6 @@
>  #define DRIVER_MAJOR	1
>  #define DRIVER_MINOR	0
>  
> -struct exynos_atomic_commit {
> -	struct work_struct	work;
> -	struct drm_device	*dev;
> -	struct drm_atomic_state *state;
> -	u32			crtcs;
> -};
> -
> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
> -{
> -	struct drm_device *dev = commit->dev;
> -	struct exynos_drm_private *priv = dev->dev_private;
> -	struct drm_atomic_state *state = commit->state;
> -
> -	drm_atomic_helper_commit_modeset_disables(dev, state);
> -
> -	drm_atomic_helper_commit_modeset_enables(dev, state);
> -
> -	/*
> -	 * Exynos can't update planes with CRTCs and encoders disabled,
> -	 * its updates routines, specially for FIMD, requires the clocks
> -	 * to be enabled. So it is necessary to handle the modeset operations
> -	 * *before* the commit_planes() step, this way it will always
> -	 * have the relevant clocks enabled to perform the update.
> -	 */
> -
> -	drm_atomic_helper_commit_planes(dev, state, 0);
> -
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> -	drm_atomic_helper_cleanup_planes(dev, state);
> -
> -	drm_atomic_state_put(state);
> -
> -	spin_lock(&priv->lock);
> -	priv->pending &= ~commit->crtcs;
> -	spin_unlock(&priv->lock);
> -
> -	wake_up_all(&priv->wait);
> -
> -	kfree(commit);
> -}
> -
> -static void exynos_drm_atomic_work(struct work_struct *work)
> -{
> -	struct exynos_atomic_commit *commit = container_of(work,
> -				struct exynos_atomic_commit, work);
> -
> -	exynos_atomic_commit_complete(commit);
> -}
> -
>  static struct device *exynos_drm_get_dma_device(void);
>  
>  static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
> @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev)
>  	dev->dev_private = NULL;
>  }
>  
> -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs)
> -{
> -	bool pending;
> -
> -	spin_lock(&priv->lock);
> -	pending = priv->pending & crtcs;
> -	spin_unlock(&priv->lock);
> -
> -	return pending;
> -}
> -
> -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state,
> -			 bool nonblock)
> -{
> -	struct exynos_drm_private *priv = dev->dev_private;
> -	struct exynos_atomic_commit *commit;
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	int i, ret;
> -
> -	commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> -	if (!commit)
> -		return -ENOMEM;
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (ret) {
> -		kfree(commit);
> -		return ret;
> -	}
> -
> -	/* This is the point of no return */
> -
> -	INIT_WORK(&commit->work, exynos_drm_atomic_work);
> -	commit->dev = dev;
> -	commit->state = state;
> -
> -	/* Wait until all affected CRTCs have completed previous commits and
> -	 * mark them as pending.
> -	 */
> -	for_each_crtc_in_state(state, crtc, crtc_state, i)
> -		commit->crtcs |= drm_crtc_mask(crtc);
> -
> -	wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs));
> -
> -	spin_lock(&priv->lock);
> -	priv->pending |= commit->crtcs;
> -	spin_unlock(&priv->lock);
> -
> -	drm_atomic_helper_swap_state(state, true);
> -
> -	drm_atomic_state_get(state);
> -	if (nonblock)
> -		schedule_work(&commit->work);
> -	else
> -		exynos_atomic_commit_complete(commit);
> -
> -	return 0;
> -}
> -
>  int exynos_atomic_check(struct drm_device *dev,
>  			struct drm_atomic_state *state)
>  {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 68d4142..c77a5ac 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,40 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>  	return exynos_fb->dma_addr[index];
>  }
>  
> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +
> +	drm_atomic_helper_commit_modeset_disables(dev, state);
> +
> +	drm_atomic_helper_commit_modeset_enables(dev, state);
> +
> +	/*
> +	 * Exynos can't update planes with CRTCs and encoders disabled,
> +	 * its updates routines, specially for FIMD, requires the clocks
> +	 * to be enabled. So it is necessary to handle the modeset operations
> +	 * *before* the commit_planes() step, this way it will always
> +	 * have the relevant clocks enabled to perform the update.
> +	 */
> +	drm_atomic_helper_commit_planes(dev, state,
> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +
> +	drm_atomic_helper_commit_hw_done(state);
> +
> +	drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +	drm_atomic_helper_cleanup_planes(dev, state);
> +}
> +
> +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
> +	.atomic_commit_tail = exynos_drm_atomic_commit_tail,
> +};
> +
>  static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
>  	.fb_create = exynos_user_fb_create,
>  	.output_poll_changed = exynos_drm_output_poll_changed,
>  	.atomic_check = exynos_atomic_check,
> -	.atomic_commit = exynos_atomic_commit,
> +	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
>  void exynos_drm_mode_config_init(struct drm_device *dev)
> @@ -208,4 +237,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
>  	dev->mode_config.max_height = 4096;
>  
>  	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
> +	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
>  }
>