[PATCH 00/22] drm: Review of mode copies

[PATCH 00/22] drm: Review of mode copies

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I might be taking this a bit too far, but the lack of
consistency in our methods to copy drm_display_mode
structs around is bugging me.

The main worry is the embedded list head, which if
clobbered could lead to list corruption. I'd also
prefer to make sure even the valid list heads don't
propagate between copies since that makes no sense.

While going through some of the code I also spotted
some very weird on stack copies being made for no
reason at all. I elimininated a few of them here,
but there could certainly be more lurking in the
shadows.

Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
Cc: Emma Anholt <emma@anholt.net>
Cc: freedreno@lists.freedesktop.org
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jyri Sarha <jyri.sarha@iki.fi>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Nikola Cornij <nikola.cornij@amd.com>
Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Tomi Valkeinen <tomba@kernel.org>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>

Ville Syrjälä (22):
  drm: Add drm_mode_init()
  drm/amdgpu: Remove pointless on stack mode copies
  drm/amdgpu: Use drm_mode_init() for on-stack modes
  drm/amdgpu: Use drm_mode_copy()
  drm/radeon: Use drm_mode_copy()
  drm/bridge: Use drm_mode_copy()
  drm/gma500: Use drm_mode_copy()
  drm/hisilicon: Use drm_mode_init() for on-stack modes
  drm/imx: Use drm_mode_duplicate()
  drm/msm: Nuke weird on stack mode copy
  drm/msm: Use drm_mode_init() for on-stack modes
  drm/msm: Use drm_mode_copy()
  drm/mtk: Use drm_mode_init() for on-stack modes
  drm/rockchip: Use drm_mode_copy()
  drm/sti: Use drm_mode_copy()
  drm/tilcdc: Use drm_mode_copy()
  drm/vc4: Use drm_mode_copy()
  drm/i915: Use drm_mode_init() for on-stack modes
  drm/i915: Use drm_mode_copy()
  drm/panel: Use drm_mode_duplicate()
  drm: Use drm_mode_init() for on-stack modes
  drm: Use drm_mode_copy()

 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  4 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 ++++++++++---------
 drivers/gpu/drm/bridge/nwl-dsi.c              |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |  2 +-
 drivers/gpu/drm/bridge/tc358767.c             |  2 +-
 drivers/gpu/drm/drm_crtc_helper.c             | 12 +++---
 drivers/gpu/drm/drm_edid.c                    |  8 +++-
 drivers/gpu/drm/drm_modes.c                   | 21 +++++++++-
 drivers/gpu/drm/drm_vblank.c                  |  2 +-
 drivers/gpu/drm/gma500/oaktrail_crtc.c        |  8 +---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c  |  2 +-
 drivers/gpu/drm/i915/display/intel_display.c  | 20 +++++----
 drivers/gpu/drm/imx/imx-ldb.c                 |  3 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c           |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  9 ++--
 drivers/gpu/drm/msm/dp/dp_display.c           |  2 +-
 drivers/gpu/drm/msm/dp/dp_drm.c               | 10 ++---
 drivers/gpu/drm/panel/panel-truly-nt35597.c   |  3 +-
 .../gpu/drm/panel/panel-visionox-rm69299.c    |  4 +-
 drivers/gpu/drm/radeon/radeon_connectors.c    |  4 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c        |  2 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c          |  2 +-
 drivers/gpu/drm/rockchip/rk3066_hdmi.c        |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c                 |  2 +-
 drivers/gpu/drm/sti/sti_hda.c                 |  2 +-
 drivers/gpu/drm/sti/sti_hdmi.c                |  2 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c          |  2 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c                |  5 +--
 include/drm/drm_modes.h                       |  2 +
 30 files changed, 105 insertions(+), 79 deletions(-)

-- 
2.34.1

