Skip to contentSkip to conversation

drm/msm/mdp5: restore cursor state when enabling crtc

PATCH In review Rob Clark 2 participants 2 messages Oct 19, 2017 – Oct 20, 2017

Subsystems (3)
lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/1 drm/msm/mdp5: restore cursor state when enabling crtc

Since we enabled runtime PM, we cannot count on cursor registers to retain their values. This can result in situations where we think the cursor is enabled when we enable the CRTC but it is trying to scan out null (and the rest of cursor position/size is lost), resulting in faults and generally angering the hw when coming out of DPMS with a cursor enabled.

stable backport note: reverting 774e39ee3572 is also a suitable fix

Fixes 774e39ee3572 drm/msm/mdp5: Set up runtime PM for MDSS
---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 100 +++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 32 deletions(-)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +68 -32
index 6aa3a688d9a4..db5c460ba593 100644
@@ -61,12 +61,15 @@ struct mdp5_crtc {
 
 		/* current cursor being scanned out: */
 		struct drm_gem_object *scanout_bo;
+		uint64_t iova;
 		uint32_t width, height;
 		uint32_t x, y;
 	} cursor;
 };
 #define to_mdp5_crtc(x) container_of(x, struct mdp5_crtc, base)
 
+static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc);
+
 static struct mdp5_kms *get_kms(struct drm_crtc *crtc)
 {
 	struct msm_drm_private *priv = crtc->dev->dev_private;
@@ -449,6 +452,21 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	pm_runtime_get_sync(dev);
 
+	/* Restore cursor state, as it might have been lost with suspend: */
+	if (mdp5_crtc->cursor.iova) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
+		mdp5_crtc_restore_cursor(crtc);
+		spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
+
+		mdp5_ctl_set_cursor(mdp5_cstate->ctl,
+			&mdp5_cstate->pipeline, 0, true);
+	} else {
+		mdp5_ctl_set_cursor(mdp5_cstate->ctl,
+			&mdp5_cstate->pipeline, 0, false);
+	}
+
 	/* Restore vblank irq handling after power is enabled */
 	drm_crtc_vblank_on(crtc);
 
@@ -729,6 +747,50 @@ static void get_roi(struct drm_crtc *crtc, uint32_t *roi_w, uint32_t *roi_h)
 			mdp5_crtc->cursor.y);
 }
 
+static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
+{
+	struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
+	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
+	struct mdp5_kms *mdp5_kms = get_kms(crtc);
+	const enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
+	uint32_t blendcfg, stride;
+	uint32_t x, y, width, height;
+	uint32_t roi_w, roi_h;
+	int lm;
+
+	assert_spin_locked(&mdp5_crtc->cursor.lock);
+
+	lm = mdp5_cstate->pipeline.mixer->lm;
+
+	x = mdp5_crtc->cursor.x;
+	y = mdp5_crtc->cursor.y;
+	width = mdp5_crtc->cursor.width;
+	height = mdp5_crtc->cursor.height;
+
+	stride = width * drm_format_plane_cpp(DRM_FORMAT_ARGB8888, 0);
+
+	get_roi(crtc, &roi_w, &roi_h);
+
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
+			MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_IMG_SIZE(lm),
+			MDP5_LM_CURSOR_IMG_SIZE_SRC_H(height) |
+			MDP5_LM_CURSOR_IMG_SIZE_SRC_W(width));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
+			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
+			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
+			MDP5_LM_CURSOR_START_XY_Y_START(y) |
+			MDP5_LM_CURSOR_START_XY_X_START(x));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
+			mdp5_crtc->cursor.iova);
+
+	blendcfg = MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_EN;
+	blendcfg |= MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_ALPHA_SEL(cur_alpha);
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BLEND_CONFIG(lm), blendcfg);
+}
+
 static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
 		struct drm_file *file, uint32_t handle,
 		uint32_t width, uint32_t height)
@@ -741,13 +803,9 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
 	struct platform_device *pdev = mdp5_kms->pdev;
 	struct msm_kms *kms = &mdp5_kms->base.base;
 	struct drm_gem_object *cursor_bo, *old_bo = NULL;
-	uint32_t blendcfg, stride;
-	uint64_t cursor_addr;
 	struct mdp5_ctl *ctl;
-	int ret, lm;
-	enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
+	int ret;
 	uint32_t flush_mask = mdp_ctl_flush_mask_cursor(0);
-	uint32_t roi_w, roi_h;
 	bool cursor_enable = true;
 	unsigned long flags;
 
@@ -767,6 +825,7 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
 	if (!handle) {
 		DBG("Cursor off");
 		cursor_enable = false;
+		mdp5_crtc->cursor.iova = 0;
 		pm_runtime_get_sync(&pdev->dev);
 		goto set_cursor;
 	}
