From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH] drm/msm/mdp5: restore cursor state when enabling crtc Date: Fri, 20 Oct 2017 18:48:10 +0530 Message-ID: References: <20171019200056.6039-1-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20171019200056.6039-1-robdclark@gmail.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Rob Clark , dri-devel@lists.freedesktop.org Cc: linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org 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 > --- > 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 > @@ -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