diff mbox series

drm/msm/dpu: Delete bonkers code

Message ID 20210430171744.1721408-1-robdclark@gmail.com
State Superseded
Headers show
Series drm/msm/dpu: Delete bonkers code | expand

Commit Message

Rob Clark April 30, 2021, 5:17 p.m. UTC
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(-)

Comments

Stephen Boyd April 30, 2021, 5:44 p.m. UTC | #1
Quoting Rob Clark (2021-04-30 10:17:39)
> 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>

Maybe better to use swboyd@chromium.org for this one.

> Reported-by: John Stultz <john.stultz@linaro.org>
> Fixes: 37418bf14c13 drm: Use state helper instead of the plane state pointer

Should be

Fixes: 37418bf14c13 ("drm: Use state helper instead of the plane state pointer")

to match the preferred format.

> Signed-off-by: Rob Clark <robdclark@chromium.org>

Otherwise looks good, thanks.
Maxime Ripard May 3, 2021, 8:19 a.m. UTC | #2
Hi,

On Fri, Apr 30, 2021 at 10:44:53AM -0700, Stephen Boyd wrote:
> Quoting Rob Clark (2021-04-30 10:17:39)

> > 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>

> 

> Maybe better to use swboyd@chromium.org for this one.

> 

> > Reported-by: John Stultz <john.stultz@linaro.org>

> > Fixes: 37418bf14c13 drm: Use state helper instead of the plane state pointer

> 

> Should be

> 

> Fixes: 37418bf14c13 ("drm: Use state helper instead of the plane state pointer")

> 

> to match the preferred format.

> 

> > Signed-off-by: Rob Clark <robdclark@chromium.org>

> 

> Otherwise looks good, thanks.


Thanks for figuring this out, I've applied it with your chromium address
and the proper fixes format.

Maxime
patchwork-bot+linux-arm-msm@kernel.org May 26, 2021, 7:03 p.m. UTC | #3
Hello:

This patch was applied to qcom/linux.git (refs/heads/for-next):

On Fri, 30 Apr 2021 10:17:39 -0700 you wrote:
> 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:

> 

> [...]


Here is the summary with links:
  - drm/msm/dpu: Delete bonkers code
    https://git.kernel.org/qcom/c/a712b307cfde

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

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