[PATCH 10/22] drm/msm: Nuke weird on stack mode copy

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This on stack middle man mode looks entirely pointless.
Just duplicate the original mode directly.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index d4d360d19eba..09188d02aa1e 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -56,7 +56,7 @@ static int dp_connector_get_modes(struct drm_connector *connector)
 	int rc = 0;
 	struct msm_dp *dp;
 	struct dp_display_mode *dp_mode = NULL;
-	struct drm_display_mode *m, drm_mode;
+	struct drm_display_mode *m;
 
 	if (!connector)
 		return 0;
@@ -82,13 +82,11 @@ static int dp_connector_get_modes(struct drm_connector *connector)
 			return rc;
 		}
 		if (dp_mode->drm_mode.clock) { /* valid DP mode */
-			memset(&drm_mode, 0x0, sizeof(drm_mode));
-			drm_mode_copy(&drm_mode, &dp_mode->drm_mode);
-			m = drm_mode_duplicate(connector->dev, &drm_mode);
+			m = drm_mode_duplicate(connector->dev, &dp_mode->drm_mode);
 			if (!m) {
 				DRM_ERROR("failed to add mode %ux%u\n",
-				       drm_mode.hdisplay,
-				       drm_mode.vdisplay);
+					  dp_mode->drm_mode.hdisplay,
+					  dp_mode->drm_mode.vdisplay);
 				kfree(dp_mode);
 				return 0;
 			}
-- 
2.34.1

[PATCH 11/22] drm/msm: Use drm_mode_init() for on-stack modes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head, or that we aren't copying
over another mode's list head.

Based on the following cocci script, with manual fixups:
@decl@
identifier M;
expression E;
@@
- struct drm_display_mode M = E;
+ struct drm_display_mode M;

@@
identifier decl.M;
expression decl.E;
statement S, S1;
@@
struct drm_display_mode M;
... when != S
+ drm_mode_init(&M, &E);
+
S1

@@
expression decl.E;
@@
- &*E
+ E

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89cd456..e7813c6f7bd9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -248,12 +248,13 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
 	unsigned long lock_flags;
 	struct dpu_hw_intf_cfg intf_cfg = { 0 };
 
+	drm_mode_init(&mode, &phys_enc->cached_mode);
+
 	if (!phys_enc->hw_ctl->ops.setup_intf_cfg) {
 		DPU_ERROR("invalid encoder %d\n", phys_enc != NULL);
 		return;
 	}
 
-	mode = phys_enc->cached_mode;
 	if (!phys_enc->hw_intf->ops.setup_timing_gen) {
 		DPU_ERROR("timing engine setup is not supported\n");
 		return;
@@ -652,7 +653,9 @@ static int dpu_encoder_phys_vid_get_frame_count(
 {
 	struct intf_status s = {0};
 	u32 fetch_start = 0;
-	struct drm_display_mode mode = phys_enc->cached_mode;
+	struct drm_display_mode mode;
+
+	drm_mode_init(&mode, &phys_enc->cached_mode);
 
 	if (!dpu_encoder_phys_vid_is_master(phys_enc))
 		return -EINVAL;
-- 
2.34.1

[PATCH 12/22] drm/msm: Use drm_mode_copy()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.

Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.

Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.

@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}

@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, &E)
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)

@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(&mode, &E)
|
- memcpy(&mode, E, S)
+ drm_mode_copy(&mode, E)
)

@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
 drivers/gpu/drm/msm/dp/dp_display.c                  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 34a6940d12c5..57592052af23 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -157,7 +157,7 @@ static void dpu_encoder_phys_cmd_mode_set(
 		DPU_ERROR("invalid args\n");
 		return;
 	}
