diff mbox

drm/exynos: use atomic helper commit

Message ID 1484558003-18691-1-git-send-email-inki.dae@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Inki Dae Jan. 16, 2017, 9:13 a.m. UTC
This patch relpaces specific atomic commit function
with atomic helper commit one, which also includes
atomic_commit_tail callback for Exynos SoC becasue
crtc devices on Exynos SoC uses power domain device
so drm_atomic_helper_commit_planes should be called
after drm_atomic_helper_commit_modeset_enables.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
 drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +------------------------------
 drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
 3 files changed, 33 insertions(+), 111 deletions(-)

Comments

Tobias Jakobi Jan. 16, 2017, 11:22 a.m. UTC | #1
Inki Dae wrote:
> This patch relpaces specific atomic commit function
> with atomic helper commit one, which also includes
> atomic_commit_tail callback for Exynos SoC becasue
> crtc devices on Exynos SoC uses power domain device
> so drm_atomic_helper_commit_planes should be called
> after drm_atomic_helper_commit_modeset_enables.
The commit message needs fixing. I think I know my way around Exynos DRM
a bit, but reading this just confuses me.

In particular the first part can probably be dropped, since it only
describes what the patch does (and I can already see this from the diff
itself).
Also some spelling issues:
relpaces -> replaces
becasue - because

With best wishes,
Tobias


> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +------------------------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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 = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 3ec0535..9d0df00 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)
>  {
> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
> +
>  }
>  
>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 68d4142..1e10b73 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,33 @@ 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);
> +
> +	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 +230,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;
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gustavo Padovan Jan. 16, 2017, 7:08 p.m. UTC | #2
Hi Inki,

2017-01-16 Inki Dae <inki.dae@samsung.com>:

> This patch relpaces specific atomic commit function
> with atomic helper commit one, which also includes
> atomic_commit_tail callback for Exynos SoC becasue
> crtc devices on Exynos SoC uses power domain device
> so drm_atomic_helper_commit_planes should be called
> after drm_atomic_helper_commit_modeset_enables.

Indeed, the commit message needs fixing.

> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +------------------------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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);
>  	}
> -

Nitpick: I wouldn't include changes like this in the patch.

>  }
>  
>  static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 3ec0535..9d0df00 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.
> -	 */

Please move this comment to the commit_tail function instead of deleting
it.

> -
> -	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)
>  {
> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
> +

This change shouldn't be here too.

>  }
>  
>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 68d4142..1e10b73 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,33 @@ 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);
> +
> +	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 +230,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;

With all that fixed:

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Gustavo
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Jan. 16, 2017, 8:34 p.m. UTC | #3
Hi Inki,

Thank you for the patch.

On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
> This patch relpaces specific atomic commit function
> with atomic helper commit one, which also includes
> atomic_commit_tail callback for Exynos SoC becasue
> crtc devices on Exynos SoC uses power domain device
> so drm_atomic_helper_commit_planes should be called
> after drm_atomic_helper_commit_modeset_enables.

Please note that drm_atomic_helper_commit() is currently broken, its async 
commit support is subject to a race condition. Maarten's "[PATCH v3 0/7] 
drm/atomic: Add accessor macros for all atomic state" patch series is an 
attempt to fix that, I'll try to review it ASAP.

> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +---------------------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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 = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 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)
>  {
> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
> 
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
> +
>  }
> 
>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,33 @@ 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);
> +
> +	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 +230,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;
>  }
Inki Dae Jan. 17, 2017, 6:16 a.m. UTC | #4
2017년 01월 16일 20:22에 Tobias Jakobi 이(가) 쓴 글:
> Inki Dae wrote:
>> This patch relpaces specific atomic commit function
>> with atomic helper commit one, which also includes
>> atomic_commit_tail callback for Exynos SoC becasue
>> crtc devices on Exynos SoC uses power domain device
>> so drm_atomic_helper_commit_planes should be called
>> after drm_atomic_helper_commit_modeset_enables.
> The commit message needs fixing. I think I know my way around Exynos DRM
> a bit, but reading this just confuses me.

