[PATCH 1/2] drm/msm: Use atomic helpers for pm_suspend/resume

[PATCH 1/2] drm/msm: Use atomic helpers for pm_suspend/resume

[Bruce Wang]

The dpu implementation of pm_resume were causing a crash. This patch
changes msm_pm_suspend and msm_pm_resume to use the atomic
helpers drm_atomic_helper_suspend and drm_atomic_helper_resume.
Removes the hooks needed for calling the dpu_kms implementations of
suspend/resume. Note that the poll_disable call is likely not needed as
of right now as the DRM_CONNECTOR_POLL_CONNECT flag is never set.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  2 --
 drivers/gpu/drm/msm/msm_drv.c           | 28 +++++++++----------------
 drivers/gpu/drm/msm/msm_kms.h           |  3 ---
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 0a683e65a9f3..e0142912d676 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -873,8 +873,6 @@ static const struct msm_kms_funcs kms_funcs = {
 	.check_modified_format = dpu_format_check_modified_format,
 	.get_format      = dpu_get_msm_format,
 	.round_pixclk    = dpu_kms_round_pixclk,
-	.pm_suspend      = dpu_kms_pm_suspend,
-	.pm_resume       = dpu_kms_pm_resume,
 	.destroy         = dpu_kms_destroy,
 	.set_encoder_mode = _dpu_kms_set_encoder_mode,
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c1abad8a8612..b8dc854c99f2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1069,37 +1069,29 @@ static int msm_pm_suspend(struct device *dev)
 {
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct msm_drm_private *priv = ddev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	/* TODO: Use atomic helper suspend/resume */
-	if (kms && kms->funcs && kms->funcs->pm_suspend)
-		return kms->funcs->pm_suspend(dev);
 
-	drm_kms_helper_poll_disable(ddev);
+	if (!IS_ERR_OR_NULL(priv->pm_state))
+		return 0;
 
 	priv->pm_state = drm_atomic_helper_suspend(ddev);
-	if (IS_ERR(priv->pm_state)) {
-		drm_kms_helper_poll_enable(ddev);
-		return PTR_ERR(priv->pm_state);
-	}
 
-	return 0;
+	return IS_ERR(priv->pm_state) ? PTR_ERR(priv->pm_state) : 0;
 }
 
 static int msm_pm_resume(struct device *dev)
 {
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct msm_drm_private *priv = ddev->dev_private;
-	struct msm_kms *kms = priv->kms;
+	int ret;
 
-	/* TODO: Use atomic helper suspend/resume */
-	if (kms && kms->funcs && kms->funcs->pm_resume)
-		return kms->funcs->pm_resume(dev);
+	if (IS_ERR_OR_NULL(priv->pm_state))
+		return 0;
 
-	drm_atomic_helper_resume(ddev, priv->pm_state);
-	drm_kms_helper_poll_enable(ddev);
+	ret = drm_atomic_helper_resume(ddev, priv->pm_state);
+	if (ret == 0)
+		priv->pm_state = NULL;
 
-	return 0;
+	return ret;
 }
 #endif
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index fd88cebb6adb..2b81b43a4bab 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -67,9 +67,6 @@ struct msm_kms_funcs {
 	void (*set_encoder_mode)(struct msm_kms *kms,
 				 struct drm_encoder *encoder,
 				 bool cmd_mode);
-	/* pm suspend/resume hooks */
-	int (*pm_suspend)(struct device *dev);
-	int (*pm_resume)(struct device *dev);
 	/* cleanup: */
 	void (*destroy)(struct msm_kms *kms);
 #ifdef CONFIG_DEBUG_FS
-- 
2.19.0.605.g01d371f741-goog

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 2/2] drm/msm/dpu: Remove all dpu pm_suspend/resume code

[Bruce Wang]

Removes the now uncalled pm_suspend/resume code. Also removes
dpu_kms_is_suspend_blocked which is never called, and changes
dpu_kms_is_suspend_state to work with the new msm_pm_suspend/resume
implementations.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 121 ------------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  23 +----
 2 files changed, 2 insertions(+), 142 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e0142912d676..ff06b50dfc87 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -709,127 +709,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
 	_dpu_kms_hw_destroy(dpu_kms);
 }
 