-	phys_enc->cached_mode = *adj_mode;
+	drm_mode_copy(&phys_enc->cached_mode, adj_mode);
 	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
 	drm_mode_debug_printmodeline(adj_mode);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index e7813c6f7bd9..d5deca07b65a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -370,7 +370,7 @@ static void dpu_encoder_phys_vid_mode_set(
 	struct dpu_encoder_irq *irq;
 
 	if (adj_mode) {
-		phys_enc->cached_mode = *adj_mode;
+		drm_mode_copy(&phys_enc->cached_mode, adj_mode);
 		drm_mode_debug_printmodeline(adj_mode);
 		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
 	}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21f2091..2ed6028ca8d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -825,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
-	dp->panel->dp_mode.drm_mode = mode->drm_mode;
+	drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode);
 	dp->panel->dp_mode.bpp = mode->bpp;
 	dp->panel->dp_mode.capabilities = mode->capabilities;
 	dp_panel_init_panel_info(dp->panel);
-- 
2.34.1

Re: [PATCH 00/22] drm: Review of mode copies

[Ville Syrjälä]

On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
>   drm: Add drm_mode_init()
>   drm/bridge: Use drm_mode_copy()
>   drm/imx: Use drm_mode_duplicate()
>   drm/panel: Use drm_mode_duplicate()
>   drm/vc4: Use drm_mode_copy()
These have been pushed to drm-misc-next.

>   drm/amdgpu: Remove pointless on stack mode copies
>   drm/amdgpu: Use drm_mode_init() for on-stack modes
>   drm/amdgpu: Use drm_mode_copy()
amdgpu ones are reviewed, but I'll leave them for the
AMD folks to push to whichever tree they prefer.


The rest are still in need of review:
>   drm/radeon: Use drm_mode_copy()
>   drm/gma500: Use drm_mode_copy()
>   drm/hisilicon: Use drm_mode_init() for on-stack modes
>   drm/msm: Nuke weird on stack mode copy
>   drm/msm: Use drm_mode_init() for on-stack modes
>   drm/msm: Use drm_mode_copy()
>   drm/mtk: Use drm_mode_init() for on-stack modes
>   drm/rockchip: Use drm_mode_copy()
>   drm/sti: Use drm_mode_copy()
>   drm/tilcdc: Use drm_mode_copy()
>   drm/i915: Use drm_mode_init() for on-stack modes
>   drm/i915: Use drm_mode_copy()
>   drm: Use drm_mode_init() for on-stack modes
>   drm: Use drm_mode_copy()

-- 
Ville Syrjälä
Intel

Re: [PATCH 00/22] drm: Review of mode copies

[Alex Deucher]

On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
> >   drm: Add drm_mode_init()
> >   drm/bridge: Use drm_mode_copy()
> >   drm/imx: Use drm_mode_duplicate()
> >   drm/panel: Use drm_mode_duplicate()
> >   drm/vc4: Use drm_mode_copy()
> These have been pushed to drm-misc-next.
>
> >   drm/amdgpu: Remove pointless on stack mode copies
> >   drm/amdgpu: Use drm_mode_init() for on-stack modes
> >   drm/amdgpu: Use drm_mode_copy()
> amdgpu ones are reviewed, but I'll leave them for the
> AMD folks to push to whichever tree they prefer.

I pulled patches 2, 4, 5 into my tree.  For 3, I'm happy to have it
land via drm-misc with the rest of the mode_init changes if you'd
prefer.

Alex


Alex

>
>
> The rest are still in need of review:
> >   drm/radeon: Use drm_mode_copy()
> >   drm/gma500: Use drm_mode_copy()
> >   drm/hisilicon: Use drm_mode_init() for on-stack modes
> >   drm/msm: Nuke weird on stack mode copy
> >   drm/msm: Use drm_mode_init() for on-stack modes
> >   drm/msm: Use drm_mode_copy()
> >   drm/mtk: Use drm_mode_init() for on-stack modes
> >   drm/rockchip: Use drm_mode_copy()
> >   drm/sti: Use drm_mode_copy()
> >   drm/tilcdc: Use drm_mode_copy()
> >   drm/i915: Use drm_mode_init() for on-stack modes
> >   drm/i915: Use drm_mode_copy()
> >   drm: Use drm_mode_init() for on-stack modes
> >   drm: Use drm_mode_copy()
>
> --
> Ville Syrjälä
> Intel

