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(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++ include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy);
+/** + * drm_mode_init - initialize the mode from another mode + * @dst: mode to overwrite + * @src: mode to copy + * + * Copy an existing mode into another mode, zeroing the + * list head of the destination mode. Typically used + * to guarantee the list head is not left with stack + * garbage in on-stack modes. + */ +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{ + memset(dst, 0, sizeof(*dst)); + drm_mode_copy(dst, src); +} +EXPORT_SYMBOL(drm_mode_init); + /** * drm_mode_duplicate - allocate and duplicate an existing mode * @dev: drm_device to allocate the duplicated mode for diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 29ba4adf0c53..e6e5a588fab1 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags); void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); +void drm_mode_init(struct drm_display_mode *dst, + const struct drm_display_mode *src); struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode); bool drm_mode_match(const struct drm_display_mode *mode1,
On 18.02.2022 11:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++ include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy);
+/**
- drm_mode_init - initialize the mode from another mode
- @dst: mode to overwrite
- @src: mode to copy
- Copy an existing mode into another mode, zeroing the
- list head of the destination mode. Typically used
- to guarantee the list head is not left with stack
- garbage in on-stack modes.
- */
+void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{
- memset(dst, 0, sizeof(*dst));
Why not just clear the list head? Or maybe poison it? It would be more cleaner.
I wonder why there is no such helper in list.h.
Regards Andrzej
- drm_mode_copy(dst, src);
+} +EXPORT_SYMBOL(drm_mode_init);
- /**
- drm_mode_duplicate - allocate and duplicate an existing mode
- @dev: drm_device to allocate the duplicated mode for
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 29ba4adf0c53..e6e5a588fab1 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags); void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); +void drm_mode_init(struct drm_display_mode *dst,
const struct drm_display_mode *src);
On Fri, Feb 18, 2022 at 12:22:44PM +0100, Andrzej Hajda wrote:
On 18.02.2022 11:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++ include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy);
+/**
- drm_mode_init - initialize the mode from another mode
- @dst: mode to overwrite
- @src: mode to copy
- Copy an existing mode into another mode, zeroing the
- list head of the destination mode. Typically used
- to guarantee the list head is not left with stack
- garbage in on-stack modes.
- */
+void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{
- memset(dst, 0, sizeof(*dst));
Why not just clear the list head? Or maybe poison it? It would be more cleaner.
Then we have two places that need to be updated if some other field gets introduced that needs preserving. With a full memset() we only have to care about drm_mode_copy(). Don't see much point in micro-optimizing this thing.
On 18.02.2022 12:56, Ville Syrjälä wrote:
On Fri, Feb 18, 2022 at 12:22:44PM +0100, Andrzej Hajda wrote:
On 18.02.2022 11:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++ include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy);
+/**
- drm_mode_init - initialize the mode from another mode
- @dst: mode to overwrite
- @src: mode to copy
- Copy an existing mode into another mode, zeroing the
- list head of the destination mode. Typically used
- to guarantee the list head is not left with stack
- garbage in on-stack modes.
- */
+void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{
- memset(dst, 0, sizeof(*dst));
Why not just clear the list head? Or maybe poison it? It would be more cleaner.
Then we have two places that need to be updated if some other field gets introduced that needs preserving. With a full memset() we only have to care about drm_mode_copy(). Don't see much point in micro-optimizing this thing.
In such case DOC should be modified to avoid updating it "if some other field..." :)
Anyway: Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
On 2022-02-18 05:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++ include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy);
+/**
- drm_mode_init - initialize the mode from another mode
- @dst: mode to overwrite
- @src: mode to copy
- Copy an existing mode into another mode, zeroing the
- list head of the destination mode. Typically used
- to guarantee the list head is not left with stack
- garbage in on-stack modes.
- */
+void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{
- memset(dst, 0, sizeof(*dst));
- drm_mode_copy(dst, src);
+} +EXPORT_SYMBOL(drm_mode_init);
/**
- drm_mode_duplicate - allocate and duplicate an existing mode
- @dev: drm_device to allocate the duplicated mode for
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 29ba4adf0c53..e6e5a588fab1 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags); void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); +void drm_mode_init(struct drm_display_mode *dst,
const struct drm_display_mode *src);struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode); bool drm_mode_match(const struct drm_display_mode *mode1,
From: Ville Syrjälä ville.syrjala@linux.intel.com
These on stack copies of the modes appear to be pointless. Just look at the originals directly.
Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Nikola Cornij nikola.cornij@amd.com Cc: Aurabindo Pillai aurabindo.pillai@amd.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 21dba337dab0..65aab0d086b6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10139,27 +10139,27 @@ static bool is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state, struct drm_crtc_state *new_crtc_state) { - struct drm_display_mode old_mode, new_mode; + const struct drm_display_mode *old_mode, *new_mode;
if (!old_crtc_state || !new_crtc_state) return false;
- old_mode = old_crtc_state->mode; - new_mode = new_crtc_state->mode; + old_mode = &old_crtc_state->mode; + new_mode = &new_crtc_state->mode;
- if (old_mode.clock == new_mode.clock && - old_mode.hdisplay == new_mode.hdisplay && - old_mode.vdisplay == new_mode.vdisplay && - old_mode.htotal == new_mode.htotal && - old_mode.vtotal != new_mode.vtotal && - old_mode.hsync_start == new_mode.hsync_start && - old_mode.vsync_start != new_mode.vsync_start && - old_mode.hsync_end == new_mode.hsync_end && - old_mode.vsync_end != new_mode.vsync_end && - old_mode.hskew == new_mode.hskew && - old_mode.vscan == new_mode.vscan && - (old_mode.vsync_end - old_mode.vsync_start) == - (new_mode.vsync_end - new_mode.vsync_start)) + if (old_mode->clock == new_mode->clock && + old_mode->hdisplay == new_mode->hdisplay && + old_mode->vdisplay == new_mode->vdisplay && + old_mode->htotal == new_mode->htotal && + old_mode->vtotal != new_mode->vtotal && + old_mode->hsync_start == new_mode->hsync_start && + old_mode->vsync_start != new_mode->vsync_start && + old_mode->hsync_end == new_mode->hsync_end && + old_mode->vsync_end != new_mode->vsync_end && + old_mode->hskew == new_mode->hskew && + old_mode->vscan == new_mode->vscan && + (old_mode->vsync_end - old_mode->vsync_start) == + (new_mode->vsync_end - new_mode->vsync_start)) return true;
return false;
On 2022-02-18 05:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
These on stack copies of the modes appear to be pointless. Just look at the originals directly.
Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Nikola Cornij nikola.cornij@amd.com Cc: Aurabindo Pillai aurabindo.pillai@amd.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 21dba337dab0..65aab0d086b6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10139,27 +10139,27 @@ static bool is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state, struct drm_crtc_state *new_crtc_state) {
- struct drm_display_mode old_mode, new_mode;
const struct drm_display_mode *old_mode, *new_mode;
if (!old_crtc_state || !new_crtc_state) return false;
- old_mode = old_crtc_state->mode;
- new_mode = new_crtc_state->mode;
- old_mode = &old_crtc_state->mode;
- new_mode = &new_crtc_state->mode;
- if (old_mode.clock == new_mode.clock &&
old_mode.hdisplay == new_mode.hdisplay &&old_mode.vdisplay == new_mode.vdisplay &&old_mode.htotal == new_mode.htotal &&old_mode.vtotal != new_mode.vtotal &&old_mode.hsync_start == new_mode.hsync_start &&old_mode.vsync_start != new_mode.vsync_start &&old_mode.hsync_end == new_mode.hsync_end &&old_mode.vsync_end != new_mode.vsync_end &&old_mode.hskew == new_mode.hskew &&old_mode.vscan == new_mode.vscan &&(old_mode.vsync_end - old_mode.vsync_start) ==(new_mode.vsync_end - new_mode.vsync_start))
if (old_mode->clock == new_mode->clock &&
old_mode->hdisplay == new_mode->hdisplay &&old_mode->vdisplay == new_mode->vdisplay &&old_mode->htotal == new_mode->htotal &&old_mode->vtotal != new_mode->vtotal &&old_mode->hsync_start == new_mode->hsync_start &&old_mode->vsync_start != new_mode->vsync_start &&old_mode->hsync_end == new_mode->hsync_end &&old_mode->vsync_end != new_mode->vsync_end &&old_mode->hskew == new_mode->hskew &&old_mode->vscan == new_mode->vscan &&(old_mode->vsync_end - old_mode->vsync_start) ==(new_mode->vsync_end - new_mode->vsync_start))return true;
return false;
Applied. Thanks!
Alex
On Fri, Feb 18, 2022 at 11:28 AM Harry Wentland harry.wentland@amd.com wrote:
On 2022-02-18 05:03, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
These on stack copies of the modes appear to be pointless. Just look at the originals directly.
Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Nikola Cornij nikola.cornij@amd.com Cc: Aurabindo Pillai aurabindo.pillai@amd.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 21dba337dab0..65aab0d086b6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10139,27 +10139,27 @@ static bool is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state, struct drm_crtc_state *new_crtc_state) {
struct drm_display_mode old_mode, new_mode;
const struct drm_display_mode *old_mode, *new_mode; if (!old_crtc_state || !new_crtc_state) return false;
old_mode = old_crtc_state->mode;new_mode = new_crtc_state->mode;
old_mode = &old_crtc_state->mode;new_mode = &new_crtc_state->mode;
if (old_mode.clock == new_mode.clock &&old_mode.hdisplay == new_mode.hdisplay &&old_mode.vdisplay == new_mode.vdisplay &&old_mode.htotal == new_mode.htotal &&old_mode.vtotal != new_mode.vtotal &&old_mode.hsync_start == new_mode.hsync_start &&old_mode.vsync_start != new_mode.vsync_start &&old_mode.hsync_end == new_mode.hsync_end &&old_mode.vsync_end != new_mode.vsync_end &&old_mode.hskew == new_mode.hskew &&old_mode.vscan == new_mode.vscan &&(old_mode.vsync_end - old_mode.vsync_start) ==(new_mode.vsync_end - new_mode.vsync_start))
if (old_mode->clock == new_mode->clock &&old_mode->hdisplay == new_mode->hdisplay &&old_mode->vdisplay == new_mode->vdisplay &&old_mode->htotal == new_mode->htotal &&old_mode->vtotal != new_mode->vtotal &&old_mode->hsync_start == new_mode->hsync_start &&old_mode->vsync_start != new_mode->vsync_start &&old_mode->hsync_end == new_mode->hsync_end &&old_mode->vsync_end != new_mode->vsync_end &&old_mode->hskew == new_mode->hskew &&old_mode->vscan == new_mode->vscan &&(old_mode->vsync_end - old_mode->vsync_start) ==(new_mode->vsync_end - new_mode->vsync_start)) return true; return false;
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: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 65aab0d086b6..bd23c9e481eb 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6361,7 +6361,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, const struct drm_connector_state *con_state = dm_state ? &dm_state->base : NULL; struct dc_stream_state *stream = NULL; - struct drm_display_mode mode = *drm_mode; + struct drm_display_mode mode; struct drm_display_mode saved_mode; struct drm_display_mode *freesync_mode = NULL; bool native_mode_found = false; @@ -6374,6 +6374,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, #endif struct dc_sink *sink = NULL;
+ drm_mode_init(&mode, drm_mode); memset(&saved_mode, 0, sizeof(saved_mode));
if (aconnector == NULL) {
On 2022-02-18 05: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: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 65aab0d086b6..bd23c9e481eb 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6361,7 +6361,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, const struct drm_connector_state *con_state = dm_state ? &dm_state->base : NULL; struct dc_stream_state *stream = NULL;
- struct drm_display_mode mode = *drm_mode;
- struct drm_display_mode mode; struct drm_display_mode saved_mode; struct drm_display_mode *freesync_mode = NULL; bool native_mode_found = false;
@@ -6374,6 +6374,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, #endif struct dc_sink *sink = NULL;
drm_mode_init(&mode, drm_mode); memset(&saved_mode, 0, sizeof(saved_mode));
if (aconnector == NULL) {
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: Alex Deucher alexander.deucher@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 ++-- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index fa20261aa928..673078faa27a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay) - memcpy(native_mode, mode, sizeof(*mode)); + drm_mode_copy(native_mode, mode); } }
@@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) { - *native_mode = *mode; + drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index bd23c9e481eb..514280699ad5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, } }
- aconnector->freesync_vid_base = *m_pref; + drm_mode_copy(&aconnector->freesync_vid_base, m_pref); return m_pref; }
@@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, recalculate_timing = is_freesync_video_mode(&mode, aconnector); if (recalculate_timing) { freesync_mode = get_highest_refresh_rate_mode(aconnector, false); - saved_mode = mode; - mode = *freesync_mode; + drm_mode_copy(&saved_mode, &mode); + drm_mode_copy(&mode, freesync_mode); } else { decide_crtc_timing_for_drm_display_mode( &mode, preferred_mode, scale);
On 2022-02-18 05: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: Alex Deucher alexander.deucher@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 ++-- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index fa20261aa928..673078faa27a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay)
memcpy(native_mode, mode, sizeof(*mode));
drm_mode_copy(native_mode, mode);@@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) {
*native_mode = *mode;
drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break;diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index bd23c9e481eb..514280699ad5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, } }
- aconnector->freesync_vid_base = *m_pref;
- drm_mode_copy(&aconnector->freesync_vid_base, m_pref); return m_pref;
}
@@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, recalculate_timing = is_freesync_video_mode(&mode, aconnector); if (recalculate_timing) { freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
saved_mode = mode;mode = *freesync_mode;
drm_mode_copy(&saved_mode, &mode);drm_mode_copy(&mode, freesync_mode);
Applied. Thanks!
Alex
On Fri, Feb 18, 2022 at 11:32 AM Harry Wentland harry.wentland@amd.com wrote:
On 2022-02-18 05: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: Alex Deucher alexander.deucher@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 ++-- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index fa20261aa928..673078faa27a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay)
memcpy(native_mode, mode, sizeof(*mode));
drm_mode_copy(native_mode, mode); } }@@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) {
*native_mode = *mode;
drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break;diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index bd23c9e481eb..514280699ad5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, } }
aconnector->freesync_vid_base = *m_pref;
drm_mode_copy(&aconnector->freesync_vid_base, m_pref); return m_pref;}
@@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, recalculate_timing = is_freesync_video_mode(&mode, aconnector); if (recalculate_timing) { freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
saved_mode = mode;mode = *freesync_mode;
drm_mode_copy(&saved_mode, &mode);drm_mode_copy(&mode, freesync_mode); } else { decide_crtc_timing_for_drm_display_mode( &mode, preferred_mode, scale);
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: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index a7925a8290b2..0cb1345c6ba4 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -777,7 +777,7 @@ static void radeon_fixup_lvds_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay) - memcpy(native_mode, mode, sizeof(*mode)); + drm_mode_copy(native_mode, mode); } }
@@ -786,7 +786,7 @@ static void radeon_fixup_lvds_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) { - *native_mode = *mode; + drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break;
Applied. Thanks!
Alex
On Fri, Feb 18, 2022 at 5:04 AM Ville Syrjala ville.syrjala@linux.intel.com 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: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index a7925a8290b2..0cb1345c6ba4 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -777,7 +777,7 @@ static void radeon_fixup_lvds_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay)
memcpy(native_mode, mode, sizeof(*mode));
drm_mode_copy(native_mode, mode); } }@@ -786,7 +786,7 @@ static void radeon_fixup_lvds_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) {
*native_mode = *mode;
drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break;-- 2.34.1
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: Andrzej Hajda andrzej.hajda@intel.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/bridge/tc358767.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 963a6794735f..881cf338d5cf 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, /* Save the new desired phy config */ memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
- memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); + drm_mode_copy(&dsi->mode, adjusted_mode); drm_mode_debug_printmodeline(adjusted_mode);
if (pm_runtime_resume_and_get(dev) < 0) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d220..a563460f8d20 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&hdmi->mutex);
/* Store the display mode for plugin/DKMS poweron events */ - memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); + drm_mode_copy(&hdmi->previous_mode, mode);
mutex_unlock(&hdmi->mutex); } diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c23e0abc65e8..7f9574b17caa 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge);
- tc->mode = *mode; + drm_mode_copy(&tc->mode, mode); }
static struct edid *tc_get_edid(struct drm_bridge *bridge,
On 18.02.2022 11: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: Andrzej Hajda andrzej.hajda@intel.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/bridge/tc358767.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 963a6794735f..881cf338d5cf 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, /* Save the new desired phy config */ memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
- memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode));
drm_mode_copy(&dsi->mode, adjusted_mode); drm_mode_debug_printmodeline(adjusted_mode);
if (pm_runtime_resume_and_get(dev) < 0)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d220..a563460f8d20 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&hdmi->mutex);
/* Store the display mode for plugin/DKMS poweron events */
- memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
drm_mode_copy(&hdmi->previous_mode, mode);
mutex_unlock(&hdmi->mutex); }
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c23e0abc65e8..7f9574b17caa 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge);
- tc->mode = *mode;
drm_mode_copy(&tc->mode, mode); }
static struct edid *tc_get_edid(struct drm_bridge *bridge,
Hi Ville,
Thank you for the patch.
On Fri, Feb 18, 2022 at 12:03:47PM +0200, 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: Andrzej Hajda andrzej.hajda@intel.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/bridge/tc358767.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 963a6794735f..881cf338d5cf 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, /* Save the new desired phy config */ memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
- memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode));
drm_mode_copy(&dsi->mode, adjusted_mode); drm_mode_debug_printmodeline(adjusted_mode);
if (pm_runtime_resume_and_get(dev) < 0)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d220..a563460f8d20 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&hdmi->mutex);
/* Store the display mode for plugin/DKMS poweron events */
- memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
drm_mode_copy(&hdmi->previous_mode, mode);
mutex_unlock(&hdmi->mutex);
} diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c23e0abc65e8..7f9574b17caa 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge);
- tc->mode = *mode;
- drm_mode_copy(&tc->mode, mode);
}
static struct edid *tc_get_edid(struct drm_bridge *bridge,
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: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c index 36c7c2686c90..79fc602b35bc 100644 --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c @@ -385,12 +385,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc, if (!gma_power_begin(dev, true)) return 0;
- memcpy(&gma_crtc->saved_mode, - mode, - sizeof(struct drm_display_mode)); - memcpy(&gma_crtc->saved_adjusted_mode, - adjusted_mode, - sizeof(struct drm_display_mode)); + drm_mode_copy(&gma_crtc->saved_mode, mode); + drm_mode_copy(&gma_crtc->saved_adjusted_mode, adjusted_mode);
list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc)
On Fri, Feb 18, 2022 at 11:04 AM Ville Syrjala ville.syrjala@linux.intel.com 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: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Looks good. Thanks! Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com
drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c index 36c7c2686c90..79fc602b35bc 100644 --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c @@ -385,12 +385,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc, if (!gma_power_begin(dev, true)) return 0;
memcpy(&gma_crtc->saved_mode,mode,sizeof(struct drm_display_mode));memcpy(&gma_crtc->saved_adjusted_mode,adjusted_mode,sizeof(struct drm_display_mode));
drm_mode_copy(&gma_crtc->saved_mode, mode);drm_mode_copy(&gma_crtc->saved_adjusted_mode, adjusted_mode); list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc)-- 2.34.1
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.
Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Tian Tao tiantao6@hisilicon.com Cc: John Stultz john.stultz@linaro.org Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Chen Feng puck.chen@hisilicon.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index 1d556482bb46..53bd2dbf38cd 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -657,7 +657,7 @@ static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder, * reset adj_mode to the mode value each time, * so we don't adjust the mode twice */ - drm_mode_copy(&adj_mode, mode); + drm_mode_init(&adj_mode, mode);
crtc_funcs = crtc->helper_private; if (crtc_funcs && crtc_funcs->mode_fixup)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Replace the hand rolled drm_mode_duplicate() with the real thing.
@is_dup@ @@ drm_mode_duplicate(...) { ... }
@depends on !is_dup@ expression dev, oldmode; identifier newmode; @@ - newmode = drm_mode_create(dev); + newmode = drm_mode_duplicate(dev, oldmode); ... - drm_mode_copy(newmode, oldmode);
Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/imx/imx-ldb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index e5078d03020d..c8cf819f39ce 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -150,10 +150,9 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector) if (imx_ldb_ch->mode_valid) { struct drm_display_mode *mode;
- mode = drm_mode_create(connector->dev); + mode = drm_mode_duplicate(connector->dev, &imx_ldb_ch->mode); if (!mode) return -EINVAL; - drm_mode_copy(mode, &imx_ldb_ch->mode); mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); num_modes++;
On Fri, 2022-02-18 at 12:03 +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Replace the hand rolled drm_mode_duplicate() with the real thing.
@is_dup@ @@ drm_mode_duplicate(...) { ... }
@depends on !is_dup@ expression dev, oldmode; identifier newmode; @@
- newmode = drm_mode_create(dev);
- newmode = drm_mode_duplicate(dev, oldmode);
...
- drm_mode_copy(newmode, oldmode);
Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Philipp Zabel p.zabel@pengutronix.de
regards Philipp
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; }
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; }
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;
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
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;
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);
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
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);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);
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.
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: Chun-Kuang Hu chunkuang.hu@kernel.org Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 3196189429bc..30f879e68a2b 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1218,7 +1218,7 @@ static int mtk_hdmi_bridge_mode_valid(struct drm_bridge *bridge, if (next_bridge) { struct drm_display_mode adjusted_mode;
- drm_mode_copy(&adjusted_mode, mode); + drm_mode_init(&adjusted_mode, mode); if (!drm_bridge_chain_mode_fixup(next_bridge, mode, &adjusted_mode)) return MODE_BAD;
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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Sandy Huang hjc@rock-chips.com Cc: "Heiko Stübner" heiko@sntech.de Cc: linux-arm-kernel@lists.infradead.org Cc: linux-rockchip@lists.infradead.org --- 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 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c index 4740cc14beb8..adf1027a3f20 100644 --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c @@ -564,7 +564,7 @@ static void cdn_dp_encoder_mode_set(struct drm_encoder *encoder, video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC); video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
- memcpy(&dp->mode, adjusted, sizeof(*mode)); + drm_mode_copy(&dp->mode, adjusted); }
static bool cdn_dp_check_link_status(struct cdn_dp_device *dp) diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c index 046e8ec2a71c..740196d30fba 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -488,7 +488,7 @@ static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder, inno_hdmi_setup(hdmi, adj_mode);
/* Store the display mode for plugin/DPMS poweron events */ - memcpy(&hdmi->previous_mode, adj_mode, sizeof(hdmi->previous_mode)); + drm_mode_copy(&hdmi->previous_mode, adj_mode); }
static void inno_hdmi_encoder_enable(struct drm_encoder *encoder) diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c index 1c546c3a8998..17e7c40a9e7b 100644 --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c @@ -383,7 +383,7 @@ rk3066_hdmi_encoder_mode_set(struct drm_encoder *encoder, struct rk3066_hdmi *hdmi = to_rk3066_hdmi(encoder);
/* Store the display mode for plugin/DPMS poweron events. */ - memcpy(&hdmi->previous_mode, adj_mode, sizeof(hdmi->previous_mode)); + drm_mode_copy(&hdmi->previous_mode, adj_mode); }
static void rk3066_hdmi_encoder_enable(struct drm_encoder *encoder)
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: Alain Volmat alain.volmat@foss.st.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/sti/sti_dvo.c | 2 +- drivers/gpu/drm/sti/sti_hda.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index b6ee8a82e656..f3a5616b7daf 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -288,7 +288,7 @@ static void sti_dvo_set_mode(struct drm_bridge *bridge,
DRM_DEBUG_DRIVER("\n");
- memcpy(&dvo->mode, mode, sizeof(struct drm_display_mode)); + drm_mode_copy(&dvo->mode, mode);
/* According to the path used (main or aux), the dvo clocks should * have a different parent clock. */ diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c index 03f3377f918c..c9e6db15ab66 100644 --- a/drivers/gpu/drm/sti/sti_hda.c +++ b/drivers/gpu/drm/sti/sti_hda.c @@ -523,7 +523,7 @@ static void sti_hda_set_mode(struct drm_bridge *bridge,
DRM_DEBUG_DRIVER("\n");
- memcpy(&hda->mode, mode, sizeof(struct drm_display_mode)); + drm_mode_copy(&hda->mode, mode);
if (!hda_get_mode_idx(hda->mode, &mode_idx)) { DRM_ERROR("Undefined mode\n"); diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index f3ace11209dd..bb2a2868de2d 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -940,7 +940,7 @@ static void sti_hdmi_set_mode(struct drm_bridge *bridge, DRM_DEBUG_DRIVER("\n");
/* Copy the drm display mode in the connector local structure */ - memcpy(&hdmi->mode, mode, sizeof(struct drm_display_mode)); + drm_mode_copy(&hdmi->mode, mode);
/* Update clock framerate according to the selected mode */ ret = clk_set_rate(hdmi->clk_pix, mode->clock * 1000);
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: Jyri Sarha jyri.sarha@iki.fi Cc: Tomi Valkeinen tomba@kernel.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 29890d704cb4..853c6b443fff 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -433,7 +433,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
set_scanout(crtc, fb);
- crtc->hwmode = crtc->state->adjusted_mode; + drm_mode_copy(&crtc->hwmode, &crtc->state->adjusted_mode);
tilcdc_crtc->hvtotal_us = tilcdc_mode_hvtotal(&crtc->hwmode);
On 18/02/2022 12: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: Jyri Sarha jyri.sarha@iki.fi Cc: Tomi Valkeinen tomba@kernel.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 29890d704cb4..853c6b443fff 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -433,7 +433,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
set_scanout(crtc, fb);
- crtc->hwmode = crtc->state->adjusted_mode;
drm_mode_copy(&crtc->hwmode, &crtc->state->adjusted_mode);
tilcdc_crtc->hvtotal_us = tilcdc_mode_hvtotal(&crtc->hwmode);
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
Tomi
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: Emma Anholt emma@anholt.net Cc: Maxime Ripard mripard@kernel.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/vc4/vc4_hdmi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 5ac3216f2d4a..6c58b0fd13fb 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1234,9 +1234,8 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder, struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
mutex_lock(&vc4_hdmi->mutex); - memcpy(&vc4_hdmi->saved_adjusted_mode, - &crtc_state->adjusted_mode, - sizeof(vc4_hdmi->saved_adjusted_mode)); + drm_mode_copy(&vc4_hdmi->saved_adjusted_mode, + &crtc_state->adjusted_mode); mutex_unlock(&vc4_hdmi->mutex); }
On Fri, 18 Feb 2022 12:03:58 +0200, 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.
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/display/intel_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 740620ef07ad..74c5a99ab276 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6898,8 +6898,9 @@ intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - struct drm_display_mode adjusted_mode = - crtc_state->hw.adjusted_mode; + struct drm_display_mode adjusted_mode; + + drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
if (crtc_state->vrr.enable) { adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
On Fri, 18 Feb 2022, Ville Syrjala ville.syrjala@linux.intel.com 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
I wonder if that cocci could be added to scripts/coccinelle or something to detect anyone adding new ones?
Anyway,
Reviewed-by: Jani Nikula jani.nikula@intel.com
drivers/gpu/drm/i915/display/intel_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 740620ef07ad..74c5a99ab276 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6898,8 +6898,9 @@ intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- struct drm_display_mode adjusted_mode =
crtc_state->hw.adjusted_mode;
struct drm_display_mode adjusted_mode;
drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
if (crtc_state->vrr.enable) { adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
On Wed, Mar 16, 2022 at 10:00:06AM +0200, Jani Nikula wrote:
On Fri, 18 Feb 2022, Ville Syrjala ville.syrjala@linux.intel.com 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
I wonder if that cocci could be added to scripts/coccinelle or something to detect anyone adding new ones?
Maybe.
Julia & co, would you be open to having drm subsystem specific coccinelle scripts? If so where should we put the? scripts/coccinelle/drm perhaps?
On Mon, 21 Mar 2022, Ville Syrjälä wrote:
On Wed, Mar 16, 2022 at 10:00:06AM +0200, Jani Nikula wrote:
On Fri, 18 Feb 2022, Ville Syrjala ville.syrjala@linux.intel.com 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
I wonder if that cocci could be added to scripts/coccinelle or something to detect anyone adding new ones?
Maybe.
Julia & co, would you be open to having drm subsystem specific coccinelle scripts? If so where should we put the? scripts/coccinelle/drm perhaps?
That would be fine. It is possible to make a script only apply to a specific directory, but I think that that is not necessary in this case, since you mention types that are only relevant to drm code.
julia
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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 74c5a99ab276..661e36435793 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5506,8 +5506,10 @@ intel_crtc_copy_uapi_to_hw_state_modeset(struct intel_atomic_state *state,
crtc_state->hw.enable = crtc_state->uapi.enable; crtc_state->hw.active = crtc_state->uapi.active; - crtc_state->hw.mode = crtc_state->uapi.mode; - crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode; + drm_mode_copy(&crtc_state->hw.mode, + &crtc_state->uapi.mode); + drm_mode_copy(&crtc_state->hw.adjusted_mode, + &crtc_state->uapi.adjusted_mode); crtc_state->hw.scaling_filter = crtc_state->uapi.scaling_filter;
intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc); @@ -5584,9 +5586,12 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state, memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); slave_crtc_state->hw.enable = master_crtc_state->hw.enable; slave_crtc_state->hw.active = master_crtc_state->hw.active; - slave_crtc_state->hw.mode = master_crtc_state->hw.mode; - slave_crtc_state->hw.pipe_mode = master_crtc_state->hw.pipe_mode; - slave_crtc_state->hw.adjusted_mode = master_crtc_state->hw.adjusted_mode; + drm_mode_copy(&slave_crtc_state->hw.mode, + &master_crtc_state->hw.mode); + drm_mode_copy(&slave_crtc_state->hw.pipe_mode, + &master_crtc_state->hw.pipe_mode); + drm_mode_copy(&slave_crtc_state->hw.adjusted_mode, + &master_crtc_state->hw.adjusted_mode); slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter;
copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc);
On Fri, 18 Feb 2022, Ville Syrjala ville.syrjala@linux.intel.com 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Jani Nikula jani.nikula@intel.com
drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 74c5a99ab276..661e36435793 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5506,8 +5506,10 @@ intel_crtc_copy_uapi_to_hw_state_modeset(struct intel_atomic_state *state,
crtc_state->hw.enable = crtc_state->uapi.enable; crtc_state->hw.active = crtc_state->uapi.active;
- crtc_state->hw.mode = crtc_state->uapi.mode;
- crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
drm_mode_copy(&crtc_state->hw.mode,
&crtc_state->uapi.mode);drm_mode_copy(&crtc_state->hw.adjusted_mode,
&crtc_state->uapi.adjusted_mode);crtc_state->hw.scaling_filter = crtc_state->uapi.scaling_filter;
intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc);
@@ -5584,9 +5586,12 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state, memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); slave_crtc_state->hw.enable = master_crtc_state->hw.enable; slave_crtc_state->hw.active = master_crtc_state->hw.active;
- slave_crtc_state->hw.mode = master_crtc_state->hw.mode;
- slave_crtc_state->hw.pipe_mode = master_crtc_state->hw.pipe_mode;
- slave_crtc_state->hw.adjusted_mode = master_crtc_state->hw.adjusted_mode;
drm_mode_copy(&slave_crtc_state->hw.mode,
&master_crtc_state->hw.mode);drm_mode_copy(&slave_crtc_state->hw.pipe_mode,
&master_crtc_state->hw.pipe_mode);drm_mode_copy(&slave_crtc_state->hw.adjusted_mode,
&master_crtc_state->hw.adjusted_mode);slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter;
copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Replace the hand rolled drm_mode_duplicate() with the real thing.
@is_dup@ @@ drm_mode_duplicate(...) { ... }
@depends on !is_dup@ expression dev, oldmode; identifier newmode; @@ - newmode = drm_mode_create(dev); + newmode = drm_mode_duplicate(dev, oldmode); ... - drm_mode_copy(newmode, oldmode);
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/panel/panel-truly-nt35597.c | 3 +-- drivers/gpu/drm/panel/panel-visionox-rm69299.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c index b24b92d93ea5..9ca5c7ff41d6 100644 --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c @@ -446,7 +446,7 @@ static int truly_nt35597_get_modes(struct drm_panel *panel, const struct nt35597_config *config;
config = ctx->config; - mode = drm_mode_create(connector->dev); + mode = drm_mode_duplicate(connector->dev, config->dm); if (!mode) { dev_err(ctx->dev, "failed to create a new display mode\n"); return 0; @@ -454,7 +454,6 @@ static int truly_nt35597_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = config->width_mm; connector->display_info.height_mm = config->height_mm; - drm_mode_copy(mode, config->dm); mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode);
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c index eb43503ec97b..db2443ac81d3 100644 --- a/drivers/gpu/drm/panel/panel-visionox-rm69299.c +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c @@ -168,7 +168,8 @@ static int visionox_rm69299_get_modes(struct drm_panel *panel, struct visionox_rm69299 *ctx = panel_to_ctx(panel); struct drm_display_mode *mode;
- mode = drm_mode_create(connector->dev); + mode = drm_mode_duplicate(connector->dev, + &visionox_rm69299_1080x2248_60hz); if (!mode) { dev_err(ctx->panel.dev, "failed to create a new display mode\n"); return 0; @@ -176,7 +177,6 @@ static int visionox_rm69299_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = 74; connector->display_info.height_mm = 131; - drm_mode_copy(mode, &visionox_rm69299_1080x2248_60hz); mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode);
Hi Ville,
On Fri, Feb 18, 2022 at 12:04:01PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Replace the hand rolled drm_mode_duplicate() with the real thing.
@is_dup@ @@ drm_mode_duplicate(...) { ... }
@depends on !is_dup@ expression dev, oldmode; identifier newmode; @@
- newmode = drm_mode_create(dev);
- newmode = drm_mode_duplicate(dev, oldmode); ...
- drm_mode_copy(newmode, oldmode);
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org
Looks good, Acked-by: Sam Ravnborg sam@ravnborg.org
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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc_helper.c | 8 ++++---- drivers/gpu/drm/drm_edid.c | 8 ++++++-- drivers/gpu/drm/drm_modes.c | 4 +++- 3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index bff917531f33..a34aa009725f 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -297,8 +297,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, return false; }
- saved_mode = crtc->mode; - saved_hwmode = crtc->hwmode; + drm_mode_init(&saved_mode, &crtc->mode); + drm_mode_init(&saved_hwmode, &crtc->hwmode); saved_x = crtc->x; saved_y = crtc->y;
@@ -411,8 +411,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, drm_mode_destroy(dev, adjusted_mode); if (!ret) { crtc->enabled = saved_enabled; - crtc->mode = saved_mode; - crtc->hwmode = saved_hwmode; + drm_mode_copy(&crtc->mode, &saved_mode); + drm_mode_copy(&crtc->hwmode, &saved_hwmode); crtc->x = saved_x; crtc->y = saved_y; } diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a7663f9a11d2..1156bfeabaf6 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3476,9 +3476,11 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
for (vic = 1; vic < cea_num_vics(); vic = cea_next_vic(vic)) { - struct drm_display_mode cea_mode = *cea_mode_for_vic(vic); + struct drm_display_mode cea_mode; unsigned int clock1, clock2;
+ drm_mode_init(&cea_mode, cea_mode_for_vic(vic)); + /* Check both 60Hz and 59.94Hz */ clock1 = cea_mode.clock; clock2 = cea_mode_alternate_clock(&cea_mode); @@ -3515,9 +3517,11 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
for (vic = 1; vic < cea_num_vics(); vic = cea_next_vic(vic)) { - struct drm_display_mode cea_mode = *cea_mode_for_vic(vic); + struct drm_display_mode cea_mode; unsigned int clock1, clock2;
+ drm_mode_init(&cea_mode, cea_mode_for_vic(vic)); + /* Check both 60Hz and 59.94Hz */ clock1 = cea_mode.clock; clock2 = cea_mode_alternate_clock(&cea_mode); diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 40d4ce4a1da4..86904d082ff2 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -788,7 +788,9 @@ EXPORT_SYMBOL(drm_mode_vrefresh); void drm_mode_get_hv_timing(const struct drm_display_mode *mode, int *hdisplay, int *vdisplay) { - struct drm_display_mode adjusted = *mode; + struct drm_display_mode adjusted; + + drm_mode_init(&adjusted, mode);
drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE_ONLY); *hdisplay = adjusted.crtc_hdisplay;
On 18.02.2022 11:04, 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc_helper.c | 4 ++-- drivers/gpu/drm/drm_vblank.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index a34aa009725f..b632825654a9 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -305,7 +305,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, /* Update crtc values up front so the driver can rely on them for mode * setting. */ - crtc->mode = *mode; + drm_mode_copy(&crtc->mode, mode); crtc->x = x; crtc->y = y;
@@ -341,7 +341,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, } DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
- crtc->hwmode = *adjusted_mode; + drm_mode_copy(&crtc->hwmode, adjusted_mode);
/* Prepare the encoders and CRTCs before setting the mode. */ drm_for_each_encoder(encoder, dev) { diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index b701cda86d0c..2ff31717a3de 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -644,7 +644,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
vblank->linedur_ns = linedur_ns; vblank->framedur_ns = framedur_ns; - vblank->hwmode = *mode; + drm_mode_copy(&vblank->hwmode, mode);
drm_dbg_core(dev, "crtc %u: hwmode: htotal %d, vtotal %d, vdisplay %d\n",
On 18.02.2022 11:04, 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
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
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()
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
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()
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()
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
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.
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()
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Alex Deucher -
Andrzej Hajda -
Dmitry Baryshkov -
Harry Wentland -
Jani Nikula -
Julia Lawall -
Laurent Pinchart -
Maxime Ripard -
Patrik Jakobsson -
Philipp Zabel -
Sam Ravnborg -
Tomi Valkeinen -
Ville Syrjala -
Ville Syrjälä