[PATCH 0/4] drm/msm: Use atomic helpers for PM suspend/resume

[PATCH 0/4] drm/msm: Use atomic helpers for PM suspend/resume

[Bruce Wang]

Patches 1-2: Changes msm_pm_resume implementation to use atomic helpers and
removes the previously used dpu_kms_pm_suspend/resume functions.
Patches 3-4: Removes reliance on knowledge about suspend state from crtc

Bruce Wang (4):
  drm/msm/dpu: Remove dpu_kms_pm_suspend/resume
  drm/msm: Cut dpu_kms hooks from msm_pm_suspend/resume
  drm/msm/dpu: Remove suspend state tracking from crtc
  drm/msm/dpu: Remove suspend_state from dpu_kms

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  61 +----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |   2 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 123 ----------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |  31 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |  15 +--
 drivers/gpu/drm/msm/msm_drv.c             |  28 ++---
 drivers/gpu/drm/msm/msm_kms.h             |   3 -
 7 files changed, 17 insertions(+), 246 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

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

[PATCH 1/4] drm/msm/dpu: Remove dpu_kms_pm_suspend/resume

[Bruce Wang]

PM resume was crashing during dpu_kms_pm_resume. This patch removes
dpu_kms_pm_suspend/resume so that msm_pm_suspend/resume uses the atomic
helpers instead (see next patch). This patch also removes
dpu_kms_is_suspend_blocked since it is never called.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 123 ------------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  15 ---
 2 files changed, 138 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..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)
@@ -873,8 +752,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/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 66d466628e2b..1991cbe424a3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -137,7 +137,6 @@ struct dpu_kms {
 
 	/* saved atomic state during system suspend */
 	struct drm_atomic_state *suspend_state;
-	bool suspend_block;
 
 	struct dpu_rm rm;
 	bool rm_init;
@@ -176,20 +175,6 @@ static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)
 	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;
-
-	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
-}
-
 /**
  * Debugfs functions - extra helper functions for debugfs support
  *
-- 
2.19.0.605.g01d371f741-goog

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

[PATCH 2/4] drm/msm: Cut dpu_kms hooks from msm_pm_suspend/resume

[Bruce Wang]

Removes the traces of the non-atomic helper calls in
msm_pm_suspend/resume since we just deleted those functions (see patch
1). Also removes the drm_kms_helper_poll_disable/enable calls, since
the DRM_CONNECTOR_POLL_CONNECT flag is never set so periodic polling
doesn't happen anyways.

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

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4904d0d41409..f987df7f10df 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 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

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

[PATCH 3/4] drm/msm/dpu: Remove suspend state tracking from crtc

[Bruce Wang]

dpu_crtc_enable/disable/reset are only being called upon
initialization/destruction of the device and as part of the PM
suspend/resume handling. Since core suspend and resume calls
are guarenteed to not overlap with one another we don't need to
track whether or not we're in a suspended state from inside the crtc.
This patch removes the suspend logic from the crtc, and removes the relevant
tracing from dpu_trace.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 61 ++---------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 15 +-----
 3 files changed, 7 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d4530d60767b..31e372554e0c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -815,35 +815,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
 	}
 }
 
-/**
- * _dpu_crtc_set_suspend - notify crtc of suspend enable/disable
- * @crtc: Pointer to drm crtc object
- * @enable: true to enable suspend, false to indicate resume
- */
-static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
-{
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-
-	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
-
-	mutex_lock(&dpu_crtc->crtc_lock);
-
-	/*
-	 * If the vblank is enabled, release a power reference on suspend
-	 * and take it back during resume (if it is still enabled).
-	 */
-	trace_dpu_crtc_set_suspend(DRMID(&dpu_crtc->base), enable, dpu_crtc);
-	if (dpu_crtc->suspend == enable)
-		DPU_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
-				crtc->base.id, enable);
-	else if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
-		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
-	}
-
-	dpu_crtc->suspend = enable;
-	mutex_unlock(&dpu_crtc->crtc_lock);
-}
-
 /**
  * dpu_crtc_duplicate_state - state duplicate hook
  * @crtc: Pointer to drm crtc structure
@@ -881,25 +852,14 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
  */
 static void dpu_crtc_reset(struct drm_crtc *crtc)
 {
-	struct dpu_crtc *dpu_crtc;
 	struct dpu_crtc_state *cstate;
 
-	if (!crtc) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
-
-	/* revert suspend actions, if necessary */
-	if (dpu_kms_is_suspend_state(crtc->dev))
-		_dpu_crtc_set_suspend(crtc, false);
-
 	/* remove previous state, if present */
 	if (crtc->state) {
 		dpu_crtc_destroy_state(crtc, crtc->state);
-		crtc->state = 0;
+		crtc->state = NULL;
 	}
 
-	dpu_crtc = to_dpu_crtc(crtc);
 	cstate = kzalloc(sizeof(*cstate), GFP_KERNEL);
 	if (!cstate) {
 		DPU_ERROR("failed to allocate state\n");
@@ -951,9 +911,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 
-	if (dpu_kms_is_suspend_state(crtc->dev))
-		_dpu_crtc_set_suspend(crtc, true);
-
 	/* Disable/save vblank irq handling */
 	drm_crtc_vblank_off(crtc);
 
@@ -966,8 +923,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 				atomic_read(&dpu_crtc->frame_pending));
 
 	trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc);