@@ -775,13 +834,11 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
 	if (!cursor_bo)
 		return -ENOENT;
 
-	ret = msm_gem_get_iova(cursor_bo, kms->aspace, &cursor_addr);
+	ret = msm_gem_get_iova(cursor_bo, kms->aspace,
+			&mdp5_crtc->cursor.iova);
 	if (ret)
 		return -EINVAL;
 
-	lm = mdp5_cstate->pipeline.mixer->lm;
-	stride = width * drm_format_plane_cpp(DRM_FORMAT_ARGB8888, 0);
-
 	pm_runtime_get_sync(&pdev->dev);
 
 	spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
@@ -791,22 +848,7 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
 	mdp5_crtc->cursor.width = width;
 	mdp5_crtc->cursor.height = height;
 
-	get_roi(crtc, &roi_w, &roi_h);
-
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
-			MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_IMG_SIZE(lm),
-			MDP5_LM_CURSOR_IMG_SIZE_SRC_H(height) |
-			MDP5_LM_CURSOR_IMG_SIZE_SRC_W(width));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
-			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
-			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm), cursor_addr);
-
-	blendcfg = MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_EN;
-	blendcfg |= MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_ALPHA_SEL(cur_alpha);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BLEND_CONFIG(lm), blendcfg);
+	mdp5_crtc_restore_cursor(crtc);
 
 	spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
 
@@ -835,7 +877,6 @@ static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	struct mdp5_kms *mdp5_kms = get_kms(crtc);
 	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
 	struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
-	uint32_t lm = mdp5_cstate->pipeline.mixer->lm;
 	uint32_t flush_mask = mdp_ctl_flush_mask_cursor(0);
 	uint32_t roi_w;
 	uint32_t roi_h;
@@ -857,12 +898,7 @@ static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	pm_runtime_get_sync(&mdp5_kms->pdev->dev);
 
 	spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
-			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
-			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
-			MDP5_LM_CURSOR_START_XY_Y_START(y) |
-			MDP5_LM_CURSOR_START_XY_X_START(x));
+	mdp5_crtc_restore_cursor(crtc);
 	spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
 
 	crtc_flush(crtc, flush_mask);
Signature · 1 line
2.13.6
Reviewed-by on v1 1/1
#1 lore raw
Quoted 172 lines · Rob Clark

Since we enabled runtime PM, we cannot count on cursor registers to retain their values. This can result in situations where we think the cursor is enabled when we enable the CRTC but it is trying to scan out null (and the rest of cursor position/size is lost), resulting in faults and generally angering the hw when coming out of DPMS with a cursor enabled.

stable backport note: reverting 774e39ee3572 is also a suitable fix

Fixes 774e39ee3572 drm/msm/mdp5: Set up runtime PM for MDSS
---
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 100 +++++++++++++++++++++----------
  1 file changed, 68 insertions(+), 32 deletions(-)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +67 -25
index 6aa3a688d9a4..db5c460ba593 100644
@@ -61,12 +61,15 @@ struct mdp5_crtc {
 
  		/* current cursor being scanned out: */
  		struct drm_gem_object *scanout_bo;
+		uint64_t iova;
  		uint32_t width, height;
  		uint32_t x, y;
  	} cursor;
  };
  #define to_mdp5_crtc(x) container_of(x, struct mdp5_crtc, base)
 
+static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc);
+
  static struct mdp5_kms *get_kms(struct drm_crtc *crtc)
  {
  	struct msm_drm_private *priv = crtc->dev->dev_private;
@@ -449,6 +452,21 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
 
  	pm_runtime_get_sync(dev);
 
+	/* Restore cursor state, as it might have been lost with suspend: */
+	if (mdp5_crtc->cursor.iova) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
+		mdp5_crtc_restore_cursor(crtc);
+		spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
+
+		mdp5_ctl_set_cursor(mdp5_cstate->ctl,
+			&mdp5_cstate->pipeline, 0, true);
+	} else {
+		mdp5_ctl_set_cursor(mdp5_cstate->ctl,
+			&mdp5_cstate->pipeline, 0, false);
+	}
+
  	/* Restore vblank irq handling after power is enabled */
  	drm_crtc_vblank_on(crtc);
 
@@ -729,6 +747,50 @@ static void get_roi(struct drm_crtc *crtc, uint32_t *roi_w, uint32_t *roi_h)
  			mdp5_crtc->cursor.y);
  }
 