Seems I didn't describe it. Thanks for pointing. Will fix it.

Thanks.

> 
> In particular the first part can probably be dropped, since it only
> describes what the patch does (and I can already see this from the diff
> itself).
> Also some spelling issues:
> relpaces -> replaces
> becasue - because
> 
> With best wishes,
> Tobias
> 
> 
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +------------------------------
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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 = {
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 3ec0535..9d0df00 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)
>>  {
>> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>>  
>>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
>> +
>>  }
>>  
>>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> index 68d4142..1e10b73 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -187,11 +187,33 @@ 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);
>> +
>> +	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 +230,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;
>>  }
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Inki Dae Jan. 17, 2017, 6:20 a.m. UTC | #5
2017년 01월 17일 05:34에 Laurent Pinchart 이(가) 쓴 글:
> Hi Inki,
> 
> Thank you for the patch.
> 
> On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
>> This patch relpaces specific atomic commit function
>> with atomic helper commit one, which also includes
>> atomic_commit_tail callback for Exynos SoC becasue
>> crtc devices on Exynos SoC uses power domain device
>> so drm_atomic_helper_commit_planes should be called
>> after drm_atomic_helper_commit_modeset_enables.
> 
> Please note that drm_atomic_helper_commit() is currently broken, its async 
> commit support is subject to a race condition. Maarten's "[PATCH v3 0/7] 
> drm/atomic: Add accessor macros for all atomic state" patch series is an 
> attempt to fix that, I'll try to review it ASAP.

Thanks for share and if you give me a review, it would help me a lot.

> 
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +---------------------------
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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 = {
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 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)
>>  {
>> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>>
>>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
>> +
>>  }
>>
>>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
>> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -187,11 +187,33 @@ 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);
>> +
>> +	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 +230,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;
>>  }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Inki Dae Jan. 17, 2017, 8:20 a.m. UTC | #6
2017년 01월 17일 04:08에 Gustavo Padovan 이(가) 쓴 글:
> Hi Inki,
> 
> 2017-01-16 Inki Dae <inki.dae@samsung.com>:
> 
>> This patch relpaces specific atomic commit function
>> with atomic helper commit one, which also includes
>> atomic_commit_tail callback for Exynos SoC becasue
>> crtc devices on Exynos SoC uses power domain device
>> so drm_atomic_helper_commit_planes should be called
>> after drm_atomic_helper_commit_modeset_enables.
> 
> Indeed, the commit message needs fixing.

Right, above comment should be fixed. 

I will update the comment like below,
"because 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
before drm_atomic_helper_commit_planes function is called. atomic_commit_tail callback implements this call ordering."

> 
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +------------------------------
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>>  3 files changed, 33 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..47da612 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,7 +102,6 @@ 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);
>>  	}
>> -
> 
> Nitpick: I wouldn't include changes like this in the patch.
> 
>>  }
>>  
>>  static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 3ec0535..9d0df00 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.
>> -	 */
> 
> Please move this comment to the commit_tail function instead of deleting
> it.

I'm not sure why we have to keep above comment becuase this is why atomic_commit_tail callback is added.
For this, I will leave this as a description of this patch like I already left my comment above.

> 
>> -
>> -	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)
>>  {
>> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>>  
>>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
>> +
> 
> This change shouldn't be here too.
> 
>>  }
>>  
>>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> index 68d4142..1e10b73 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -187,11 +187,33 @@ 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);
>> +
>> +	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 +230,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;
> 
> With all that fixed:
> 
> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Thanks.

> 
> Gustavo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Jan. 17, 2017, 9:17 a.m. UTC | #7
Hi Inki,

Thank you for the patch.

On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
> This patch relpaces specific atomic commit function
> with atomic helper commit one, which also includes
> atomic_commit_tail callback for Exynos SoC becasue
> crtc devices on Exynos SoC uses power domain device
> so drm_atomic_helper_commit_planes should be called
> after drm_atomic_helper_commit_modeset_enables.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +---------------------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>  3 files changed, 33 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..47da612 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;
> +	}

You also need to handle events for exynos_drm_crtc_enable(). I'm not too 
familiar with the exynos drm driver, is that taken care of by event handling 
in exynos_crtc_atomic_flush() ?

You could also split this change into a separate patch with a clear 
explanation of why it's needed in the commit message.

>  }

[snip]

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c

[snip]

> -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.
> -	 */

There's a value in this comment, I would copy it to 
exynos_drm_atomic_commit_tail()

> -	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);
> -}

[snip]

> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
> 
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
> +

This isn't needed.

>  }
> 
>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,33 @@ 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);
> +
> +	drm_atomic_helper_commit_planes(dev, state,
> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);

The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change 
should go in a separate patch (or at least be documented in the commit 
message).

> +	drm_atomic_helper_commit_hw_done(state);
> +
> +	drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +	drm_atomic_helper_cleanup_planes(dev, state);
> +}

[snip]
Inki Dae Jan. 18, 2017, 2:17 a.m. UTC | #8
Hi Laurent,

2017년 01월 17일 18:17에 Laurent Pinchart 이(가) 쓴 글:
> Hi Inki,
> 
> Thank you for the patch.
> 
> On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
>> This patch relpaces specific atomic commit function
>> with atomic helper commit one, which also includes
>> atomic_commit_tail callback for Exynos SoC becasue
>> crtc devices on Exynos SoC uses power domain device
>> so drm_atomic_helper_commit_planes should be called
>> after drm_atomic_helper_commit_modeset_enables.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +---------------------------
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>>  3 files changed, 33 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..47da612 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;
>> +	}
> 
> You also need to handle events for exynos_drm_crtc_enable(). I'm not too

Is there any corner case? I think there should be no event which is not handled when exynos_drm_crtc_enable() is called.
 
> familiar with the exynos drm driver, is that taken care of by event handling 
> in exynos_crtc_atomic_flush() ?

Yes, exynos_crtc_atomic_flush() handles the event - making crtc->state->event to NULL and handing the event.

However, I think no need to handle the event at this funtion so relevant code in exynos_crtc_ctomic_flush() is not clear to me excepting setting crtc->state->event to NULL - without this code, hw_done function will say warning.
This patch would be a first step to clean up atomic interfaces of Exynos DRM driver.

> 
> You could also split this change into a separate patch with a clear 
> explanation of why it's needed in the commit message.
> 
>>  }
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> 
> [snip]
> 
>> -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.
>> -	 */
> 
> There's a value in this comment, I would copy it to 
> exynos_drm_atomic_commit_tail()

As I already left the comment at other email thread, I will update description of this patch instead because atomic kms helper already described why atomic_commit_tail callback is required - the driver using runtime PM should have different call order.
But.. ok, this is what Gustavo wanted also. Seems better to leave above comment because the more explanation, the better. 

> 
>> -	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);
>> -}
> 
> [snip]
> 
>> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>>
>>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
>> +
> 
> This isn't needed.
> 
>>  }
>>
>>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
>> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -187,11 +187,33 @@ 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);
>> +
>> +	drm_atomic_helper_commit_planes(dev, state,
>> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change 
> should go in a separate patch (or at least be documented in the commit 
> message).

Agree. I will update description of this patch about why DRM_PLANE_COMMIT_ACTIVE_ONLY flag is used.

Thanks.

> 
>> +	drm_atomic_helper_commit_hw_done(state);
>> +
>> +	drm_atomic_helper_wait_for_vblanks(dev, state);
>> +
>> +	drm_atomic_helper_cleanup_planes(dev, state);
>> +}
> 
> [snip]
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 2530bf5..47da612 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,7 +102,6 @@  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 = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 3ec0535..9d0df00 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)
 {
@@ -313,6 +204,7 @@  static void exynos_drm_preclose(struct drm_device *dev,
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		exynos_drm_crtc_cancel_page_flip(crtc, file);
+
 }
 
 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 68d4142..1e10b73 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -187,11 +187,33 @@  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);
+
+	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 +230,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;
 }