From: Rob Clark <robdclark@gmail.com> To: dri-devel@lists.freedesktop.org Cc: "Jordan Crouse" <jordan@cosmicpenguin.net>, "Rob Clark" <robdclark@chromium.org>, "Stephen Boyd" <sboyd@kernel.org>, "John Stultz" <john.stultz@linaro.org>, "Rob Clark" <robdclark@gmail.com>, "Sean Paul" <sean@poorly.run>, "David Airlie" <airlied@linux.ie>, "Daniel Vetter" <daniel@ffwll.ch>, "Abhinav Kumar" <abhinavk@codeaurora.org>, "Maxime Ripard" <maxime@cerno.tech>, "Thomas Zimmermann" <tzimmermann@suse.de>, "Stephen Boyd" <swboyd@chromium.org>, "Kalyan Thota" <kalyan_t@codeaurora.org>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Qinglang Miao" <miaoqinglang@huawei.com>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Lee Jones" <lee.jones@linaro.org>, "Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, linux-arm-msm@vger.kernel.org (open list:DRM DRIVER FOR MSM ADRENO GPU), freedreno@lists.freedesktop.org (open list:DRM DRIVER FOR MSM ADRENO GPU), linux-kernel@vger.kernel.org (open list) Subject: [PATCH] drm/msm/dpu: Delete bonkers code Date: Tue, 1 Jun 2021 15:47:19 -0700 [thread overview] Message-ID: <20210601224750.513996-2-robdclark@gmail.com> (raw) In-Reply-To: <20210601224750.513996-1-robdclark@gmail.com> From: Rob Clark <robdclark@chromium.org> dpu_crtc_atomic_flush() was directly poking it's attached planes in a code path that ended up in dpu_plane_atomic_update(), even if the plane was not involved in the current atomic update. While a bit dubious, this worked before because plane->state would always point to something valid. But now using drm_atomic_get_new_plane_state() we could get a NULL state pointer instead, leading to: [ 20.873273] Call trace: [ 20.875740] dpu_plane_atomic_update+0x5c/0xed0 [ 20.880311] dpu_plane_restore+0x40/0x88 [ 20.884266] dpu_crtc_atomic_flush+0xf4/0x208 [ 20.888660] drm_atomic_helper_commit_planes+0x150/0x238 [ 20.894014] msm_atomic_commit_tail+0x1d4/0x7a0 [ 20.898579] commit_tail+0xa4/0x168 [ 20.902102] drm_atomic_helper_commit+0x164/0x178 [ 20.906841] drm_atomic_commit+0x54/0x60 [ 20.910798] drm_atomic_connector_commit_dpms+0x10c/0x118 [ 20.916236] drm_mode_obj_set_property_ioctl+0x1e4/0x440 [ 20.921588] drm_connector_property_set_ioctl+0x60/0x88 [ 20.926852] drm_ioctl_kernel+0xd0/0x120 [ 20.930807] drm_ioctl+0x21c/0x478 [ 20.934235] __arm64_sys_ioctl+0xa8/0xe0 [ 20.938193] invoke_syscall+0x64/0x130 [ 20.941977] el0_svc_common.constprop.3+0x5c/0xe0 [ 20.946716] do_el0_svc+0x80/0xa0 [ 20.950058] el0_svc+0x20/0x30 [ 20.953145] el0_sync_handler+0x88/0xb0 [ 20.957014] el0_sync+0x13c/0x140 The reason for the codepath seems dubious, the atomic suspend/resume heplers should handle the power-collapse case. If not, the CRTC's atomic_check() should be adding the planes to the atomic update. Reported-by: Stephen Boyd <sboyd@kernel.org> Reported-by: John Stultz <john.stultz@linaro.org> Fixes: 37418bf14c13 drm: Use state helper instead of the plane state pointer Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 10 ---------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 16 ---------------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ------ 3 files changed, 32 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 7c29976be243..18bc76b7f1a3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -648,16 +648,6 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc, if (unlikely(!cstate->num_mixers)) return; - /* - * For planes without commit update, drm framework will not add - * those planes to current state since hardware update is not - * required. However, if those planes were power collapsed since - * last commit cycle, driver has to restore the hardware state - * of those planes explicitly here prior to plane flush. - */ - drm_atomic_crtc_for_each_plane(plane, crtc) - dpu_plane_restore(plane, state); - /* update performance setting before crtc kickoff */ dpu_core_perf_crtc_update(crtc, 1, false); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index df7f3d3afd8b..7a993547eb75 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -1258,22 +1258,6 @@ static void dpu_plane_atomic_update(struct drm_plane *plane, } } -void dpu_plane_restore(struct drm_plane *plane, struct drm_atomic_state *state) -{ - struct dpu_plane *pdpu; - - if (!plane || !plane->state) { - DPU_ERROR("invalid plane\n"); - return; - } - - pdpu = to_dpu_plane(plane); - - DPU_DEBUG_PLANE(pdpu, "\n"); - - dpu_plane_atomic_update(plane, state); -} - static void dpu_plane_destroy(struct drm_plane *plane) { struct dpu_plane *pdpu = plane ? to_dpu_plane(plane) : NULL; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index 03b6365a750c..34e03ac05f4a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -84,12 +84,6 @@ bool is_dpu_plane_virtual(struct drm_plane *plane); void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl, u32 *flush_sspp); -/** - * dpu_plane_restore - restore hw state if previously power collapsed - * @plane: Pointer to drm plane structure - */ -void dpu_plane_restore(struct drm_plane *plane, struct drm_atomic_state *state); - /** * dpu_plane_flush - final plane operations before commit flush * @plane: Pointer to drm plane structure -- 2.30.2
WARNING: multiple messages have this Message-ID (diff)
From: Rob Clark <robdclark@gmail.com> To: dri-devel@lists.freedesktop.org Cc: David Airlie <airlied@linux.ie>, Jordan Crouse <jordan@cosmicpenguin.net>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Lee Jones <lee.jones@linaro.org>, Rob Clark <robdclark@chromium.org>, Qinglang Miao <miaoqinglang@huawei.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, Abhinav Kumar <abhinavk@codeaurora.org>, Stephen Boyd <swboyd@chromium.org>, Maxime Ripard <maxime@cerno.tech>, Kalyan Thota <kalyan_t@codeaurora.org>, Sean Paul <sean@poorly.run>, Stephen Boyd <sboyd@kernel.org>, open list <linux-kernel@vger.kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org> Subject: [PATCH] drm/msm/dpu: Delete bonkers code Date: Tue, 1 Jun 2021 15:47:19 -0700 [thread overview] Message-ID: <20210601224750.513996-2-robdclark@gmail.com> (raw) In-Reply-To: <20210601224750.513996-1-robdclark@gmail.com> From: Rob Clark <robdclark@chromium.org> dpu_crtc_atomic_flush() was directly poking it's attached planes in a code path that ended up in dpu_plane_atomic_update(), even if the plane was not involved in the current atomic update. While a bit dubious, this worked before because plane->state would always point to something valid. But now using drm_atomic_get_new_plane_state() we could get a NULL state pointer instead, leading to: [ 20.873273] Call trace: [ 20.875740] dpu_plane_atomic_update+0x5c/0xed0 [ 20.880311] dpu_plane_restore+0x40/0x88 [ 20.884266] dpu_crtc_atomic_flush+0xf4/0x208 [ 20.888660] drm_atomic_helper_commit_planes+0x150/0x238 [ 20.894014] msm_atomic_commit_tail+0x1d4/0x7a0 [ 20.898579] commit_tail+0xa4/0x168 [ 20.902102] drm_atomic_helper_commit+0x164/0x178 [ 20.906841] drm_atomic_commit+0x54/0x60 [ 20.910798] drm_atomic_connector_commit_dpms+0x10c/0x118 [ 20.916236] drm_mode_obj_set_property_ioctl+0x1e4/0x440 [ 20.921588] drm_connector_property_set_ioctl+0x60/0x88 [ 20.926852] drm_ioctl_kernel+0xd0/0x120 [ 20.930807] drm_ioctl+0x21c/0x478 [ 20.934235] __arm64_sys_ioctl+0xa8/0xe0 [ 20.938193] invoke_syscall+0x64/0x130 [ 20.941977] el0_svc_common.constprop.3+0x5c/0xe0 [ 20.946716] do_el0_svc+0x80/0xa0 [ 20.950058] el0_svc+0x20/0x30 [ 20.953145] el0_sync_handler+0x88/0xb0 [ 20.957014] el0_sync+0x13c/0x140 The reason for the codepath seems dubious, the atomic suspend/resume heplers should handle the power-collapse case. If not, the CRTC's atomic_check() should be adding the planes to the atomic update. Reported-by: Stephen Boyd <sboyd@kernel.org> Reported-by: John Stultz <john.stultz@linaro.org> Fixes: 37418bf14c13 drm: Use state helper instead of the plane state pointer Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 10 ---------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 16 ---------------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ------ 3 files changed, 32 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 7c29976be243..18bc76b7f1a3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -648,16 +648,6 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc, if (unlikely(!cstate->num_mixers)) return; - /* - * For planes without commit update, drm framework will not add - * those planes to current state since hardware update is not - * required. However, if those planes were power collapsed since - * last commit cycle, driver has to restore the hardware state - * of those planes explicitly here prior to plane flush. - */ - drm_atomic_crtc_for_each_plane(plane, crtc) - dpu_plane_restore(plane, state); - /* update performance setting before crtc kickoff */ dpu_core_perf_crtc_update(crtc, 1, false); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index df7f3d3afd8b..7a993547eb75 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -1258,22 +1258,6 @@ static void dpu_plane_atomic_update(struct drm_plane *plane, } } -void dpu_plane_restore(struct drm_plane *plane, struct drm_atomic_state *state) -{ - struct dpu_plane *pdpu; - - if (!plane || !plane->state) { - DPU_ERROR("invalid plane\n"); - return; - } - - pdpu = to_dpu_plane(plane); - - DPU_DEBUG_PLANE(pdpu, "\n"); - - dpu_plane_atomic_update(plane, state); -} - static void dpu_plane_destroy(struct drm_plane *plane) { struct dpu_plane *pdpu = plane ? to_dpu_plane(plane) : NULL; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index 03b6365a750c..34e03ac05f4a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -84,12 +84,6 @@ bool is_dpu_plane_virtual(struct drm_plane *plane); void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl, u32 *flush_sspp); -/** - * dpu_plane_restore - restore hw state if previously power collapsed - * @plane: Pointer to drm plane structure - */ -void dpu_plane_restore(struct drm_plane *plane, struct drm_atomic_state *state); - /** * dpu_plane_flush - final plane operations before commit flush * @plane: Pointer to drm plane structure -- 2.30.2
next prev parent reply other threads:[~2021-06-01 22:44 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-01 22:47 [PATCH v4 0/6] iommu/arm-smmu: adreno-smmu page fault handling Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark [this message] 2021-06-01 22:47 ` [PATCH] drm/msm/dpu: Delete bonkers code Rob Clark 2021-06-01 22:47 ` [PATCH v4 1/6] iommu/arm-smmu: Add support for driver IOMMU fault handlers Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` [PATCH v4 2/6] iommu/arm-smmu-qcom: Add an adreno-smmu-priv callback to get pagefault info Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` [PATCH v4 3/6] drm/msm: Improve the a6xx page fault handler Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` [PATCH v4 4/6] iommu/arm-smmu-qcom: Add stall support Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-01 22:47 ` [PATCH v4 5/6] drm/msm: Add crashdump support for stalled SMMU Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-08 15:12 ` Jordan Crouse 2021-06-08 15:12 ` Jordan Crouse 2021-06-09 21:46 ` Rob Clark 2021-06-09 21:46 ` Rob Clark 2021-06-01 22:47 ` [PATCH v4 6/6] drm/msm: devcoredump iommu fault support Rob Clark 2021-06-01 22:47 ` Rob Clark 2021-06-08 15:20 ` Jordan Crouse 2021-06-08 15:20 ` Jordan Crouse 2021-06-09 21:50 ` Rob Clark 2021-06-09 21:50 ` Rob Clark -- strict thread matches above, loose matches on Subject: below -- 2021-04-30 17:17 [PATCH] drm/msm/dpu: Delete bonkers code Rob Clark 2021-04-30 17:17 ` Rob Clark 2021-04-30 17:37 ` John Stultz 2021-04-30 17:37 ` John Stultz 2021-04-30 17:44 ` Stephen Boyd 2021-04-30 17:44 ` Stephen Boyd 2021-05-03 8:19 ` Maxime Ripard 2021-05-03 8:19 ` Maxime Ripard 2021-05-26 19:03 ` patchwork-bot+linux-arm-msm
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210601224750.513996-2-robdclark@gmail.com \ --to=robdclark@gmail.com \ --cc=abhinavk@codeaurora.org \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=john.stultz@linaro.org \ --cc=jordan@cosmicpenguin.net \ --cc=kalyan_t@codeaurora.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=lee.jones@linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maxime@cerno.tech \ --cc=miaoqinglang@huawei.com \ --cc=robdclark@chromium.org \ --cc=sakari.ailus@linux.intel.com \ --cc=sboyd@kernel.org \ --cc=sean@poorly.run \ --cc=swboyd@chromium.org \ --cc=tzimmermann@suse.de \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.