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

Archit Taneja architt at codeaurora.org
Fri Oct 20 13:18:10 UTC 2017



On 10/20/2017 01:30 AM, Rob Clark wrote:
> 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
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
>   drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 100 +++++++++++++++++++++----------
>   1 file changed, 68 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index 6aa3a688d9a4..db5c460ba593 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -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;

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

Otherwise:
Reviewed-by: Archit Taneja <architt at codeaurora.org>

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


More information about the dri-devel mailing list