+static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
+{
+	struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
+	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
+	struct mdp5_kms *mdp5_kms = get_kms(crtc);
+	const enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
+	uint32_t blendcfg, stride;
+	uint32_t x, y, width, height;
+	uint32_t roi_w, roi_h;
+	int lm;
+
+	assert_spin_locked(&mdp5_crtc->cursor.lock);
+
+	lm = mdp5_cstate->pipeline.mixer->lm;
+
+	x = mdp5_crtc->cursor.x;
+	y = mdp5_crtc->cursor.y;
+	width = mdp5_crtc->cursor.width;
+	height = mdp5_crtc->cursor.height;
+
+	stride = width * drm_format_plane_cpp(DRM_FORMAT_ARGB8888, 0);
+
+	get_roi(crtc, &roi_w, &roi_h);
+
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
+			MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_IMG_SIZE(lm),
+			MDP5_LM_CURSOR_IMG_SIZE_SRC_H(height) |
+			MDP5_LM_CURSOR_IMG_SIZE_SRC_W(width));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
+			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
+			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
+			MDP5_LM_CURSOR_START_XY_Y_START(y) |
+			MDP5_LM_CURSOR_START_XY_X_START(x));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
+			mdp5_crtc->cursor.iova);
+
+	blendcfg = MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_EN;
+	blendcfg |= MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_ALPHA_SEL(cur_alpha);
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BLEND_CONFIG(lm), blendcfg);
+}
+
  static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
  		struct drm_file *file, uint32_t handle,
  		uint32_t width, uint32_t height)
@@ -741,13 +803,9 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
  	struct platform_device *pdev = mdp5_kms->pdev;
  	struct msm_kms *kms = &mdp5_kms->base.base;
  	struct drm_gem_object *cursor_bo, *old_bo = NULL;
-	uint32_t blendcfg, stride;
-	uint64_t cursor_addr;
  	struct mdp5_ctl *ctl;
-	int ret, lm;
-	enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
+	int ret;
  	uint32_t flush_mask = mdp_ctl_flush_mask_cursor(0);
-	uint32_t roi_w, roi_h;
  	bool cursor_enable = true;
  	unsigned long flags;
 
@@ -767,6 +825,7 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
  	if (!handle) {
  		DBG("Cursor off");
  		cursor_enable = false;
+		mdp5_crtc->cursor.iova = 0;
  		pm_runtime_get_sync(&pdev->dev);
  		goto set_cursor;
  	}
@@ -775,13 +834,11 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
  	if (!cursor_bo)
  		return -ENOENT;
 
-	ret = msm_gem_get_iova(cursor_bo, kms->aspace, &cursor_addr);
+	ret = msm_gem_get_iova(cursor_bo, kms->aspace,
+			&mdp5_crtc->cursor.iova);
  	if (ret)
  		return -EINVAL;
 
-	lm = mdp5_cstate->pipeline.mixer->lm;
-	stride = width * drm_format_plane_cpp(DRM_FORMAT_ARGB8888, 0);
-
  	pm_runtime_get_sync(&pdev->dev);
 
  	spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
@@ -791,22 +848,7 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,
  	mdp5_crtc->cursor.width = width;
  	mdp5_crtc->cursor.height = height;
 
-	get_roi(crtc, &roi_w, &roi_h);
-
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
-			MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_IMG_SIZE(lm),
-			MDP5_LM_CURSOR_IMG_SIZE_SRC_H(height) |
-			MDP5_LM_CURSOR_IMG_SIZE_SRC_W(width));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
-			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
-			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm), cursor_addr);
-
-	blendcfg = MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_EN;
-	blendcfg |= MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_ALPHA_SEL(cur_alpha);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BLEND_CONFIG(lm), blendcfg);
+	mdp5_crtc_restore_cursor(crtc);
 
  	spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
 

drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) · line 882

Quoted 7 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +0 -1
@@ -835,7 +877,6 @@ static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
  	struct mdp5_kms *mdp5_kms = get_kms(crtc);
  	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
  	struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
-	uint32_t lm = mdp5_cstate->pipeline.mixer->lm;
  	uint32_t flush_mask = mdp_ctl_flush_mask_cursor(0);
  	uint32_t roi_w;
  	uint32_t roi_h;

I guess we could get rid of roi_w, roi_h and the call to get_roi() in this fucn too?

Otherwise:

Quoted 15 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +1 -6
@@ -857,12 +898,7 @@ static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
  	pm_runtime_get_sync(&mdp5_kms->pdev->dev);
 
  	spin_lock_irqsave(&mdp5_crtc->cursor.lock, flags);
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
-			MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
-			MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
-	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
-			MDP5_LM_CURSOR_START_XY_Y_START(y) |
-			MDP5_LM_CURSOR_START_XY_X_START(x));
+	mdp5_crtc_restore_cursor(crtc);
  	spin_unlock_irqrestore(&mdp5_crtc->cursor.lock, flags);
 
  	crtc_flush(crtc, flush_mask);
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project