| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 20 Oct 2017 18:48:10 +0530
From: Archit Taneja <architt@...eaurora.org>
To: Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org
Cc: linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel.vetter@...ll.ch>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/mdp5: restore cursor state when enabling crtc
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@...il.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@...eaurora.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
Powered by blists - more mailing lists