Re: [PATCH 00/22] drm: Review of mode copies

[Ville Syrjälä]

On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote:
> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
> > >   drm: Add drm_mode_init()
> > >   drm/bridge: Use drm_mode_copy()
> > >   drm/imx: Use drm_mode_duplicate()
> > >   drm/panel: Use drm_mode_duplicate()
> > >   drm/vc4: Use drm_mode_copy()
> > These have been pushed to drm-misc-next.
> >
> > >   drm/amdgpu: Remove pointless on stack mode copies
> > >   drm/amdgpu: Use drm_mode_init() for on-stack modes
> > >   drm/amdgpu: Use drm_mode_copy()
> > amdgpu ones are reviewed, but I'll leave them for the
> > AMD folks to push to whichever tree they prefer.
> 
> I pulled patches 2, 4, 5 into my tree.

Thanks.

> For 3, I'm happy to have it
> land via drm-misc with the rest of the mode_init changes if you'd
> prefer.

Either way works for me. I don't yet have reviews yet for
the other drivers, so I'll proably hold off for a bit more
at least. And the i915 patch I'll be merging via drm-intel.

> > >   drm/radeon: Use drm_mode_copy()
> > >   drm/gma500: Use drm_mode_copy()
> > >   drm/tilcdc: Use drm_mode_copy()
> > >   drm/i915: Use drm_mode_copy()

Those are now all in.

Which leaves us with these stragglers:
> > >   drm/hisilicon: Use drm_mode_init() for on-stack modes
> > >   drm/msm: Nuke weird on stack mode copy
> > >   drm/msm: Use drm_mode_init() for on-stack modes
> > >   drm/msm: Use drm_mode_copy()
> > >   drm/mtk: Use drm_mode_init() for on-stack modes
> > >   drm/rockchip: Use drm_mode_copy()
> > >   drm/sti: Use drm_mode_copy()
> > >   drm: Use drm_mode_init() for on-stack modes
> > >   drm: Use drm_mode_copy()

-- 
Ville Syrjälä
Intel

Re: [PATCH 11/22] drm/msm: Use drm_mode_init() for on-stack modes

[Dmitry Baryshkov]

On 18/02/2022 13:03, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Initialize on-stack modes with drm_mode_init() to guarantee
> no stack garbage in the list head, or that we aren't copying
> over another mode's list head.
> 
> Based on the following cocci script, with manual fixups:
> @decl@
> identifier M;
> expression E;
> @@
> - struct drm_display_mode M = E;
> + struct drm_display_mode M;
> 
> @@
> identifier decl.M;
> expression decl.E;
> statement S, S1;
> @@
> struct drm_display_mode M;
> ... when != S
> + drm_mode_init(&M, &E);
> +
> S1
> 
> @@
> expression decl.E;
> @@
> - &*E
> + E
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---

-- 
With best wishes
Dmitry

Re: [PATCH 12/22] drm/msm: Use drm_mode_copy()

[Dmitry Baryshkov]

On 18/02/2022 13:03, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> struct drm_display_mode embeds a list head, so overwriting
> the full struct with another one will corrupt the list
> (if the destination mode is on a list). Use drm_mode_copy()
> instead which explicitly preserves the list head of
> the destination mode.
> 
> Even if we know the destination mode is not on any list
> using drm_mode_copy() seems decent as it sets a good
> example. Bad examples of not using it might eventually
> get copied into code where preserving the list head
> actually matters.
> 
> Obviously one case not covered here is when the mode
> itself is embedded in a larger structure and the whole
> structure is copied. But if we are careful when copying
> into modes embedded in structures I think we can be a
> little more reassured that bogus list heads haven't been
> propagated in.
> 
> @is_mode_copy@
> @@
> drm_mode_copy(...)
> {
> ...
> }
> 
> @depends on !is_mode_copy@
> struct drm_display_mode *mode;
> expression E, S;
> @@
> (
> - *mode = E
> + drm_mode_copy(mode, &E)
> |
> - memcpy(mode, E, S)
> + drm_mode_copy(mode, E)
> )
> 
> @depends on !is_mode_copy@
> struct drm_display_mode mode;
> expression E;
> @@
> (
> - mode = E
> + drm_mode_copy(&mode, &E)
> |
> - memcpy(&mode, E, S)
> + drm_mode_copy(&mode, E)
> )
> 
> @@
> struct drm_display_mode *mode;
> @@
> - &*mode
> + mode
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---