-static int dpu_kms_pm_suspend(struct device *dev)
-{
-	struct drm_device *ddev;
-	struct drm_modeset_acquire_ctx ctx;
-	struct drm_atomic_state *state;
-	struct dpu_kms *dpu_kms;
-	int ret = 0, num_crtcs = 0;
-
-	if (!dev)
-		return -EINVAL;
-
-	ddev = dev_get_drvdata(dev);
-	if (!ddev || !ddev_to_msm_kms(ddev))
-		return -EINVAL;
-
-	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
-
-	/* disable hot-plug polling */
-	drm_kms_helper_poll_disable(ddev);
-
-	/* acquire modeset lock(s) */
-	drm_modeset_acquire_init(&ctx, 0);
-
-retry:
-	DPU_ATRACE_BEGIN("kms_pm_suspend");
-
-	ret = drm_modeset_lock_all_ctx(ddev, &ctx);
-	if (ret)
-		goto unlock;
-
-	/* save current state for resume */
-	if (dpu_kms->suspend_state)
-		drm_atomic_state_put(dpu_kms->suspend_state);
-	dpu_kms->suspend_state = drm_atomic_helper_duplicate_state(ddev, &ctx);
-	if (IS_ERR_OR_NULL(dpu_kms->suspend_state)) {
-		DRM_ERROR("failed to back up suspend state\n");
-		dpu_kms->suspend_state = NULL;
-		goto unlock;
-	}
-
-	/* create atomic state to disable all CRTCs */
-	state = drm_atomic_state_alloc(ddev);
-	if (IS_ERR_OR_NULL(state)) {
-		DRM_ERROR("failed to allocate crtc disable state\n");
-		goto unlock;
-	}
-
-	state->acquire_ctx = &ctx;
-
-	/* check for nothing to do */
-	if (num_crtcs == 0) {
-		DRM_DEBUG("all crtcs are already in the off state\n");
-		drm_atomic_state_put(state);
-		goto suspended;
-	}
-
-	/* commit the "disable all" state */
-	ret = drm_atomic_commit(state);
-	if (ret < 0) {
-		DRM_ERROR("failed to disable crtcs, %d\n", ret);
-		drm_atomic_state_put(state);
-		goto unlock;
-	}
-
-suspended:
-	dpu_kms->suspend_block = true;
-
-unlock:
-	if (ret == -EDEADLK) {
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-
-	DPU_ATRACE_END("kms_pm_suspend");
-	return 0;
-}
-
-static int dpu_kms_pm_resume(struct device *dev)
-{
-	struct drm_device *ddev;
-	struct dpu_kms *dpu_kms;
-	int ret;
-
-	if (!dev)
-		return -EINVAL;
-
-	ddev = dev_get_drvdata(dev);
-	if (!ddev || !ddev_to_msm_kms(ddev))
-		return -EINVAL;
-
-	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
-
-	DPU_ATRACE_BEGIN("kms_pm_resume");
-
-	drm_mode_config_reset(ddev);
-
-	drm_modeset_lock_all(ddev);
-
-	dpu_kms->suspend_block = false;
-
-	if (dpu_kms->suspend_state) {
-		dpu_kms->suspend_state->acquire_ctx =
-			ddev->mode_config.acquire_ctx;
-		ret = drm_atomic_commit(dpu_kms->suspend_state);
-		if (ret < 0) {
-			DRM_ERROR("failed to restore state, %d\n", ret);
-			drm_atomic_state_put(dpu_kms->suspend_state);
-		}
-		dpu_kms->suspend_state = NULL;
-	}
-	drm_modeset_unlock_all(ddev);
-
-	/* enable hot-plug polling */
-	drm_kms_helper_poll_enable(ddev);
-
-	DPU_ATRACE_END("kms_pm_resume");
-	return 0;
-}
-
 static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
 				 struct drm_encoder *encoder,
 				 bool cmd_mode)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 66d466628e2b..b04aa222e150 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -135,10 +135,6 @@ struct dpu_kms {
 
 	struct dpu_core_perf perf;
 
-	/* saved atomic state during system suspend */
-	struct drm_atomic_state *suspend_state;
-	bool suspend_block;
-
 	struct dpu_rm rm;
 	bool rm_init;
 
@@ -170,24 +166,9 @@ struct vsync_info {
  */
 static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)
 {
-	if (!ddev_to_msm_kms(dev))
-		return false;
-
-	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_state != NULL;
-}
-
-/**
- * dpu_kms_is_suspend_blocked - whether or not commits are blocked due to pm
- *				suspend status
- * @dev: Pointer to drm device
- * Return: True if commits should be rejected due to pm suspend
- */
-static inline bool dpu_kms_is_suspend_blocked(struct drm_device *dev)
-{
-	if (!dpu_kms_is_suspend_state(dev))
-		return false;
+	struct msm_drm_private *priv = dev->dev_private;
 
-	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
+	return !IS_ERR_OR_NULL(priv->pm_state);
 }
 
 /**
-- 
2.19.0.605.g01d371f741-goog

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/2] drm/msm: Use atomic helpers for pm_suspend/resume

[Sean Paul]

On Tue, Oct 02, 2018 at 11:05:29AM -0400, Bruce Wang wrote:
> The dpu implementation of pm_resume were causing a crash. This patch
> changes msm_pm_suspend and msm_pm_resume to use the atomic
> helpers drm_atomic_helper_suspend and drm_atomic_helper_resume.
> Removes the hooks needed for calling the dpu_kms implementations of
> suspend/resume. Note that the poll_disable call is likely not needed as
> of right now as the DRM_CONNECTOR_POLL_CONNECT flag is never set.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  2 --
>  drivers/gpu/drm/msm/msm_drv.c           | 28 +++++++++----------------
>  drivers/gpu/drm/msm/msm_kms.h           |  3 ---
>  3 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0a683e65a9f3..e0142912d676 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -873,8 +873,6 @@ static const struct msm_kms_funcs kms_funcs = {
>  	.check_modified_format = dpu_format_check_modified_format,
>  	.get_format      = dpu_get_msm_format,
>  	.round_pixclk    = dpu_kms_round_pixclk,
> -	.pm_suspend      = dpu_kms_pm_suspend,
> -	.pm_resume       = dpu_kms_pm_resume,

Can you flip the order of these patches such that you remove the dpu
implementation entirely first, and then remove the unused bits in msm core? That
way the dpu and msm bits are clearly delineated.

Sean

>  	.destroy         = dpu_kms_destroy,
>  	.set_encoder_mode = _dpu_kms_set_encoder_mode,
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c1abad8a8612..b8dc854c99f2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1069,37 +1069,29 @@ static int msm_pm_suspend(struct device *dev)
>  {
>  	struct drm_device *ddev = dev_get_drvdata(dev);
>  	struct msm_drm_private *priv = ddev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -
> -	/* TODO: Use atomic helper suspend/resume */
> -	if (kms && kms->funcs && kms->funcs->pm_suspend)
> -		return kms->funcs->pm_suspend(dev);
>  
> -	drm_kms_helper_poll_disable(ddev);
> +	if (!IS_ERR_OR_NULL(priv->pm_state))
> +		return 0;
>  
>  	priv->pm_state = drm_atomic_helper_suspend(ddev);
> -	if (IS_ERR(priv->pm_state)) {
> -		drm_kms_helper_poll_enable(ddev);
> -		return PTR_ERR(priv->pm_state);
> -	}
>  
> -	return 0;
> +	return IS_ERR(priv->pm_state) ? PTR_ERR(priv->pm_state) : 0;
>  }
>  
>  static int msm_pm_resume(struct device *dev)
>  {
>  	struct drm_device *ddev = dev_get_drvdata(dev);
>  	struct msm_drm_private *priv = ddev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> +	int ret;
>  
> -	/* TODO: Use atomic helper suspend/resume */
> -	if (kms && kms->funcs && kms->funcs->pm_resume)
> -		return kms->funcs->pm_resume(dev);
> +	if (IS_ERR_OR_NULL(priv->pm_state))
> +		return 0;
>  
> -	drm_atomic_helper_resume(ddev, priv->pm_state);
> -	drm_kms_helper_poll_enable(ddev);
> +	ret = drm_atomic_helper_resume(ddev, priv->pm_state);
> +	if (ret == 0)
> +		priv->pm_state = NULL;
>  
> -	return 0;
> +	return ret;
>  }
>  #endif
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index fd88cebb6adb..2b81b43a4bab 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -67,9 +67,6 @@ struct msm_kms_funcs {
>  	void (*set_encoder_mode)(struct msm_kms *kms,
>  				 struct drm_encoder *encoder,
>  				 bool cmd_mode);
> -	/* pm suspend/resume hooks */
> -	int (*pm_suspend)(struct device *dev);
> -	int (*pm_resume)(struct device *dev);
>  	/* cleanup: */
>  	void (*destroy)(struct msm_kms *kms);
>  #ifdef CONFIG_DEBUG_FS
> -- 
> 2.19.0.605.g01d371f741-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 2/2] drm/msm/dpu: Remove all dpu pm_suspend/resume code

