drm/msm/sde: toggle vblank using enable state in display thread

Add enabled flag to the CRTC to be able to track the enabled or
disabled status in the display thread instead of the atomic
state. The atomic state is swapped prior to the display thread
dispatch. This is before the display thread executes the enable
or disable operation, so the state is not properly reflected for
a running VSYNC work item.
Also, simplify the CRTC VBLANK function structure to use a
single common function call to update the CRTC's VBLANK
registration with the encoder to avoid extra refs and unrefs.

CRs-Fixed: 2071893
Change-Id: I9b473becec15427b03b5ebf0333e10e4911dfd9b
Signed-off-by: Lloyd Atkinson <latkinso@codeaurora.org>
Signed-off-by: Ray Zhang <rayz@codeaurora.org>

drivers/gpu/drm/msm/sde/sde_crtc.c

@@ -979,8 +979,10 @@ end:
  * _sde_crtc_vblank_enable_nolock - update power resource and vblank request
  * @sde_crtc: Pointer to sde crtc structure
  * @enable: Whether to enable/disable vblanks
+ *
+ * @Return: error code
  */
-static void _sde_crtc_vblank_enable_nolock(
+static int _sde_crtc_vblank_enable_no_lock(
 		struct sde_crtc *sde_crtc, bool enable)
 {
 	struct drm_device *dev;
@@ -988,10 +990,11 @@ static void _sde_crtc_vblank_enable_nolock(
 	struct drm_encoder *enc;
 	struct msm_drm_private *priv;
 	struct sde_kms *sde_kms;
+	int ret = 0;
 
 	if (!sde_crtc) {
 		SDE_ERROR("invalid crtc\n");
-		return;
+		return -EINVAL;
 	}
 
 	crtc = &sde_crtc->base;
@@ -1000,13 +1003,16 @@ static void _sde_crtc_vblank_enable_nolock(
 
 	if (!priv->kms) {
 		SDE_ERROR("invalid kms\n");
-		return;
+		return -EINVAL;
 	}
 	sde_kms = to_sde_kms(priv->kms);
 
 	if (enable) {
-		sde_power_resource_enable(&priv->phandle,
+		ret = sde_power_resource_enable(&priv->phandle,
 				sde_kms->core_client, true);
+		if (ret)
+			return ret;
+
 		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
 			if (enc->crtc != crtc)
 				continue;
@@ -1025,9 +1031,11 @@ static void _sde_crtc_vblank_enable_nolock(
 
 			sde_encoder_register_vblank_callback(enc, NULL, NULL);
 		}
-		sde_power_resource_enable(&priv->phandle,
+		ret = sde_power_resource_enable(&priv->phandle,
 				sde_kms->core_client, false);
 	}
+
+	return ret;
 }
 
 /**
@@ -1073,8 +1081,8 @@ static void _sde_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
 	if (sde_crtc->suspend == enable)
 		SDE_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
 				crtc->base.id, enable);
-	else if (atomic_read(&sde_crtc->vblank_refcount) != 0)
+	else if (sde_crtc->enabled && sde_crtc->vblank_requested)
-		_sde_crtc_vblank_enable_nolock(sde_crtc, !enable);
+		_sde_crtc_vblank_enable_no_lock(sde_crtc, !enable);
 
 	sde_crtc->suspend = enable;
 
@@ -1158,39 +1166,13 @@ static void sde_crtc_reset(struct drm_crtc *crtc)
 	crtc->state = &cstate->base;
 }
 
-static int _sde_crtc_vblank_no_lock(struct sde_crtc *sde_crtc, bool en)
-{
-	if (!sde_crtc) {
-		SDE_ERROR("invalid crtc\n");
-		return -EINVAL;
-	} else if (en && atomic_inc_return(&sde_crtc->vblank_refcount) == 1) {
-		SDE_DEBUG("crtc%d vblank enable\n", sde_crtc->base.base.id);
-		if (!sde_crtc->suspend)
-			_sde_crtc_vblank_enable_nolock(sde_crtc, true);
-	} else if (!en && atomic_read(&sde_crtc->vblank_refcount) < 1) {
-		SDE_ERROR("crtc%d invalid vblank disable\n",
-				sde_crtc->base.base.id);
-		return -EINVAL;
-	} else if (!en && atomic_dec_return(&sde_crtc->vblank_refcount) == 0) {
-		SDE_DEBUG("crtc%d vblank disable\n", sde_crtc->base.base.id);
-		if (!sde_crtc->suspend)
-			_sde_crtc_vblank_enable_nolock(sde_crtc, false);
-	} else {
-		SDE_DEBUG("crtc%d vblank %s refcount:%d\n",
-				sde_crtc->base.base.id,
-				en ? "enable" : "disable",
-				atomic_read(&sde_crtc->vblank_refcount));
-	}
-
-	return 0;
-}
-
 static void sde_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
 	struct sde_crtc *sde_crtc;
 	struct sde_kms *sde_kms;
 	struct msm_drm_private *priv;
+	int ret = 0;
 
 	if (!crtc || !crtc->dev || !crtc->state) {
 		SDE_ERROR("invalid crtc\n");
@@ -1210,17 +1192,19 @@ static void sde_crtc_disable(struct drm_crtc *crtc)
 		_sde_crtc_set_suspend(crtc, true);
 
 	mutex_lock(&sde_crtc->crtc_lock);
-	SDE_EVT32(DRMID(crtc));
+	SDE_EVT32(DRMID(crtc), sde_crtc->enabled, sde_crtc->suspend,
+			sde_crtc->vblank_requested);
 
-	if (atomic_read(&sde_crtc->vblank_refcount) && !sde_crtc->suspend) {
+	if (sde_crtc->enabled && !sde_crtc->suspend &&
-		SDE_ERROR("crtc%d invalid vblank refcount\n",
+			sde_crtc->vblank_requested) {
-				crtc->base.id);
+		ret = _sde_crtc_vblank_enable_no_lock(sde_crtc, false);
-		SDE_EVT32(DRMID(crtc), atomic_read(&sde_crtc->vblank_refcount));
+		if (ret)
-		while (atomic_read(&sde_crtc->vblank_refcount))
+			SDE_ERROR("%s vblank enable failed: %d\n",
-			if (_sde_crtc_vblank_no_lock(sde_crtc, false))
+				sde_crtc->name, ret);
-				break;
 	}
 
+	sde_crtc->enabled = false;
+
 	if (atomic_read(&sde_crtc->frame_pending)) {
 		/* release bandwidth and other resources */
 		SDE_ERROR("crtc%d invalid frame pending\n",
@@ -1255,6 +1239,7 @@ static void sde_crtc_enable(struct drm_crtc *crtc)
 	struct sde_hw_mixer_cfg cfg;
 	struct drm_encoder *encoder;
 	int i;
+	int ret = 0;
 
 	if (!crtc) {
 		SDE_ERROR("invalid crtc\n");
@@ -1283,6 +1268,19 @@ static void sde_crtc_enable(struct drm_crtc *crtc)
 				sde_crtc_request_flip_cb, (void *)crtc);
 	}
 
+	mutex_lock(&sde_crtc->crtc_lock);
+	SDE_EVT32(DRMID(crtc), sde_crtc->enabled, sde_crtc->suspend,
+			sde_crtc->vblank_requested);
+	if (!sde_crtc->enabled && !sde_crtc->suspend &&
+			sde_crtc->vblank_requested) {
+		ret = _sde_crtc_vblank_enable_no_lock(sde_crtc, true);
+		if (ret)
+			SDE_ERROR("%s vblank enable failed: %d\n",
+				sde_crtc->name, ret);
+	}
+	sde_crtc->enabled = true;
+	mutex_unlock(&sde_crtc->crtc_lock);
+
 	for (i = 0; i < sde_crtc->num_mixers; i++) {
 		lm = mixer[i].hw_lm;
 		cfg.out_width = sde_crtc_mixer_width(sde_crtc, mode);
@@ -1438,7 +1436,7 @@ end:
 int sde_crtc_vblank(struct drm_crtc *crtc, bool en)
 {
 	struct sde_crtc *sde_crtc;
-	int rc;
+	int ret;
 
 	if (!crtc) {
 		SDE_ERROR("invalid crtc\n");
@@ -1447,10 +1445,19 @@ int sde_crtc_vblank(struct drm_crtc *crtc, bool en)
 	sde_crtc = to_sde_crtc(crtc);
 
 	mutex_lock(&sde_crtc->crtc_lock);
-	rc = _sde_crtc_vblank_no_lock(sde_crtc, en);
+	SDE_EVT32(DRMID(&sde_crtc->base), en, sde_crtc->enabled,
+			sde_crtc->suspend, sde_crtc->vblank_requested);
+	if (sde_crtc->enabled && !sde_crtc->suspend) {
+		ret = _sde_crtc_vblank_enable_no_lock(sde_crtc, en);
+		if (ret)
+			SDE_ERROR("%s vblank enable failed: %d\n",
+					sde_crtc->name, ret);
+	}
+
+	sde_crtc->vblank_requested = en;
 	mutex_unlock(&sde_crtc->crtc_lock);
 
-	return rc;
+	return 0;
 }
 
 void sde_crtc_cancel_pending_flip(struct drm_crtc *crtc,
@@ -1763,8 +1770,7 @@ static int _sde_debugfs_status_show(struct seq_file *s, void *data)
 		sde_crtc->vblank_cb_time = ktime_set(0, 0);
 	}
 
-	seq_printf(s, "vblank_refcount:%d\n",
+	seq_printf(s, "vblank_enable:%d\n", sde_crtc->vblank_requested);
-			atomic_read(&sde_crtc->vblank_refcount));
 
 	mutex_unlock(&sde_crtc->crtc_lock);
 
@@ -1892,7 +1898,6 @@ struct drm_crtc *sde_crtc_init(struct drm_device *dev,
 
 	crtc = &sde_crtc->base;
 	crtc->dev = dev;
-	atomic_set(&sde_crtc->vblank_refcount, 0);
 
 	mutex_init(&sde_crtc->crtc_lock);
 	spin_lock_init(&sde_crtc->spin_lock);

drivers/gpu/drm/msm/sde/sde_crtc.h

@@ -81,8 +81,11 @@ struct sde_crtc_frame_event {
  * @debugfs_root  : Parent of debugfs node
  * @vblank_cb_count : count of vblank callback since last reset
  * @vblank_cb_time  : ktime at vblank count reset
- * @vblank_refcount : reference count for vblank enable request
+ * @vblank_requested : whether the user has requested vblank events
  * @suspend         : whether or not a suspend operation is in progress
+ * @enabled       : whether the SDE 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
  * @feature_list  : list of color processing features supported on a crtc
  * @active_list   : list of color processing features are active
  * @dirty_list    : list of color processing features are dirty
@@ -117,8 +120,9 @@ struct sde_crtc {
 
 	u32 vblank_cb_count;
 	ktime_t vblank_cb_time;
-	atomic_t vblank_refcount;
+	bool vblank_requested;
 	bool suspend;
+	bool enabled;
 
 	struct list_head feature_list;
 	struct list_head active_list;