-- 
With best wishes
Dmitry

Re: [PATCH 10/22] drm/msm: Nuke weird on stack mode copy

[Dmitry Baryshkov]

On 18/02/2022 13:03, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This on stack middle man mode looks entirely pointless.
> Just duplicate the original mode directly.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I took a glance at the surrounding piece of code.
The dp_connector_get_modes() calls dp_display_get_modes() in attempt to 
fill the dp_mode argument. However the dp_display_get_modes() function 
just calls dp_panel_get_modes(), which does not touch dp_mode argument 
since the commit ab205927592b ("drm/msm/dp: remove mode hard-coding in 
case of DP CTS") dating September 2020. I think we can drop this piece 
of code completely.

> ---
>   drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index d4d360d19eba..09188d02aa1e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -56,7 +56,7 @@ static int dp_connector_get_modes(struct drm_connector *connector)
>   	int rc = 0;
>   	struct msm_dp *dp;
>   	struct dp_display_mode *dp_mode = NULL;
> -	struct drm_display_mode *m, drm_mode;
> +	struct drm_display_mode *m;
>   
>   	if (!connector)
>   		return 0;
> @@ -82,13 +82,11 @@ static int dp_connector_get_modes(struct drm_connector *connector)
>   			return rc;
>   		}
>   		if (dp_mode->drm_mode.clock) { /* valid DP mode */
> -			memset(&drm_mode, 0x0, sizeof(drm_mode));
> -			drm_mode_copy(&drm_mode, &dp_mode->drm_mode);
> -			m = drm_mode_duplicate(connector->dev, &drm_mode);
> +			m = drm_mode_duplicate(connector->dev, &dp_mode->drm_mode);
>   			if (!m) {
>   				DRM_ERROR("failed to add mode %ux%u\n",
> -				       drm_mode.hdisplay,
> -				       drm_mode.vdisplay);
> +					  dp_mode->drm_mode.hdisplay,
> +					  dp_mode->drm_mode.vdisplay);
>   				kfree(dp_mode);
>   				return 0;
>   			}


-- 
With best wishes
Dmitry

Re: [PATCH 00/22] drm: Review of mode copies

[Dmitry Baryshkov]

On 22/03/2022 01:37, Ville Syrjälä wrote:
> On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote:
>> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>>
>>> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
>>>>    drm: Add drm_mode_init()
>>>>    drm/bridge: Use drm_mode_copy()
>>>>    drm/imx: Use drm_mode_duplicate()
>>>>    drm/panel: Use drm_mode_duplicate()
>>>>    drm/vc4: Use drm_mode_copy()
>>> These have been pushed to drm-misc-next.
>>>
>>>>    drm/amdgpu: Remove pointless on stack mode copies
>>>>    drm/amdgpu: Use drm_mode_init() for on-stack modes
>>>>    drm/amdgpu: Use drm_mode_copy()
>>> amdgpu ones are reviewed, but I'll leave them for the
>>> AMD folks to push to whichever tree they prefer.
>>
>> I pulled patches 2, 4, 5 into my tree.
> 
> Thanks.
> 
>> For 3, I'm happy to have it
>> land via drm-misc with the rest of the mode_init changes if you'd
>> prefer.
> 
> Either way works for me. I don't yet have reviews yet for
> the other drivers, so I'll proably hold off for a bit more
> at least. And the i915 patch I'll be merging via drm-intel.
> 
>>>>    drm/radeon: Use drm_mode_copy()
>>>>    drm/gma500: Use drm_mode_copy()
>>>>    drm/tilcdc: Use drm_mode_copy()
>>>>    drm/i915: Use drm_mode_copy()
> 
> Those are now all in.
> 
> Which leaves us with these stragglers:
>>>>    drm/hisilicon: Use drm_mode_init() for on-stack modes

>>>>    drm/msm: Nuke weird on stack mode copy
>>>>    drm/msm: Use drm_mode_init() for on-stack modes
>>>>    drm/msm: Use drm_mode_copy()

These three patches went beneath my radar for some reason.

I have just sent a replacement for the first patch ([1]). Other two 
patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next 
development cycle if that works for you.

[1] https://patchwork.freedesktop.org/series/101682/

>>>>    drm/mtk: Use drm_mode_init() for on-stack modes
>>>>    drm/rockchip: Use drm_mode_copy()
>>>>    drm/sti: Use drm_mode_copy()
>>>>    drm: Use drm_mode_init() for on-stack modes
>>>>    drm: Use drm_mode_copy()
> 


-- 
With best wishes
Dmitry

Re: [PATCH 00/22] drm: Review of mode copies

[Ville Syrjälä]

On Wed, Mar 23, 2022 at 01:39:44PM +0300, Dmitry Baryshkov wrote:
> On 22/03/2022 01:37, Ville Syrjälä wrote:
> > On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote:
> >> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >>>
> >>> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
> >>>>    drm: Add drm_mode_init()
> >>>>    drm/bridge: Use drm_mode_copy()
> >>>>    drm/imx: Use drm_mode_duplicate()
> >>>>    drm/panel: Use drm_mode_duplicate()
> >>>>    drm/vc4: Use drm_mode_copy()
> >>> These have been pushed to drm-misc-next.
> >>>
> >>>>    drm/amdgpu: Remove pointless on stack mode copies
> >>>>    drm/amdgpu: Use drm_mode_init() for on-stack modes
> >>>>    drm/amdgpu: Use drm_mode_copy()
> >>> amdgpu ones are reviewed, but I'll leave them for the
> >>> AMD folks to push to whichever tree they prefer.
> >>
> >> I pulled patches 2, 4, 5 into my tree.
> > 
> > Thanks.
> > 
> >> For 3, I'm happy to have it
> >> land via drm-misc with the rest of the mode_init changes if you'd
> >> prefer.
> > 
> > Either way works for me. I don't yet have reviews yet for
> > the other drivers, so I'll proably hold off for a bit more
> > at least. And the i915 patch I'll be merging via drm-intel.
> > 
> >>>>    drm/radeon: Use drm_mode_copy()
> >>>>    drm/gma500: Use drm_mode_copy()
> >>>>    drm/tilcdc: Use drm_mode_copy()
> >>>>    drm/i915: Use drm_mode_copy()
> > 
> > Those are now all in.
> > 
> > Which leaves us with these stragglers:
> >>>>    drm/hisilicon: Use drm_mode_init() for on-stack modes
> 
> >>>>    drm/msm: Nuke weird on stack mode copy
> >>>>    drm/msm: Use drm_mode_init() for on-stack modes
> >>>>    drm/msm: Use drm_mode_copy()
> 
> These three patches went beneath my radar for some reason.
> 
> I have just sent a replacement for the first patch ([1]). Other two 
> patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next 
> development cycle if that works for you.

That'll do fine for me. Thanks.

> 
> [1] https://patchwork.freedesktop.org/series/101682/
> 
> >>>>    drm/mtk: Use drm_mode_init() for on-stack modes
> >>>>    drm/rockchip: Use drm_mode_copy()
> >>>>    drm/sti: Use drm_mode_copy()
> >>>>    drm: Use drm_mode_init() for on-stack modes
> >>>>    drm: Use drm_mode_copy()
> > 
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Ville Syrjälä
Intel

Re: [PATCH 11/22] drm/msm: Use drm_mode_init() for on-stack modes

[Abhinav Kumar]

On 2/18/2022 2:03 AM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Initialize on-stack modes with drm_mode_init() to guarantee
> no stack garbage in the list head, or that we aren't copying
> over another mode's list head.
> 
> Based on the following cocci script, with manual fixups:
> @decl@
> identifier M;
> expression E;
> @@
> - struct drm_display_mode M = E;
> + struct drm_display_mode M;
> 
> @@
> identifier decl.M;
> expression decl.E;
> statement S, S1;
> @@
> struct drm_display_mode M;
> ... when != S
> + drm_mode_init(&M, &E);
> +
> S1
> 
> @@
> expression decl.E;
> @@
> - &*E
> + E
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index ddd9d89cd456..e7813c6f7bd9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -248,12 +248,13 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>   	unsigned long lock_flags;
>   	struct dpu_hw_intf_cfg intf_cfg = { 0 };
>   
> +	drm_mode_init(&mode, &phys_enc->cached_mode);
> +
>   	if (!phys_enc->hw_ctl->ops.setup_intf_cfg) {
>   		DPU_ERROR("invalid encoder %d\n", phys_enc != NULL);
>   		return;
>   	}
>   
> -	mode = phys_enc->cached_mode;
>   	if (!phys_enc->hw_intf->ops.setup_timing_gen) {
>   		DPU_ERROR("timing engine setup is not supported\n");
>   		return;
> @@ -652,7 +653,9 @@ static int dpu_encoder_phys_vid_get_frame_count(
>   {
>   	struct intf_status s = {0};
>   	u32 fetch_start = 0;
> -	struct drm_display_mode mode = phys_enc->cached_mode;
> +	struct drm_display_mode mode;
> +
> +	drm_mode_init(&mode, &phys_enc->cached_mode);
>   
>   	if (!dpu_encoder_phys_vid_is_master(phys_enc))
>   		return -EINVAL;

Re: [PATCH 12/22] drm/msm: Use drm_mode_copy()

[Abhinav Kumar]

On 2/18/2022 2:03 AM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> struct drm_display_mode embeds a list head, so overwriting
> the full struct with another one will corrupt the list
> (if the destination mode is on a list). Use drm_mode_copy()
> instead which explicitly preserves the list head of
> the destination mode.
> 
> Even if we know the destination mode is not on any list
> using drm_mode_copy() seems decent as it sets a good
> example. Bad examples of not using it might eventually
> get copied into code where preserving the list head
> actually matters.
> 
> Obviously one case not covered here is when the mode
> itself is embedded in a larger structure and the whole
> structure is copied. But if we are careful when copying
> into modes embedded in structures I think we can be a
> little more reassured that bogus list heads haven't been
> propagated in.
> 
> @is_mode_copy@
> @@
> drm_mode_copy(...)
> {
> ...
> }
> 
> @depends on !is_mode_copy@
> struct drm_display_mode *mode;
> expression E, S;
> @@
> (
> - *mode = E
> + drm_mode_copy(mode, &E)
> |
> - memcpy(mode, E, S)
> + drm_mode_copy(mode, E)
> )
> 
> @depends on !is_mode_copy@
> struct drm_display_mode mode;
> expression E;
> @@
> (
> - mode = E
> + drm_mode_copy(&mode, &E)
> |
> - memcpy(&mode, E, S)
> + drm_mode_copy(&mode, E)
> )
> 
> @@
> struct drm_display_mode *mode;
> @@
> - &*mode
> + mode
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>   drivers/gpu/drm/msm/dp/dp_display.c                  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 34a6940d12c5..57592052af23 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -157,7 +157,7 @@ static void dpu_encoder_phys_cmd_mode_set(
>   		DPU_ERROR("invalid args\n");
>   		return;
>   	}
> -	phys_enc->cached_mode = *adj_mode;
> +	drm_mode_copy(&phys_enc->cached_mode, adj_mode);
>   	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
>   	drm_mode_debug_printmodeline(adj_mode);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index e7813c6f7bd9..d5deca07b65a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -370,7 +370,7 @@ static void dpu_encoder_phys_vid_mode_set(
>   	struct dpu_encoder_irq *irq;
>   
>   	if (adj_mode) {
> -		phys_enc->cached_mode = *adj_mode;
> +		drm_mode_copy(&phys_enc->cached_mode, adj_mode);
>   		drm_mode_debug_printmodeline(adj_mode);
>   		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
>   	}
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21f2091..2ed6028ca8d6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -825,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
>   
>   	dp = container_of(dp_display, struct dp_display_private, dp_display);
>   
> -	dp->panel->dp_mode.drm_mode = mode->drm_mode;
> +	drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode);
>   	dp->panel->dp_mode.bpp = mode->bpp;
>   	dp->panel->dp_mode.capabilities = mode->capabilities;
>   	dp_panel_init_panel_info(dp->panel);

Re: [PATCH 00/22] drm: Review of mode copies

[Abhinav Kumar]

On 3/23/2022 3:39 AM, Dmitry Baryshkov wrote:
> On 22/03/2022 01:37, Ville Syrjälä wrote:
>> On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote:
>>> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>>>
>>>> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote:
>>>>>    drm: Add drm_mode_init()
>>>>>    drm/bridge: Use drm_mode_copy()
>>>>>    drm/imx: Use drm_mode_duplicate()
>>>>>    drm/panel: Use drm_mode_duplicate()
>>>>>    drm/vc4: Use drm_mode_copy()
>>>> These have been pushed to drm-misc-next.
>>>>
>>>>>    drm/amdgpu: Remove pointless on stack mode copies
>>>>>    drm/amdgpu: Use drm_mode_init() for on-stack modes
>>>>>    drm/amdgpu: Use drm_mode_copy()
>>>> amdgpu ones are reviewed, but I'll leave them for the
>>>> AMD folks to push to whichever tree they prefer.
>>>
>>> I pulled patches 2, 4, 5 into my tree.
>>
>> Thanks.
>>
>>> For 3, I'm happy to have it
>>> land via drm-misc with the rest of the mode_init changes if you'd
>>> prefer.
>>
>> Either way works for me. I don't yet have reviews yet for
>> the other drivers, so I'll proably hold off for a bit more
>> at least. And the i915 patch I'll be merging via drm-intel.
>>
>>>>>    drm/radeon: Use drm_mode_copy()
>>>>>    drm/gma500: Use drm_mode_copy()
>>>>>    drm/tilcdc: Use drm_mode_copy()
>>>>>    drm/i915: Use drm_mode_copy()
>>
>> Those are now all in.
>>
>> Which leaves us with these stragglers:
>>>>>    drm/hisilicon: Use drm_mode_init() for on-stack modes
> 
>>>>>    drm/msm: Nuke weird on stack mode copy
>>>>>    drm/msm: Use drm_mode_init() for on-stack modes
>>>>>    drm/msm: Use drm_mode_copy()
> 
> These three patches went beneath my radar for some reason.
> 
> I have just sent a replacement for the first patch ([1]). Other two 
> patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next 
> development cycle if that works for you.
> 
> [1] https://patchwork.freedesktop.org/series/101682/

Agree with this approach.

We can replace the first patch with 
https://patchwork.freedesktop.org/series/101682/.

Thanks

Abhinav

> 
>>>>>    drm/mtk: Use drm_mode_init() for on-stack modes
>>>>>    drm/rockchip: Use drm_mode_copy()
>>>>>    drm/sti: Use drm_mode_copy()
>>>>>    drm: Use drm_mode_init() for on-stack modes
>>>>>    drm: Use drm_mode_copy()
>>
> 
>