[Sean Paul]

On Tue, Oct 02, 2018 at 11:05:30AM -0400, Bruce Wang wrote:
> Removes the now uncalled pm_suspend/resume code. Also removes
> dpu_kms_is_suspend_blocked which is never called, and changes
> dpu_kms_is_suspend_state to work with the new msm_pm_suspend/resume
> implementations.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 121 ------------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  23 +----
>  2 files changed, 2 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e0142912d676..ff06b50dfc87 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -709,127 +709,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>  	_dpu_kms_hw_destroy(dpu_kms);
>  }
>  
> -static int dpu_kms_pm_suspend(struct device *dev)
> -{
> -	struct drm_device *ddev;
> -	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_atomic_state *state;
> -	struct dpu_kms *dpu_kms;
> -	int ret = 0, num_crtcs = 0;
> -
> -	if (!dev)
> -		return -EINVAL;
> -
> -	ddev = dev_get_drvdata(dev);
> -	if (!ddev || !ddev_to_msm_kms(ddev))
> -		return -EINVAL;
> -
> -	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
> -
> -	/* disable hot-plug polling */
> -	drm_kms_helper_poll_disable(ddev);
> -
> -	/* acquire modeset lock(s) */
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -retry:
> -	DPU_ATRACE_BEGIN("kms_pm_suspend");
> -
> -	ret = drm_modeset_lock_all_ctx(ddev, &ctx);
> -	if (ret)
> -		goto unlock;
> -
> -	/* save current state for resume */
> -	if (dpu_kms->suspend_state)
> -		drm_atomic_state_put(dpu_kms->suspend_state);
> -	dpu_kms->suspend_state = drm_atomic_helper_duplicate_state(ddev, &ctx);
> -	if (IS_ERR_OR_NULL(dpu_kms->suspend_state)) {
> -		DRM_ERROR("failed to back up suspend state\n");
> -		dpu_kms->suspend_state = NULL;
> -		goto unlock;
> -	}
> -
> -	/* create atomic state to disable all CRTCs */
> -	state = drm_atomic_state_alloc(ddev);
> -	if (IS_ERR_OR_NULL(state)) {
> -		DRM_ERROR("failed to allocate crtc disable state\n");
> -		goto unlock;
> -	}
> -
> -	state->acquire_ctx = &ctx;
> -
> -	/* check for nothing to do */
> -	if (num_crtcs == 0) {
> -		DRM_DEBUG("all crtcs are already in the off state\n");
> -		drm_atomic_state_put(state);
> -		goto suspended;
> -	}
> -
> -	/* commit the "disable all" state */
> -	ret = drm_atomic_commit(state);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to disable crtcs, %d\n", ret);
> -		drm_atomic_state_put(state);
> -		goto unlock;
> -	}
> -
> -suspended:
> -	dpu_kms->suspend_block = true;
> -
> -unlock:
> -	if (ret == -EDEADLK) {
> -		drm_modeset_backoff(&ctx);
> -		goto retry;
> -	}
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -
> -	DPU_ATRACE_END("kms_pm_suspend");
> -	return 0;
> -}
> -
> -static int dpu_kms_pm_resume(struct device *dev)
> -{
> -	struct drm_device *ddev;
> -	struct dpu_kms *dpu_kms;
> -	int ret;
> -
> -	if (!dev)
> -		return -EINVAL;
> -
> -	ddev = dev_get_drvdata(dev);
> -	if (!ddev || !ddev_to_msm_kms(ddev))
> -		return -EINVAL;
> -
> -	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
> -
> -	DPU_ATRACE_BEGIN("kms_pm_resume");
> -
> -	drm_mode_config_reset(ddev);
> -
> -	drm_modeset_lock_all(ddev);
> -
> -	dpu_kms->suspend_block = false;
> -
> -	if (dpu_kms->suspend_state) {
> -		dpu_kms->suspend_state->acquire_ctx =
> -			ddev->mode_config.acquire_ctx;
> -		ret = drm_atomic_commit(dpu_kms->suspend_state);
> -		if (ret < 0) {
> -			DRM_ERROR("failed to restore state, %d\n", ret);
> -			drm_atomic_state_put(dpu_kms->suspend_state);
> -		}
> -		dpu_kms->suspend_state = NULL;
> -	}
> -	drm_modeset_unlock_all(ddev);
> -
> -	/* enable hot-plug polling */
> -	drm_kms_helper_poll_enable(ddev);
> -
> -	DPU_ATRACE_END("kms_pm_resume");
> -	return 0;
> -}
> -
>  static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>  				 struct drm_encoder *encoder,
>  				 bool cmd_mode)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 66d466628e2b..b04aa222e150 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -135,10 +135,6 @@ struct dpu_kms {
>  
>  	struct dpu_core_perf perf;
>  
> -	/* saved atomic state during system suspend */
> -	struct drm_atomic_state *suspend_state;
> -	bool suspend_block;
> -
>  	struct dpu_rm rm;
>  	bool rm_init;
>  
> @@ -170,24 +166,9 @@ struct vsync_info {
>   */
>  static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)

I have a feeling we should be getting rid of this. If not, it probably means
some thread is not being terminated on suspend and we're trying to change hw
after the core has suspended. Can you please audit the callsites and either
remove this, add appropriate thread joins/flushs/locking, or add a comment here
about why this is safe?

Thanks!


>  {
> -	if (!ddev_to_msm_kms(dev))
> -		return false;
> -
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_state != NULL;
> -}
> -
> -/**
> - * dpu_kms_is_suspend_blocked - whether or not commits are blocked due to pm
> - *				suspend status
> - * @dev: Pointer to drm device
> - * Return: True if commits should be rejected due to pm suspend
> - */
> -static inline bool dpu_kms_is_suspend_blocked(struct drm_device *dev)
> -{
> -	if (!dpu_kms_is_suspend_state(dev))
> -		return false;
> +	struct msm_drm_private *priv = dev->dev_private;
>  
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
> +	return !IS_ERR_OR_NULL(priv->pm_state);
>  }
>  
>  /**
> -- 
> 2.19.0.605.g01d371f741-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/2] drm/msm: Use atomic helpers for pm_suspend/resume

[Jordan Crouse]

On Tue, Oct 02, 2018 at 11:05:29AM -0400, Bruce Wang wrote:
> The dpu implementation of pm_resume were causing a crash. This patch
> changes msm_pm_suspend and msm_pm_resume to use the atomic
> helpers drm_atomic_helper_suspend and drm_atomic_helper_resume.
> Removes the hooks needed for calling the dpu_kms implementations of
> suspend/resume. Note that the poll_disable call is likely not needed as
> of right now as the DRM_CONNECTOR_POLL_CONNECT flag is never set.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  2 --
>  drivers/gpu/drm/msm/msm_drv.c           | 28 +++++++++----------------
>  drivers/gpu/drm/msm/msm_kms.h           |  3 ---
>  3 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0a683e65a9f3..e0142912d676 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -873,8 +873,6 @@ static const struct msm_kms_funcs kms_funcs = {
>  	.check_modified_format = dpu_format_check_modified_format,
>  	.get_format      = dpu_get_msm_format,
>  	.round_pixclk    = dpu_kms_round_pixclk,
> -	.pm_suspend      = dpu_kms_pm_suspend,
> -	.pm_resume       = dpu_kms_pm_resume,
>  	.destroy         = dpu_kms_destroy,
>  	.set_encoder_mode = _dpu_kms_set_encoder_mode,
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c1abad8a8612..b8dc854c99f2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1069,37 +1069,29 @@ static int msm_pm_suspend(struct device *dev)
>  {
>  	struct drm_device *ddev = dev_get_drvdata(dev);
>  	struct msm_drm_private *priv = ddev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -
> -	/* TODO: Use atomic helper suspend/resume */
> -	if (kms && kms->funcs && kms->funcs->pm_suspend)
> -		return kms->funcs->pm_suspend(dev);
>  
> -	drm_kms_helper_poll_disable(ddev);
> +	if (!IS_ERR_OR_NULL(priv->pm_state))
> +		return 0;
>  
>  	priv->pm_state = drm_atomic_helper_suspend(ddev);
> -	if (IS_ERR(priv->pm_state)) {
> -		drm_kms_helper_poll_enable(ddev);
> -		return PTR_ERR(priv->pm_state);
> -	}
>  
> -	return 0;
> +	return IS_ERR(priv->pm_state) ? PTR_ERR(priv->pm_state) : 0;

Nit: this can be PTR_ERR_OR_ZERO(priv->pm_state);

>  }
>  
>  static int msm_pm_resume(struct device *dev)
>  {
>  	struct drm_device *ddev = dev_get_drvdata(dev);
>  	struct msm_drm_private *priv = ddev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> +	int ret;
>  
> -	/* TODO: Use atomic helper suspend/resume */
> -	if (kms && kms->funcs && kms->funcs->pm_resume)
> -		return kms->funcs->pm_resume(dev);
> +	if (IS_ERR_OR_NULL(priv->pm_state))
> +		return 0;
>  
> -	drm_atomic_helper_resume(ddev, priv->pm_state);
> -	drm_kms_helper_poll_enable(ddev);
> +	ret = drm_atomic_helper_resume(ddev, priv->pm_state);
> +	if (ret == 0)
> +		priv->pm_state = NULL;
>  
> -	return 0;
> +	return ret;
>  }
>  #endif
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index fd88cebb6adb..2b81b43a4bab 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -67,9 +67,6 @@ struct msm_kms_funcs {
>  	void (*set_encoder_mode)(struct msm_kms *kms,
>  				 struct drm_encoder *encoder,
>  				 bool cmd_mode);
> -	/* pm suspend/resume hooks */
> -	int (*pm_suspend)(struct device *dev);
> -	int (*pm_resume)(struct device *dev);
>  	/* cleanup: */
>  	void (*destroy)(struct msm_kms *kms);
>  #ifdef CONFIG_DEBUG_FS
> -- 
> 2.19.0.605.g01d371f741-goog
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno