linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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


  reply	other threads:[~2021-06-01 22:44 UTC|newest]

Thread overview: 14+ 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 [this message]
2021-06-01 22:47 ` [PATCH v4 3/6] drm/msm: Improve the a6xx page fault handler Rob Clark
2021-06-01 22:47 ` [PATCH v4 5/6] drm/msm: Add crashdump support for stalled SMMU Rob Clark
2021-06-08 15:12   ` Jordan Crouse
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-08 15:20   ` Jordan Crouse
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:37 ` John Stultz
2021-04-30 17:44 ` Stephen Boyd
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 \
    --subject='Re: [PATCH] drm/msm/dpu: Delete bonkers code' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).