-	if (dpu_crtc->enabled && !dpu_crtc->suspend &&
-			dpu_crtc->vblank_requested) {
+	if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
 		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
 	}
 	dpu_crtc->enabled = false;
@@ -1033,8 +989,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
-	if (!dpu_crtc->enabled && !dpu_crtc->suspend &&
-			dpu_crtc->vblank_requested) {
+	if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) {
 		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
 	}
 	dpu_crtc->enabled = true;
@@ -1289,17 +1244,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 
 int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 {
-	struct dpu_crtc *dpu_crtc;
-
-	if (!crtc) {
-		DPU_ERROR("invalid crtc\n");
-		return -EINVAL;
-	}
-	dpu_crtc = to_dpu_crtc(crtc);
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
-	if (dpu_crtc->enabled && !dpu_crtc->suspend) {
+	if (dpu_crtc->enabled) {
 		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
 	}
 	dpu_crtc->vblank_requested = en;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 3723b4830335..7dc181aabb4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -133,7 +133,6 @@ struct dpu_crtc_frame_event {
  * @play_count    : frame count between crtc enable and disable
  * @vblank_cb_time  : ktime at vblank count reset
  * @vblank_requested : whether the user has requested vblank events
- * @suspend         : whether or not a suspend operation is in progress
  * @enabled       : whether the DPU CRTC is currently enabled. updated in the
  *                  commit-thread, not state-swap time which is earlier, so
  *                  safe to make decisions on during VBLANK on/off work
@@ -169,7 +168,6 @@ struct dpu_crtc {
 	u64 play_count;
 	ktime_t vblank_cb_time;
 	bool vblank_requested;
-	bool suspend;
 	bool enabled;
 
 	struct list_head feature_list;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 636b31b0d311..0c122e173892 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -749,7 +749,6 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
 		__field(	uint32_t,		enc_id	)
 		__field(	bool,			enable	)
 		__field(	bool,			enabled )
-		__field(	bool,			suspend )
 		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
@@ -757,15 +756,12 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
 		__entry->enc_id = enc_id;
 		__entry->enable = enable;
 		__entry->enabled = crtc->enabled;
-		__entry->suspend = crtc->suspend;
 		__entry->vblank_requested = crtc->vblank_requested;
 	),
-	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
-		  "vblank_req:%s}",
+	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s vblank_req:%s}",
 		  __entry->drm_id, __entry->enc_id,
 		  __entry->enable ? "true" : "false",
 		  __entry->enabled ? "true" : "false",
-		  __entry->suspend ? "true" : "false",
 		  __entry->vblank_requested ? "true" : "false")
 );
 
@@ -776,26 +772,19 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
 		__field(	uint32_t,		drm_id	)
 		__field(	bool,			enable	)
 		__field(	bool,			enabled )
-		__field(	bool,			suspend )
 		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->enable = enable;
 		__entry->enabled = crtc->enabled;
-		__entry->suspend = crtc->suspend;
 		__entry->vblank_requested = crtc->vblank_requested;
 	),
-	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s vblank_req:%s}",
+	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
 		  __entry->drm_id, __entry->enable ? "true" : "false",
 		  __entry->enabled ? "true" : "false",
-		  __entry->suspend ? "true" : "false",
 		  __entry->vblank_requested ? "true" : "false")
 );
-DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
-	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
-	TP_ARGS(drm_id, enable, crtc)
-);
 DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
 	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
 	TP_ARGS(drm_id, enable, crtc)
-- 
2.19.0.605.g01d371f741-goog

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

[PATCH 4/4] drm/msm/dpu: Remove suspend_state from dpu_kms

[Bruce Wang]

The suspend_state field is no longer used and can be removed along with
dpu_kms_is_suspend_state.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 1991cbe424a3..2a3625eef6d3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -135,9 +135,6 @@ struct dpu_kms {
 
 	struct dpu_core_perf perf;
 
-	/* saved atomic state during system suspend */
-	struct drm_atomic_state *suspend_state;
-
 	struct dpu_rm rm;
 	bool rm_init;
 
@@ -162,19 +159,6 @@ struct vsync_info {
 #define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \
 		((struct msm_drm_private *)((D)->dev_private))->kms : NULL)
 
-/**
- * dpu_kms_is_suspend_state - whether or not the system is pm suspended
- * @dev: Pointer to drm device
- * Return: Suspend status
- */
-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;
-}
-
 /**
  * Debugfs functions - extra helper functions for debugfs support
  *
-- 
2.19.0.605.g01d371f741-goog

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

Re: [PATCH 1/4] drm/msm/dpu: Remove dpu_kms_pm_suspend/resume

[Sean Paul]

On Fri, Oct 05, 2018 at 11:48:40AM -0400, Bruce Wang wrote:
> PM resume was crashing during dpu_kms_pm_resume. This patch removes
> dpu_kms_pm_suspend/resume so that msm_pm_suspend/resume uses the atomic
> helpers instead (see next patch). This patch also removes
> dpu_kms_is_suspend_blocked since it is never called.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 123 ------------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  15 ---
>  2 files changed, 138 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..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)
> @@ -873,8 +752,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/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 66d466628e2b..1991cbe424a3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -137,7 +137,6 @@ struct dpu_kms {
>  
>  	/* saved atomic state during system suspend */
>  	struct drm_atomic_state *suspend_state;
> -	bool suspend_block;
>  
>  	struct dpu_rm rm;
>  	bool rm_init;
> @@ -176,20 +175,6 @@ static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)
>  	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;
> -
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
> -}
> -
>  /**
>   * Debugfs functions - extra helper functions for debugfs support
>   *
> -- 
> 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/4] drm/msm: Cut dpu_kms hooks from msm_pm_suspend/resume

[Sean Paul]

On Fri, Oct 05, 2018 at 11:48:41AM -0400, Bruce Wang wrote:
> Removes the traces of the non-atomic helper calls in
> msm_pm_suspend/resume since we just deleted those functions (see patch
> 1). Also removes the drm_kms_helper_poll_disable/enable calls, since
> the DRM_CONNECTOR_POLL_CONNECT flag is never set so periodic polling
> doesn't happen anyways.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 28 ++++++++++------------------
>  drivers/gpu/drm/msm/msm_kms.h |  3 ---
>  2 files changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4904d0d41409..f987df7f10df 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;

Hmm, I don't think we want to completely abort suspending if it didn't succeed
previously. It would be easier if we just scrubbed the value if it's not valid.
So something like:

        if (WARN_ON(priv->pm_state))
                drm_atomic_state_put(priv->pm_state);

        priv->pm_state = drm_atomic_helper_suspend(ddev);
        if (IS_ERR(priv->pm_state)) {
                int ret = PTR_ERR(priv->pm_state);
                DRM_ERROR("Failed to suspend dpu, %d\n", ret);
                return ret;
        }

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

If you take my suggestion above, this can become:

        if (WARN_ON(!priv->pm_state))
                return -ENOENT;

> +		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)

nit: This is usually expressed as:

        if (!ret)

> +		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 1/4] drm/msm/dpu: Remove dpu_kms_pm_suspend/resume

[Sean Paul]

On Fri, Oct 05, 2018 at 01:27:54PM -0400, Sean Paul wrote:
> On Fri, Oct 05, 2018 at 11:48:40AM -0400, Bruce Wang wrote:
> > PM resume was crashing during dpu_kms_pm_resume. This patch removes
> > dpu_kms_pm_suspend/resume so that msm_pm_suspend/resume uses the atomic
> > helpers instead (see next patch). This patch also removes
> > dpu_kms_is_suspend_blocked since it is never called.
> > 
> > Signed-off-by: Bruce Wang <bzwang@chromium.org>
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

One more thing. This series should have the prefix '[PATCH v2]' and should
contain a 'Changes in v2:' section detailing what's different from the first
version.

Sean

> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 123 ------------------------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  15 ---
> >  2 files changed, 138 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..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)
> > @@ -873,8 +752,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/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index 66d466628e2b..1991cbe424a3 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -137,7 +137,6 @@ struct dpu_kms {
> >  
> >  	/* saved atomic state during system suspend */
> >  	struct drm_atomic_state *suspend_state;
> > -	bool suspend_block;
> >  
> >  	struct dpu_rm rm;
> >  	bool rm_init;
> > @@ -176,20 +175,6 @@ static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)
> >  	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;
> > -
> > -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
> > -}
> > -
> >  /**
> >   * Debugfs functions - extra helper functions for debugfs support
> >   *
> > -- 
> > 2.19.0.605.g01d371f741-goog
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH 3/4] drm/msm/dpu: Remove suspend state tracking from crtc

[Sean Paul]

On Fri, Oct 05, 2018 at 11:48:42AM -0400, Bruce Wang wrote:
> dpu_crtc_enable/disable/reset are only being called upon
> initialization/destruction of the device and as part of the PM
> suspend/resume handling.

*and dpms/enable on/off


> Since core suspend and resume calls
> are guarenteed to not overlap with one another we don't need to

*guaranteed

It also might be more clear to just state "Since drm core's modeset locks
serialize atomic commits we don't need to..."


> track whether or not we're in a suspended state from inside the crtc.
> This patch removes the suspend logic from the crtc, and removes the relevant
> tracing from dpu_trace.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 61 ++---------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 15 +-----
>  3 files changed, 7 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d4530d60767b..31e372554e0c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -815,35 +815,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  	}
>  }
>  
> -/**
> - * _dpu_crtc_set_suspend - notify crtc of suspend enable/disable
> - * @crtc: Pointer to drm crtc object
> - * @enable: true to enable suspend, false to indicate resume
> - */
> -static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
> -{
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -
> -	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
> -
> -	mutex_lock(&dpu_crtc->crtc_lock);
> -
> -	/*
> -	 * If the vblank is enabled, release a power reference on suspend
> -	 * and take it back during resume (if it is still enabled).
> -	 */
> -	trace_dpu_crtc_set_suspend(DRMID(&dpu_crtc->base), enable, dpu_crtc);
> -	if (dpu_crtc->suspend == enable)
> -		DPU_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
> -				crtc->base.id, enable);
> -	else if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
> -		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
> -	}
> -
> -	dpu_crtc->suspend = enable;
> -	mutex_unlock(&dpu_crtc->crtc_lock);
> -}
> -
>  /**
>   * dpu_crtc_duplicate_state - state duplicate hook
>   * @crtc: Pointer to drm crtc structure
> @@ -881,25 +852,14 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
>   */
>  static void dpu_crtc_reset(struct drm_crtc *crtc)
>  {

Can you add a patch to the series to delete this function entirely and use
drm_atomic_helper_crtc_reset in dpu_crtc_funcs instead?

> -	struct dpu_crtc *dpu_crtc;
>  	struct dpu_crtc_state *cstate;
>  
> -	if (!crtc) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
> -

Since we're going to delete this function entirely, please also rollback the
cleanups in it so the diff is a bit more easy to read.

> -	/* revert suspend actions, if necessary */
> -	if (dpu_kms_is_suspend_state(crtc->dev))
> -		_dpu_crtc_set_suspend(crtc, false);
> -
>  	/* remove previous state, if present */
>  	if (crtc->state) {
>  		dpu_crtc_destroy_state(crtc, crtc->state);
> -		crtc->state = 0;
> +		crtc->state = NULL;
>  	}
>  
> -	dpu_crtc = to_dpu_crtc(crtc);
>  	cstate = kzalloc(sizeof(*cstate), GFP_KERNEL);
>  	if (!cstate) {
>  		DPU_ERROR("failed to allocate state\n");
> @@ -951,9 +911,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  
>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>  
> -	if (dpu_kms_is_suspend_state(crtc->dev))
> -		_dpu_crtc_set_suspend(crtc, true);
> -
>  	/* Disable/save vblank irq handling */
>  	drm_crtc_vblank_off(crtc);
>  
> @@ -966,8 +923,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  				atomic_read(&dpu_crtc->frame_pending));
>  
>  	trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc);
> -	if (dpu_crtc->enabled && !dpu_crtc->suspend &&
> -			dpu_crtc->vblank_requested) {
> +	if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
>  	}
>  	dpu_crtc->enabled = false;
> @@ -1033,8 +989,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> -	if (!dpu_crtc->enabled && !dpu_crtc->suspend &&
> -			dpu_crtc->vblank_requested) {
> +	if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
>  	}
>  	dpu_crtc->enabled = true;
> @@ -1289,17 +1244,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>  {
> -	struct dpu_crtc *dpu_crtc;
> -
> -	if (!crtc) {
> -		DPU_ERROR("invalid crtc\n");
> -		return -EINVAL;
> -	}
> -	dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
> -	if (dpu_crtc->enabled && !dpu_crtc->suspend) {
> +	if (dpu_crtc->enabled) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
>  	}
>  	dpu_crtc->vblank_requested = en;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 3723b4830335..7dc181aabb4d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -133,7 +133,6 @@ struct dpu_crtc_frame_event {
>   * @play_count    : frame count between crtc enable and disable
>   * @vblank_cb_time  : ktime at vblank count reset
>   * @vblank_requested : whether the user has requested vblank events
> - * @suspend         : whether or not a suspend operation is in progress
>   * @enabled       : whether the DPU CRTC is currently enabled. updated in the
>   *                  commit-thread, not state-swap time which is earlier, so
>   *                  safe to make decisions on during VBLANK on/off work
> @@ -169,7 +168,6 @@ struct dpu_crtc {
>  	u64 play_count;
>  	ktime_t vblank_cb_time;
>  	bool vblank_requested;
> -	bool suspend;
>  	bool enabled;
>  
>  	struct list_head feature_list;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 636b31b0d311..0c122e173892 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -749,7 +749,6 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
>  		__field(	uint32_t,		enc_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			suspend )
>  		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
> @@ -757,15 +756,12 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
>  		__entry->enc_id = enc_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->suspend = crtc->suspend;
>  		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
> -		  "vblank_req:%s}",
> +	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s vblank_req:%s}",
>  		  __entry->drm_id, __entry->enc_id,
>  		  __entry->enable ? "true" : "false",
>  		  __entry->enabled ? "true" : "false",
> -		  __entry->suspend ? "true" : "false",
>  		  __entry->vblank_requested ? "true" : "false")
>  );
>  
> @@ -776,26 +772,19 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
>  		__field(	uint32_t,		drm_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			suspend )
>  		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->suspend = crtc->suspend;
>  		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s vblank_req:%s}",
> +	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
>  		  __entry->drm_id, __entry->enable ? "true" : "false",
>  		  __entry->enabled ? "true" : "false",
> -		  __entry->suspend ? "true" : "false",
>  		  __entry->vblank_requested ? "true" : "false")
>  );
> -DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
> -	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
> -	TP_ARGS(drm_id, enable, crtc)
> -);
>  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
>  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
>  	TP_ARGS(drm_id, enable, crtc)
> -- 
> 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 4/4] drm/msm/dpu: Remove suspend_state from dpu_kms

[Sean Paul]

On Fri, Oct 05, 2018 at 11:48:43AM -0400, Bruce Wang wrote:
> The suspend_state field is no longer used and can be removed along with
> dpu_kms_is_suspend_state.
> 

This can be rolled up into patch 3, since that's where the usage of these is
being removed.

Sean

> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 1991cbe424a3..2a3625eef6d3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -135,9 +135,6 @@ struct dpu_kms {
>  
>  	struct dpu_core_perf perf;
>  
> -	/* saved atomic state during system suspend */
> -	struct drm_atomic_state *suspend_state;
> -
>  	struct dpu_rm rm;
>  	bool rm_init;
>  
> @@ -162,19 +159,6 @@ struct vsync_info {
>  #define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \
>  		((struct msm_drm_private *)((D)->dev_private))->kms : NULL)
>  
> -/**
> - * dpu_kms_is_suspend_state - whether or not the system is pm suspended
> - * @dev: Pointer to drm device
> - * Return: Suspend status
> - */
> -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;
> -}
> -
>  /**
>   * Debugfs functions - extra helper functions for debugfs support
>   *
> -- 
> 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