* [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:15 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 2/8] drm/aspeed: " Ze Huang
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Instantiate plane, CRTC and encoder directly and wire them up with
standard atomic helpers.
This removes arcpgu's dependency on deprecated simple-KMS display pipe
interface.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/tiny/arcpgu.c | 179 +++++++++++++++++++++++++++++++-----------
1 file changed, 133 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index c93d61ac0bb7..e0cce4798f3e 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -8,6 +8,7 @@
#include <linux/clk.h>
#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_debugfs.h>
#include <drm/drm_device.h>
@@ -17,12 +18,13 @@
#include <drm/drm_fbdev_dma.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_dma_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_module.h>
#include <drm/drm_of.h>
+#include <drm/drm_plane.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/of_reserved_mem.h>
@@ -52,13 +54,16 @@ struct arcpgu_drm_private {
struct drm_device drm;
void __iomem *regs;
struct clk *clk;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector sim_conn;
};
-#define dev_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, drm)
-
-#define pipe_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, pipe)
+static inline struct arcpgu_drm_private *dev_to_arcpgu(struct drm_device *drm)
+{
+ return container_of(drm, struct arcpgu_drm_private, drm);
+}
static inline void arc_pgu_write(struct arcpgu_drm_private *arcpgu,
unsigned int reg, u32 value)
@@ -117,7 +122,7 @@ static const u32 arc_pgu_supported_formats[] = {
static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
{
- const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
+ const struct drm_framebuffer *fb = arcpgu->plane.state->fb;
uint32_t pixel_format = fb->format->format;
u32 format = DRM_FORMAT_INVALID;
int i;
@@ -139,10 +144,10 @@ static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, reg_ctrl);
}
-static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+static enum drm_mode_status arcpgu_crtc_helper_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(crtc->dev);
long rate, clk_rate = mode->clock * 1000;
long diff = clk_rate / 200; /* +-0.5% allowed by HDMI spec */
@@ -155,7 +160,7 @@ static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *p
static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
{
- struct drm_display_mode *m = &arcpgu->pipe.crtc.state->adjusted_mode;
+ struct drm_display_mode *m = &arcpgu->crtc.state->adjusted_mode;
u32 val;
arc_pgu_write(arcpgu, ARCPGU_REG_FMT,
@@ -194,11 +199,10 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
clk_set_rate(arcpgu->clk, m->crtc_clock * 1000);
}
-static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void arcpgu_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(crtc->dev);
arc_pgu_mode_set(arcpgu);
@@ -208,9 +212,10 @@ static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
+static void arcpgu_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(crtc->dev);
clk_disable_unprepare(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
@@ -218,35 +223,95 @@ static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
~ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *state)
+static void arcpgu_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
struct arcpgu_drm_private *arcpgu;
struct drm_gem_dma_object *gem;
- if (!pipe->plane.state->fb)
+ if (!pstate->fb)
return;
- arcpgu = pipe_to_arcpgu_priv(pipe);
- gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
+ arcpgu = dev_to_arcpgu(plane->dev);
+ gem = drm_fb_dma_get_gem_obj(pstate->fb, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
}
-static const struct drm_simple_display_pipe_funcs arc_pgu_pipe_funcs = {
- .update = arc_pgu_update,
- .mode_valid = arc_pgu_mode_valid,
- .enable = arc_pgu_enable,
- .disable = arc_pgu_disable,
-};
-
static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
- .fb_create = drm_gem_fb_create,
+ .fb_create = drm_gem_fb_create,
.atomic_check = drm_atomic_helper_check,
.atomic_commit = drm_atomic_helper_commit,
};
DEFINE_DRM_GEM_DMA_FOPS(arcpgu_drm_ops);
+static int arcpgu_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state = NULL;
+
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+}
+
+static const struct drm_plane_helper_funcs arcpgu_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = arcpgu_plane_helper_atomic_check,
+ .atomic_update = arcpgu_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs arcpgu_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static int arcpgu_crtc_helper_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static const struct drm_crtc_helper_funcs arcpgu_crtc_helper_funcs = {
+ .mode_valid = arcpgu_crtc_helper_mode_valid,
+ .atomic_check = arcpgu_crtc_helper_atomic_check,
+ .atomic_enable = arcpgu_crtc_helper_atomic_enable,
+ .atomic_disable = arcpgu_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs arcpgu_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs arcpgu_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
{
struct platform_device *pdev = to_platform_device(arcpgu->drm.dev);
@@ -254,6 +319,9 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
struct device_node *endpoint_node = NULL;
struct drm_connector *connector = NULL;
struct drm_device *drm = &arcpgu->drm;
+ struct drm_plane *plane;
+ struct drm_encoder *encoder;
+ struct drm_crtc *crtc;
int ret;
arcpgu->clk = devm_clk_get(drm->dev, "pxlclk");
@@ -285,6 +353,30 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
if (dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32)))
return -ENODEV;
+ plane = &arcpgu->plane;
+ ret = drm_universal_plane_init(drm, plane, 0,
+ &arcpgu_plane_funcs,
+ arc_pgu_supported_formats,
+ ARRAY_SIZE(arc_pgu_supported_formats),
+ NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+ drm_plane_helper_add(plane, &arcpgu_plane_helper_funcs);
+
+ crtc = &arcpgu->crtc;
+ ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
+ &arcpgu_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+ drm_crtc_helper_add(crtc, &arcpgu_crtc_helper_funcs);
+
+ encoder = &arcpgu->encoder;
+ ret = drm_encoder_init(drm, encoder, &arcpgu_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+ encoder->possible_crtcs = drm_crtc_mask(crtc);
+
/*
* There is only one output port inside each device. It is linked with
* encoder endpoint.
@@ -293,29 +385,24 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
if (endpoint_node) {
encoder_node = of_graph_get_remote_port_parent(endpoint_node);
of_node_put(endpoint_node);
- } else {
- connector = &arcpgu->sim_conn;
- dev_info(drm->dev, "no encoder found. Assumed virtual LCD on simulation platform\n");
- ret = arcpgu_drm_sim_init(drm, connector);
- if (ret < 0)
- return ret;
- }
- ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
- arc_pgu_supported_formats,
- ARRAY_SIZE(arc_pgu_supported_formats),
- NULL, connector);
- if (ret)
- return ret;
-
- if (encoder_node) {
/* Locate drm bridge from the hdmi encoder DT node */
struct drm_bridge *bridge __free(drm_bridge_put) =
of_drm_find_and_get_bridge(encoder_node);
if (!bridge)
return -EPROBE_DEFER;
- ret = drm_simple_display_pipe_attach_bridge(&arcpgu->pipe, bridge);
+ ret = drm_bridge_attach(encoder, bridge, NULL, 0);
+ if (ret)
+ return ret;
+ } else {
+ connector = &arcpgu->sim_conn;
+ dev_info(drm->dev, "no encoder found. Assumed virtual LCD on simulation platform\n");
+ ret = arcpgu_drm_sim_init(drm, connector);
+ if (ret < 0)
+ return ret;
+
+ ret = drm_connector_attach_encoder(connector, encoder);
if (ret)
return ret;
}
@@ -342,7 +429,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
struct drm_device *drm = node->minor->dev;
struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
unsigned long clkrate = clk_get_rate(arcpgu->clk);
- unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
+ unsigned long mode_clock = arcpgu->crtc.state->mode.crtc_clock * 1000;
seq_printf(m, "hw : %lu\n", clkrate);
seq_printf(m, "mode: %lu\n", mode_clock);
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular " Ze Huang
@ 2026-07-16 9:15 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:15 UTC (permalink / raw)
To: Ze Huang; +Cc: dri-devel, Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free in debugfs when reading CRTC state without a lock
- [High] Unchecked `encoder_node` passed to `of_drm_find_and_get_bridge()`
- [Medium] Dynamic pixel format changes will not be programmed into the hardware
--
--- Patch [1]: [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 053328e64a955beacd393bc82052adf8a231ec8a
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers
This replaces the deprecated simple-KMS display pipe interface with regular
atomic helpers. It instantiates the plane, CRTC, and encoder directly.
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..e0cce4798f3ed 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
> [ ... ]
> @@ -218,35 +223,95 @@ static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
> [ ... ]
> +static int arcpgu_plane_helper_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> +{
> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
> + struct drm_crtc *crtc = plane_state->crtc;
> + struct drm_crtc_state *crtc_state = NULL;
> +
> + if (crtc)
> + crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> +
> + return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, false);
> +}
[Severity: Medium]
Will dynamic pixel format changes be programmed into the hardware with this
check?
When a DRM client issues an atomic commit that only changes the framebuffer's
pixel format without changing the display mode, this function delegates to
drm_atomic_helper_check_plane_state() but doesn't check if fb->format
changed.
Because crtc_state->mode_changed isn't set to true on format changes, and
the hardware's pixel format register is only updated in arc_pgu_mode_set()
during atomic_enable(), does this mean the hardware might continue using the
old format and cause display corruption? The previous simple-kms helper
handled this by forcing a modeset when the format changed.
> [ ... ]
> @@ -285,6 +353,30 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
> [ ... ]
> /*
> * There is only one output port inside each device. It is linked with
> * encoder endpoint.
> @@ -293,29 +385,24 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
> if (endpoint_node) {
> encoder_node = of_graph_get_remote_port_parent(endpoint_node);
> of_node_put(endpoint_node);
> - } else {
> - connector = &arcpgu->sim_conn;
> - dev_info(drm->dev, "no encoder found. Assumed virtual LCD on simulation platform\n");
> - ret = arcpgu_drm_sim_init(drm, connector);
> - if (ret < 0)
> - return ret;
> - }
>
> - ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
> - arc_pgu_supported_formats,
> - ARRAY_SIZE(arc_pgu_supported_formats),
> - NULL, connector);
> - if (ret)
> - return ret;
> -
> - if (encoder_node) {
> /* Locate drm bridge from the hdmi encoder DT node */
> struct drm_bridge *bridge __free(drm_bridge_put) =
> of_drm_find_and_get_bridge(encoder_node);
[Severity: High]
Could this lead to attaching an unrelated bridge or indefinitely deferring
the probe?
In arcpgu_load(), if endpoint_node has no remote parent, encoder_node will
be NULL. In the previous code, the bridge lookup was guarded by an explicit
if (encoder_node) check. Now, of_drm_find_and_get_bridge(NULL) is called
unconditionally.
If a NULL node is passed, the bridge lookup iterates through registered
bridges and might return the first bridge with a NULL of_node, incorrectly
attaching it. If none are found, it fails with -EPROBE_DEFER, which might
leave the system without a working display. Should this retain the
encoder_node check?
> if (!bridge)
> return -EPROBE_DEFER;
> [ ... ]
> @@ -342,7 +429,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
> struct drm_device *drm = node->minor->dev;
> struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
> unsigned long clkrate = clk_get_rate(arcpgu->clk);
> - unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
> + unsigned long mode_clock = arcpgu->crtc.state->mode.crtc_clock * 1000;
[Severity: High]
Does this introduce a use-after-free risk when reading the CRTC state?
In arcpgu_show_pxlclock(), the new code accesses arcpgu->crtc.state without
holding the crtc->mutex lock.
If a user reads the pxlclock debugfs file concurrently with a display mode
setting operation completing, drm_atomic_helper_swap_state() might swap out
the old CRTC state and free it asynchronously. Dereferencing this dynamically
allocated state pointer without synchronization could lead to a kernel oops.
The previous simple-KMS code avoided this by reading pipe.crtc.mode, which
is embedded in the CRTC struct itself and updated safely for legacy access.
> seq_printf(m, "hw : %lu\n", clkrate);
> seq_printf(m, "mode: %lu\n", mode_clock);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
2026-07-16 9:01 ` [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:27 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 3/8] drm/mcde: " Ze Huang
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Replace simple display pipe with explicit plane, CRTC and encoder
objects. Move callbacks to plane and CRTC helpers, with vblank handling
through drm_crtc_funcs.
Move page-flip event handling out of the plane update path and into the
CRTC atomic flush path.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/aspeed/aspeed_gfx.h | 11 +-
drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 187 ++++++++++++++++++++++---------
drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 3 +-
3 files changed, 145 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h b/drivers/gpu/drm/aspeed/aspeed_gfx.h
index 4e6a442c3886..61588f4273a0 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
@@ -2,7 +2,6 @@
/* Copyright 2018 IBM Corporation */
#include <drm/drm_device.h>
-#include <drm/drm_simple_kms_helper.h>
struct aspeed_gfx {
struct drm_device drm;
@@ -17,10 +16,16 @@ struct aspeed_gfx {
u32 throd_val;
u32 scan_line_max;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector connector;
};
-#define to_aspeed_gfx(x) container_of(x, struct aspeed_gfx, drm)
+
+static struct aspeed_gfx *to_aspeed_gfx(struct drm_device *drm)
+{
+ return container_of(drm, struct aspeed_gfx, drm);
+}
int aspeed_gfx_create_pipe(struct drm_device *drm);
int aspeed_gfx_create_output(struct drm_device *drm);
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
index 7877a57b8e26..5946af36cc69 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
@@ -5,6 +5,8 @@
#include <linux/reset.h>
#include <linux/regmap.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_device.h>
#include <drm/drm_fb_dma_helper.h>
#include <drm/drm_fourcc.h>
@@ -12,22 +14,15 @@
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_dma_helper.h>
#include <drm/drm_panel.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_vblank.h>
#include "aspeed_gfx.h"
-static struct aspeed_gfx *
-drm_pipe_to_aspeed_gfx(struct drm_simple_display_pipe *pipe)
-{
- return container_of(pipe, struct aspeed_gfx, pipe);
-}
-
static int aspeed_gfx_set_pixel_fmt(struct aspeed_gfx *priv, u32 *bpp)
{
- struct drm_crtc *crtc = &priv->pipe.crtc;
+ struct drm_crtc *crtc = &priv->crtc;
struct drm_device *drm = crtc->dev;
- const u32 format = crtc->primary->state->fb->format->format;
+ const u32 format = priv->plane.state->fb->format->format;
u32 ctrl1;
ctrl1 = readl(priv->base + CRT_CTRL1);
@@ -79,7 +74,7 @@ static void aspeed_gfx_disable_controller(struct aspeed_gfx *priv)
static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
{
- struct drm_display_mode *m = &priv->pipe.crtc.state->adjusted_mode;
+ struct drm_display_mode *m = &priv->crtc.state->adjusted_mode;
u32 ctrl1, d_offset, t_count, bpp;
int err;
@@ -139,48 +134,33 @@ static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
writel(priv->throd_val, priv->base + CRT_THROD);
}
-static void aspeed_gfx_pipe_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void aspeed_gfx_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
- struct drm_crtc *crtc = &pipe->crtc;
+ struct aspeed_gfx *priv = to_aspeed_gfx(crtc->dev);
aspeed_gfx_crtc_mode_set_nofb(priv);
aspeed_gfx_enable_controller(priv);
drm_crtc_vblank_on(crtc);
}
-static void aspeed_gfx_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void aspeed_gfx_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
- struct drm_crtc *crtc = &pipe->crtc;
+ struct aspeed_gfx *priv = to_aspeed_gfx(crtc->dev);
drm_crtc_vblank_off(crtc);
aspeed_gfx_disable_controller(priv);
}
-static void aspeed_gfx_pipe_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *plane_state)
+static void aspeed_gfx_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_framebuffer *fb = pipe->plane.state->fb;
- struct drm_pending_vblank_event *event;
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(commit, plane);
+ struct aspeed_gfx *priv = to_aspeed_gfx(plane->dev);
+ struct drm_framebuffer *fb = new_plane_state->fb;
struct drm_gem_dma_object *gem;
- spin_lock_irq(&crtc->dev->event_lock);
- event = crtc->state->event;
- if (event) {
- crtc->state->event = NULL;
-
- if (drm_crtc_vblank_get(crtc) == 0)
- drm_crtc_arm_vblank_event(crtc, event);
- else
- drm_crtc_send_vblank_event(crtc, event);
- }
- spin_unlock_irq(&crtc->dev->event_lock);
-
if (!fb)
return;
@@ -190,9 +170,9 @@ static void aspeed_gfx_pipe_update(struct drm_simple_display_pipe *pipe,
writel(gem->dma_addr, priv->base + CRT_ADDR);
}
-static int aspeed_gfx_enable_vblank(struct drm_simple_display_pipe *pipe)
+static int aspeed_gfx_crtc_enable_vblank(struct drm_crtc *crtc)
{
- struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
+ struct aspeed_gfx *priv = to_aspeed_gfx(crtc->dev);
u32 reg = readl(priv->base + CRT_CTRL1);
/* Clear pending VBLANK IRQ */
@@ -204,9 +184,9 @@ static int aspeed_gfx_enable_vblank(struct drm_simple_display_pipe *pipe)
return 0;
}
-static void aspeed_gfx_disable_vblank(struct drm_simple_display_pipe *pipe)
+static void aspeed_gfx_crtc_disable_vblank(struct drm_crtc *crtc)
{
- struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
+ struct aspeed_gfx *priv = to_aspeed_gfx(crtc->dev);
u32 reg = readl(priv->base + CRT_CTRL1);
reg &= ~CRT_CTRL_VERTICAL_INTR_EN;
@@ -216,12 +196,91 @@ static void aspeed_gfx_disable_vblank(struct drm_simple_display_pipe *pipe)
writel(reg | CRT_CTRL_VERTICAL_INTR_STS, priv->base + CRT_CTRL1);
}
-static const struct drm_simple_display_pipe_funcs aspeed_gfx_funcs = {
- .enable = aspeed_gfx_pipe_enable,
- .disable = aspeed_gfx_pipe_disable,
- .update = aspeed_gfx_pipe_update,
- .enable_vblank = aspeed_gfx_enable_vblank,
- .disable_vblank = aspeed_gfx_disable_vblank,
+static int aspeed_gfx_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state = NULL;
+
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+}
+
+static const struct drm_plane_helper_funcs aspeed_gfx_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = aspeed_gfx_plane_helper_atomic_check,
+ .atomic_update = aspeed_gfx_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs aspeed_gfx_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static int aspeed_gfx_crtc_helper_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static void aspeed_gfx_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ struct drm_pending_vblank_event *event = new_crtc_state->event;
+
+ if (!event)
+ return;
+
+ new_crtc_state->event = NULL;
+
+ spin_lock_irq(&crtc->dev->event_lock);
+ if (drm_crtc_vblank_get(crtc) == 0)
+ drm_crtc_arm_vblank_event(crtc, event);
+ else
+ drm_crtc_send_vblank_event(crtc, event);
+ spin_unlock_irq(&crtc->dev->event_lock);
+}
+
+static const struct drm_crtc_helper_funcs aspeed_gfx_crtc_helper_funcs = {
+ .atomic_check = aspeed_gfx_crtc_helper_atomic_check,
+ .atomic_enable = aspeed_gfx_crtc_helper_atomic_enable,
+ .atomic_disable = aspeed_gfx_crtc_helper_atomic_disable,
+ .atomic_flush = aspeed_gfx_crtc_helper_atomic_flush,
+};
+
+static const struct drm_crtc_funcs aspeed_gfx_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+ .enable_vblank = aspeed_gfx_crtc_enable_vblank,
+ .disable_vblank = aspeed_gfx_crtc_disable_vblank,
+};
+
+static const struct drm_encoder_funcs aspeed_gfx_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
static const uint32_t aspeed_gfx_formats[] = {
@@ -232,10 +291,36 @@ static const uint32_t aspeed_gfx_formats[] = {
int aspeed_gfx_create_pipe(struct drm_device *drm)
{
struct aspeed_gfx *priv = to_aspeed_gfx(drm);
+ struct drm_plane *plane = &priv->plane;
+ struct drm_crtc *crtc = &priv->crtc;
+ struct drm_encoder *encoder = &priv->encoder;
+ int ret;
+
+ ret = drm_universal_plane_init(drm, plane, 0,
+ &aspeed_gfx_plane_funcs,
+ aspeed_gfx_formats,
+ ARRAY_SIZE(aspeed_gfx_formats),
+ NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+ drm_plane_helper_add(plane, &aspeed_gfx_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
+ &aspeed_gfx_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+ drm_crtc_helper_add(crtc, &aspeed_gfx_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, encoder, &aspeed_gfx_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+ encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+ ret = drm_connector_attach_encoder(&priv->connector, encoder);
+ if (ret)
+ return ret;
- return drm_simple_display_pipe_init(drm, &priv->pipe, &aspeed_gfx_funcs,
- aspeed_gfx_formats,
- ARRAY_SIZE(aspeed_gfx_formats),
- NULL,
- &priv->connector);
+ return 0;
}
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 46094cca2974..b2d805f0c16d 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -21,7 +21,6 @@
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_module.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_vblank.h>
#include <drm/drm_drv.h>
@@ -130,7 +129,7 @@ static irqreturn_t aspeed_gfx_irq_handler(int irq, void *data)
reg = readl(priv->base + CRT_CTRL1);
if (reg & CRT_CTRL_VERTICAL_INTR_STS) {
- drm_crtc_handle_vblank(&priv->pipe.crtc);
+ drm_crtc_handle_vblank(&priv->crtc);
writel(reg, priv->base + priv->int_clr_reg);
return IRQ_HANDLED;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 2/8] drm/aspeed: " Ze Huang
@ 2026-07-16 9:27 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:27 UTC (permalink / raw)
To: Ze Huang; +Cc: Frank.Li, dri-devel, imx
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Missing `inline` keyword on `to_aspeed_gfx` in a header file.
Pre-existing issues:
- [Critical] Direct dereference of `obj->state` inside atomic helpers leads to potential use-after-free and NULL pointer dereferences.
--
--- Patch [2]: [PATCH v2 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers ---
(Note: The format of this report is altered to a free-form summary due to
automated recitation restrictions. Code snippets are not quoted directly.)
commit d1e92d6b727d3655d12a72430c723784264108f5
Author: Ze Huang <ze.huang@oss.qualcomm.com>
Subject: drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers
This commit replaces the simple display pipe with explicit plane, CRTC, and
encoder objects in the aspeed DRM driver. It moves callbacks to their
respective atomic helpers and shifts page-flip event handling to the CRTC
atomic flush path.
Issue 1: Missing inline keyword
File: drivers/gpu/drm/aspeed/aspeed_gfx.h
Function: to_aspeed_gfx
[Severity: Low]
Does this function need the inline keyword? The to_aspeed_gfx function is
defined as static in a header file without being marked inline. Could this
cause duplicate unreferenced copies of the function to be emitted in every
translation unit that includes the header, potentially leading to binary
bloat or compiler warnings when compiled with -Werror?
Issue 2: Direct state dereference in atomic context
File: drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
Functions: aspeed_gfx_set_pixel_fmt and aspeed_gfx_crtc_mode_set_nofb
[Severity: Critical]
This is a pre-existing issue, but is it safe to directly dereference the
object state pointers here? In aspeed_gfx_set_pixel_fmt, the code directly
accesses priv->plane.state->fb->format->format. Similarly, in
aspeed_gfx_crtc_mode_set_nofb, it directly accesses
priv->crtc.state->adjusted_mode.
Since this is accessed during atomic commits, if a second non-blocking
commit disables the plane before the first commit reaches its atomic enable
execution, the plane state could be updated to a new state with a NULL
framebuffer. When the first commit's worker eventually reads the state,
could this lead to a NULL pointer dereference and a kernel crash? Would it
be safer to pass the specific plane and CRTC states down from the atomic
helpers instead of reading them directly from the objects?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
2026-07-16 9:01 ` [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular " Ze Huang
2026-07-16 9:01 ` [PATCH v2 2/8] drm/aspeed: " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:21 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 4/8] drm/pl111: " Ze Huang
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Convert MCDE to explicit plane, CRTC and encoder objects.
Handle pending page-flip events from the CRTC atomic flush path using the
commit-local CRTC state, and avoid manual connector/encoder attachment
before the connector has been created by the bridge path.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/mcde/mcde_display.c | 251 +++++++++++++++++++++++-------------
drivers/gpu/drm/mcde/mcde_drm.h | 8 +-
drivers/gpu/drm/mcde/mcde_drv.c | 8 +-
3 files changed, 173 insertions(+), 94 deletions(-)
diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
index 257a6e84dd58..86edf94c5237 100644
--- a/drivers/gpu/drm/mcde/mcde_display.c
+++ b/drivers/gpu/drm/mcde/mcde_display.c
@@ -10,6 +10,7 @@
#include <linux/regulator/consumer.h>
#include <linux/media-bus-format.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_device.h>
#include <drm/drm_fb_dma_helper.h>
#include <drm/drm_fourcc.h>
@@ -18,7 +19,6 @@
#include <drm/drm_gem_dma_helper.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_print.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_bridge.h>
#include <drm/drm_vblank.h>
#include <video/mipi_display.h>
@@ -132,7 +132,7 @@ void mcde_display_irq(struct mcde *mcde)
writel(mispp, mcde->regs + MCDE_RISPP);
if (vblank)
- drm_crtc_handle_vblank(&mcde->pipe.crtc);
+ drm_crtc_handle_vblank(&mcde->crtc);
if (misovl)
dev_info(mcde->dev, "some stray overlay IRQ %08x\n", misovl);
@@ -157,40 +157,57 @@ void mcde_display_disable_irqs(struct mcde *mcde)
writel(0xFFFFFFFF, mcde->regs + MCDE_RISCHNL);
}
-static int mcde_display_check(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *pstate,
- struct drm_crtc_state *cstate)
+static int mcde_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *old_fb = pipe->plane.state->fb;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_crtc *crtc = pstate->crtc;
+ struct drm_crtc_state *cstate = NULL;
+ const struct drm_display_mode *mode;
+ struct drm_framebuffer *old_fb = old_pstate->fb;
struct drm_framebuffer *fb = pstate->fb;
+ int ret;
- if (fb) {
- u32 offset = drm_fb_dma_get_gem_addr(fb, pstate, 0);
+ if (crtc)
+ cstate = drm_atomic_get_new_crtc_state(commit, crtc);
- /* FB base address must be dword aligned. */
- if (offset & 3) {
- DRM_DEBUG_KMS("FB not 32-bit aligned\n");
- return -EINVAL;
- }
+ ret = drm_atomic_helper_check_plane_state(pstate, cstate,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+ if (ret)
+ return ret;
- /*
- * There's no pitch register, the mode's hdisplay
- * controls this.
- */
- if (fb->pitches[0] != mode->hdisplay * fb->format->cpp[0]) {
- DRM_DEBUG_KMS("can't handle pitches\n");
- return -EINVAL;
- }
+ if (!pstate->visible || !fb)
+ return 0;
- /*
- * We can't change the FB format in a flicker-free
- * manner (and only update it during CRTC enable).
- */
- if (old_fb && old_fb->format != fb->format)
- cstate->mode_changed = true;
+ mode = &cstate->mode;
+
+ u32 offset = drm_fb_dma_get_gem_addr(fb, pstate, 0);
+
+ /* FB base address must be dword aligned. */
+ if (offset & 3) {
+ DRM_DEBUG_KMS("FB not 32-bit aligned\n");
+ return -EINVAL;
+ }
+
+ /*
+ * There's no pitch register, the mode's hdisplay
+ * controls this.
+ */
+ if (fb->pitches[0] != mode->hdisplay * fb->format->cpp[0]) {
+ DRM_DEBUG_KMS("can't handle pitches\n");
+ return -EINVAL;
}
+ /*
+ * We can't change the FB format in a flicker-free
+ * manner (and only update it during CRTC enable).
+ */
+ if (old_fb && old_fb->format != fb->format)
+ cstate->mode_changed = true;
+
return 0;
}
@@ -1149,16 +1166,15 @@ static void mcde_setup_dsi(struct mcde *mcde, const struct drm_display_mode *mod
*dsi_formatter_frame = formatter_frame;
}
-static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *cstate,
- struct drm_plane_state *plane_state)
+static void mcde_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_plane *plane = &pipe->plane;
struct drm_device *drm = crtc->dev;
struct mcde *mcde = to_mcde(drm);
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, &mcde->plane);
const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *fb = plane->state->fb;
+ struct drm_framebuffer *fb = plane_state->fb;
u32 format = fb->format->format;
int dsi_pkt_size;
int fifo_wtrmrk;
@@ -1298,9 +1314,10 @@ static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
dev_info(drm->dev, "MCDE display is enabled\n");
}
-static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
+static void mcde_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
struct drm_device *drm = crtc->dev;
struct mcde *mcde = to_mcde(drm);
struct drm_pending_vblank_event *event;
@@ -1318,9 +1335,9 @@ static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
mcde_dsi_disable(mcde->bridge);
}
- event = crtc->state->event;
+ event = cstate->event;
if (event) {
- crtc->state->event = NULL;
+ cstate->event = NULL;
spin_lock_irq(&crtc->dev->event_lock);
drm_crtc_send_vblank_event(crtc, event);
@@ -1381,44 +1398,13 @@ static void mcde_set_extsrc(struct mcde *mcde, u32 buffer_address)
writel(buffer_address + mcde->stride, mcde->regs + MCDE_EXTSRCXA1);
}
-static void mcde_display_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_pstate)
+static void mcde_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_device *drm = crtc->dev;
- struct mcde *mcde = to_mcde(drm);
- struct drm_pending_vblank_event *event = crtc->state->event;
- struct drm_plane *plane = &pipe->plane;
- struct drm_plane_state *pstate = plane->state;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct mcde *mcde = to_mcde(plane->dev);
struct drm_framebuffer *fb = pstate->fb;
- /*
- * Handle any pending event first, we need to arm the vblank
- * interrupt before sending any update to the display so we don't
- * miss the interrupt.
- */
- if (event) {
- crtc->state->event = NULL;
-
- spin_lock_irq(&crtc->dev->event_lock);
- /*
- * Hardware must be on before we can arm any vblank event,
- * this is not a scanout controller where there is always
- * some periodic update going on, it is completely frozen
- * until we get an update. If MCDE output isn't yet enabled,
- * we just send a vblank dummy event back.
- */
- if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0) {
- dev_dbg(mcde->dev, "arm vblank event\n");
- drm_crtc_arm_vblank_event(crtc, event);
- } else {
- dev_dbg(mcde->dev, "insert fake vblank event\n");
- drm_crtc_send_vblank_event(crtc, event);
- }
-
- spin_unlock_irq(&crtc->dev->event_lock);
- }
-
/*
* We do not start sending framebuffer updates before the
* display is enabled. Update events will however be dispatched
@@ -1443,9 +1429,40 @@ static void mcde_display_update(struct drm_simple_display_pipe *pipe,
}
}
-static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
+static void mcde_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ struct drm_pending_vblank_event *event = crtc_state->event;
+
+ /*
+ * Handle any pending event first, we need to arm the vblank
+ * interrupt before sending any update to the display so we don't
+ * miss the interrupt.
+ */
+ if (!event)
+ return;
+
+ crtc_state->event = NULL;
+
+ spin_lock_irq(&crtc->dev->event_lock);
+ /*
+ * Hardware must be on before we can arm any vblank event,
+ * this is not a scanout controller where there is always
+ * some periodic update going on, it is completely frozen
+ * until we get an update. If MCDE output isn't yet enabled,
+ * we just send a vblank dummy event back.
+ */
+ if (crtc_state->active && drm_crtc_vblank_get(crtc) == 0)
+ drm_crtc_arm_vblank_event(crtc, event);
+ else
+ drm_crtc_send_vblank_event(crtc, event);
+
+ spin_unlock_irq(&crtc->dev->event_lock);
+}
+
+static int mcde_crtc_enable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct mcde *mcde = to_mcde(drm);
u32 val;
@@ -1462,9 +1479,8 @@ static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
return 0;
}
-static void mcde_display_disable_vblank(struct drm_simple_display_pipe *pipe)
+static void mcde_crtc_disable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct mcde *mcde = to_mcde(drm);
@@ -1474,13 +1490,55 @@ static void mcde_display_disable_vblank(struct drm_simple_display_pipe *pipe)
writel(0xFFFFFFFF, mcde->regs + MCDE_RISPP);
}
-static struct drm_simple_display_pipe_funcs mcde_display_funcs = {
- .check = mcde_display_check,
- .enable = mcde_display_enable,
- .disable = mcde_display_disable,
- .update = mcde_display_update,
- .enable_vblank = mcde_display_enable_vblank,
- .disable_vblank = mcde_display_disable_vblank,
+static int mcde_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static const struct drm_crtc_funcs mcde_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+ .enable_vblank = mcde_crtc_enable_vblank,
+ .disable_vblank = mcde_crtc_disable_vblank,
+};
+
+static const struct drm_crtc_helper_funcs mcde_crtc_helper_funcs = {
+ .atomic_check = mcde_crtc_helper_atomic_check,
+ .atomic_enable = mcde_crtc_helper_atomic_enable,
+ .atomic_disable = mcde_crtc_helper_atomic_disable,
+ .atomic_flush = mcde_crtc_helper_atomic_flush,
+};
+
+static const struct drm_plane_funcs mcde_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .reset = drm_atomic_helper_plane_reset,
+ .destroy = drm_plane_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs mcde_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = mcde_plane_helper_atomic_check,
+ .atomic_update = mcde_plane_helper_atomic_update,
+};
+
+static const struct drm_encoder_funcs mcde_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
int mcde_display_init(struct drm_device *drm)
@@ -1510,14 +1568,29 @@ int mcde_display_init(struct drm_device *drm)
if (ret)
return ret;
- ret = drm_simple_display_pipe_init(drm, &mcde->pipe,
- &mcde_display_funcs,
- formats, ARRAY_SIZE(formats),
- NULL,
- mcde->connector);
+ ret = drm_universal_plane_init(drm, &mcde->plane, 0,
+ &mcde_plane_funcs,
+ formats, ARRAY_SIZE(formats),
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+
+ drm_plane_helper_add(&mcde->plane, &mcde_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &mcde->crtc, &mcde->plane,
+ NULL, &mcde_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&mcde->crtc, &mcde_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &mcde->encoder, &mcde_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
if (ret)
return ret;
+ mcde->encoder.possible_crtcs = drm_crtc_mask(&mcde->crtc);
+
return 0;
}
EXPORT_SYMBOL_GPL(mcde_display_init);
diff --git a/drivers/gpu/drm/mcde/mcde_drm.h b/drivers/gpu/drm/mcde/mcde_drm.h
index ecb70b4b737c..10dd79893811 100644
--- a/drivers/gpu/drm/mcde/mcde_drm.h
+++ b/drivers/gpu/drm/mcde/mcde_drm.h
@@ -4,7 +4,7 @@
* Parts of this file were based on the MCDE driver by Marcus Lorentzon
* (C) ST-Ericsson SA 2013
*/
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_encoder.h>
#ifndef _MCDE_DRM_H_
#define _MCDE_DRM_H_
@@ -72,7 +72,9 @@ struct mcde {
struct drm_panel *panel;
struct drm_bridge *bridge;
struct drm_connector *connector;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct mipi_dsi_device *mdsi;
bool dpi_output;
s16 stride;
@@ -95,7 +97,7 @@ struct mcde {
struct regulator *vana;
};
-#define to_mcde(dev) container_of(dev, struct mcde, drm)
+struct mcde *to_mcde(struct drm_device *dev);
static inline bool mcde_flow_is_video(struct mcde *mcde)
{
diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 5f2c462bad7e..01787b923520 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -109,6 +109,11 @@ static const struct drm_mode_config_helper_funcs mcde_mode_config_helpers = {
.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
};
+struct mcde *to_mcde(struct drm_device *dev)
+{
+ return container_of(dev, struct mcde, drm);
+}
+
static irqreturn_t mcde_irq(int irq, void *data)
{
struct mcde *mcde = data;
@@ -186,8 +191,7 @@ static int mcde_modeset_init(struct drm_device *drm)
}
/* Attach the bridge. */
- ret = drm_simple_display_pipe_attach_bridge(&mcde->pipe,
- mcde->bridge);
+ ret = drm_bridge_attach(&mcde->encoder, mcde->bridge, NULL, 0);
if (ret) {
dev_err(drm->dev, "failed to attach display output bridge\n");
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 3/8] drm/mcde: " Ze Huang
@ 2026-07-16 9:21 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:21 UTC (permalink / raw)
To: Ze Huang; +Cc: imx, dri-devel, Frank.Li
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in atomic commit causing display hang in oneshot mode due to premature hardware start.
--
--- Patch [3]: [PATCH v2 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 24a767bb6a2c4adfefe9e0a5de7e9c8bbe7b2a64
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers
Convert MCDE to explicit plane, CRTC and encoder objects. Handle pending
page-flip events from the CRTC atomic flush path using the commit-local
CRTC state.
> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 257a6e84dd583..86edf94c52372 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
[ ... ]
> @@ -1381,44 +1398,13 @@ static void mcde_set_extsrc(struct mcde *mcde, u32 buffer_address)
> -static void mcde_display_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_pstate)
> +static void mcde_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_device *drm = crtc->dev;
> - struct mcde *mcde = to_mcde(drm);
> - struct drm_pending_vblank_event *event = crtc->state->event;
> - struct drm_plane *plane = &pipe->plane;
> - struct drm_plane_state *pstate = plane->state;
> + struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
> + struct mcde *mcde = to_mcde(plane->dev);
> struct drm_framebuffer *fb = pstate->fb;
>
> - /*
> - * Handle any pending event first, we need to arm the vblank
> - * interrupt before sending any update to the display so we don't
> - * miss the interrupt.
> - */
> - if (event) {
[ ... ]
> - spin_unlock_irq(&crtc->dev->event_lock);
> - }
> -
> /*
> * We do not start sending framebuffer updates before the
> * display is enabled. Update events will however be dispatched
[ ... ]
> -static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
> +static void mcde_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> +{
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> + struct drm_pending_vblank_event *event = crtc_state->event;
> +
> + /*
> + * Handle any pending event first, we need to arm the vblank
> + * interrupt before sending any update to the display so we don't
> + * miss the interrupt.
> + */
> + if (!event)
> + return;
[Severity: High]
Does this split introduce a race condition that could hang the pipeline in
oneshot mode?
Because the DRM core executes the plane's atomic_update callback before the
CRTC's atomic_flush callback, the hardware flow is now started before the
vblank event is armed.
If mcde_start_flow() is triggered during mcde_plane_helper_atomic_update()
(which happens just below the quoted block), it could complete the hardware
transfer and fire its interrupt before the vblank event gets armed later in
mcde_crtc_helper_atomic_flush().
The relocated comment in atomic_flush still notes this exact requirement:
"we need to arm the vblank interrupt before sending any update to the
display so we don't miss the interrupt."
If the interrupt fires early in oneshot mode, it will be missed, and the
atomic commit could wait indefinitely for an event that will never complete.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
` (2 preceding siblings ...)
2026-07-16 9:01 ` [PATCH v2 3/8] drm/mcde: " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:26 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 5/8] drm/gm12u320: " Ze Huang
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Replace PL111 simple display pipe with explicit plane, CRTC and encoder
objects.
Keep the existing PL111 timing, format and pitch validation, but move it
into the explicit atomic check paths. Move event handling to the CRTC
commit path.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/pl111/pl111_display.c | 202 ++++++++++++++++++++++++----------
drivers/gpu/drm/pl111/pl111_drm.h | 5 +-
drivers/gpu/drm/pl111/pl111_drv.c | 3 +-
3 files changed, 150 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 5d10bc5fdf1f..beb6423d4d43 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -15,6 +15,7 @@
#include <linux/media-bus-format.h>
#include <linux/of_graph.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_fb_dma_helper.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_framebuffer.h>
@@ -37,7 +38,7 @@ irqreturn_t pl111_irq(int irq, void *data)
return IRQ_NONE;
if (irq_stat & CLCD_IRQ_NEXTBASE_UPDATE) {
- drm_crtc_handle_vblank(&priv->pipe.crtc);
+ drm_crtc_handle_vblank(&priv->crtc);
status = IRQ_HANDLED;
}
@@ -49,10 +50,10 @@ irqreturn_t pl111_irq(int irq, void *data)
}
static enum drm_mode_status
-pl111_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+pl111_crtc_helper_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct drm_device *drm = pipe->crtc.dev;
+ struct drm_device *drm = crtc->dev;
struct pl111_drm_dev_private *priv = drm->dev_private;
u32 cpp = DIV_ROUND_UP(priv->variant->fb_depth, 8);
u64 bw;
@@ -83,13 +84,32 @@ pl111_mode_valid(struct drm_simple_display_pipe *pipe,
return MODE_OK;
}
-static int pl111_display_check(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *pstate,
- struct drm_crtc_state *cstate)
+static int pl111_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *old_fb = pipe->plane.state->fb;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_crtc *crtc = pstate->crtc;
+ struct drm_crtc_state *cstate = NULL;
+ const struct drm_display_mode *mode;
+ struct drm_framebuffer *old_fb = old_pstate->fb;
struct drm_framebuffer *fb = pstate->fb;
+ int ret;
+
+ if (crtc)
+ cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ ret = drm_atomic_helper_check_plane_state(pstate, cstate,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+ if (ret)
+ return ret;
+
+ if (!pstate->visible)
+ return 0;
+
+ mode = &cstate->mode;
if (mode->hdisplay % 16)
return -EINVAL;
@@ -117,16 +137,14 @@ static int pl111_display_check(struct drm_simple_display_pipe *pipe,
return 0;
}
-static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *cstate,
- struct drm_plane_state *plane_state)
+static void pl111_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_plane *plane = &pipe->plane;
struct drm_device *drm = crtc->dev;
struct pl111_drm_dev_private *priv = drm->dev_private;
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *fb = plane->state->fb;
+ struct drm_framebuffer *fb = priv->plane.state->fb;
struct drm_connector *connector = priv->connector;
struct drm_bridge *bridge = priv->bridge;
bool grayscale = false;
@@ -355,9 +373,9 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
drm_crtc_vblank_on(crtc);
}
-static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
+static void pl111_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct pl111_drm_dev_private *priv = drm->dev_private;
u32 cntl;
@@ -387,38 +405,47 @@ static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
clk_disable_unprepare(priv->clk);
}
-static void pl111_display_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_pstate)
+static void pl111_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_device *drm = crtc->dev;
- struct pl111_drm_dev_private *priv = drm->dev_private;
- struct drm_pending_vblank_event *event = crtc->state->event;
- struct drm_plane *plane = &pipe->plane;
- struct drm_plane_state *pstate = plane->state;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_crtc *crtc = pstate->crtc;
+ struct drm_device *drm;
+ struct pl111_drm_dev_private *priv;
struct drm_framebuffer *fb = pstate->fb;
- if (fb) {
- u32 addr = drm_fb_dma_get_gem_addr(fb, pstate, 0);
+ if (!fb)
+ return;
- writel(addr, priv->regs + CLCD_UBAS);
- }
+ drm = crtc->dev;
+ priv = drm->dev_private;
- if (event) {
- crtc->state->event = NULL;
+ u32 addr = drm_fb_dma_get_gem_addr(fb, pstate, 0);
- spin_lock_irq(&crtc->dev->event_lock);
- if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
- drm_crtc_arm_vblank_event(crtc, event);
- else
- drm_crtc_send_vblank_event(crtc, event);
- spin_unlock_irq(&crtc->dev->event_lock);
- }
+ writel(addr, priv->regs + CLCD_UBAS);
+}
+
+static void pl111_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+ struct drm_pending_vblank_event *event = cstate->event;
+
+ if (!event)
+ return;
+
+ cstate->event = NULL;
+
+ spin_lock_irq(&crtc->dev->event_lock);
+ if (cstate->active && drm_crtc_vblank_get(crtc) == 0)
+ drm_crtc_arm_vblank_event(crtc, event);
+ else
+ drm_crtc_send_vblank_event(crtc, event);
+ spin_unlock_irq(&crtc->dev->event_lock);
}
-static int pl111_display_enable_vblank(struct drm_simple_display_pipe *pipe)
+static int pl111_display_enable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct pl111_drm_dev_private *priv = drm->dev_private;
@@ -427,21 +454,62 @@ static int pl111_display_enable_vblank(struct drm_simple_display_pipe *pipe)
return 0;
}
-static void pl111_display_disable_vblank(struct drm_simple_display_pipe *pipe)
+static void pl111_display_disable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct pl111_drm_dev_private *priv = drm->dev_private;
writel(0, priv->regs + priv->ienb);
}
-static struct drm_simple_display_pipe_funcs pl111_display_funcs = {
- .mode_valid = pl111_mode_valid,
- .check = pl111_display_check,
- .enable = pl111_display_enable,
- .disable = pl111_display_disable,
- .update = pl111_display_update,
+static int pl111_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static struct drm_crtc_funcs pl111_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_crtc_helper_funcs pl111_crtc_helper_funcs = {
+ .mode_valid = pl111_crtc_helper_mode_valid,
+ .atomic_check = pl111_crtc_helper_atomic_check,
+ .atomic_enable = pl111_crtc_helper_atomic_enable,
+ .atomic_disable = pl111_crtc_helper_atomic_disable,
+ .atomic_flush = pl111_crtc_helper_atomic_flush,
+};
+
+static const struct drm_plane_funcs pl111_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .reset = drm_atomic_helper_plane_reset,
+ .destroy = drm_plane_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs pl111_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = pl111_plane_helper_atomic_check,
+ .atomic_update = pl111_plane_helper_atomic_update,
+};
+
+static const struct drm_encoder_funcs pl111_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
static int pl111_clk_div_choose_div(struct clk_hw *hw, unsigned long rate,
@@ -583,18 +651,40 @@ int pl111_display_init(struct drm_device *drm)
return ret;
if (!priv->variant->broken_vblank) {
- pl111_display_funcs.enable_vblank = pl111_display_enable_vblank;
- pl111_display_funcs.disable_vblank = pl111_display_disable_vblank;
+ pl111_crtc_funcs.enable_vblank = pl111_display_enable_vblank;
+ pl111_crtc_funcs.disable_vblank = pl111_display_disable_vblank;
}
- ret = drm_simple_display_pipe_init(drm, &priv->pipe,
- &pl111_display_funcs,
- priv->variant->formats,
- priv->variant->nformats,
- NULL,
- priv->connector);
+ ret = drm_universal_plane_init(drm, &priv->plane, 0,
+ &pl111_plane_funcs,
+ priv->variant->formats,
+ priv->variant->nformats,
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+
+ drm_plane_helper_add(&priv->plane, &pl111_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->plane,
+ NULL, &pl111_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&priv->crtc, &pl111_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &priv->encoder, &pl111_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
if (ret)
return ret;
+ priv->encoder.possible_crtcs = drm_crtc_mask(&priv->crtc);
+
+ if (priv->connector) {
+ ret = drm_connector_attach_encoder(priv->connector,
+ &priv->encoder);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index d1fe756444ee..ec92a5a180a8 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -21,7 +21,6 @@
#include <drm/drm_encoder.h>
#include <drm/drm_gem.h>
#include <drm/drm_panel.h>
-#include <drm/drm_simple_kms_helper.h>
/*
* CLCD Controller Internal Register addresses
@@ -135,7 +134,9 @@ struct pl111_drm_dev_private {
struct drm_connector *connector;
struct drm_panel *panel;
struct drm_bridge *bridge;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
void *regs;
u32 memory_bw;
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index ac7b1d12a0f5..f649c266c33a 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -168,8 +168,7 @@ static int pl111_modeset_init(struct drm_device *dev)
goto out_bridge;
}
- ret = drm_simple_display_pipe_attach_bridge(&priv->pipe,
- bridge);
+ ret = drm_bridge_attach(&priv->encoder, bridge, NULL, 0);
if (ret)
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 4/8] drm/pl111: " Ze Huang
@ 2026-07-16 9:26 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:26 UTC (permalink / raw)
To: Ze Huang; +Cc: dri-devel, Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Pending vblank events are leaked during CRTC disable. The DRM subsystem requires that pending vblank events requested by userspace (e.g., during a pageflip) are consumed and sent when the CRTC is disabled. In this patch, event consumption is implemented in `pl111_crtc_helper_atomic_flush()`. However, the DRM core (`drm_atomic_helper_commit_planes`) only calls `atomic_flush` for active CRTCs. When a CRTC is being disabled, `atomic_flush` is skipped, and the event must be consumed in the `atomic_disable` hook. `pl111_crtc_helper_atomic_disable()` fails to check for or consume `cstate->event`. As a result, the event is leaked.
- [High] Unsafe access to global plane state in `pl111_crtc_helper_atomic_enable()`. The function retrieves the framebuffer using `priv->plane.state->fb` directly instead of using the committed atomic state via `drm_atomic_get_new_plane_state()`. In the DRM core, non-blocking atomic commits synchronously swap the global state pointers before the asynchronous commit workers run. This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability.
- [High] Sleeping functions are called from atomic context. The DRM subsystem guide explicitly defines CRTC `atomic_enable` and `atomic_disable` callbacks as executing in an atomic context during non-blocking commits, where sleeping is strictly forbidden. However, `pl111_crtc_helper_atomic_enable()` and `pl111_crtc_helper_atomic_disable()` call blocking functions: `clk_prepare_enable()`, `clk_disable_unprepare()`, and `msleep(20)`.
--
--- Patch [4]: [PATCH v2 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 07a755e3243c0f70ec87dc33910c7d68aa70590c
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers
Replace PL111 simple display pipe with explicit plane, CRTC and encoder
objects. Keep the existing PL111 timing, format and pitch validation, but
move it into the explicit atomic check paths. Move event handling to the CRTC
commit path.
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 5d10bc5fdf1f6..beb6423d4d439 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
[ ... ]
> @@ -117,16 +137,14 @@ static int pl111_display_check(struct drm_simple_display_pipe *pipe,
> -static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *cstate,
> - struct drm_plane_state *plane_state)
> +static void pl111_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_plane *plane = &pipe->plane;
> struct drm_device *drm = crtc->dev;
> struct pl111_drm_dev_private *priv = drm->dev_private;
> + struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
> const struct drm_display_mode *mode = &cstate->mode;
> - struct drm_framebuffer *fb = plane->state->fb;
> + struct drm_framebuffer *fb = priv->plane.state->fb;
[Severity: High]
This is a pre-existing issue, but does this direct access to the global plane
state in pl111_crtc_helper_atomic_enable() introduce a time-of-check to
time-of-use vulnerability?
By retrieving the framebuffer using priv->plane.state->fb directly instead
of fetching the committed atomic state via drm_atomic_get_new_plane_state(),
this might race with subsequent commits. If userspace submits a non-blocking
commit to enable the CRTC, followed immediately by another commit that
disables the primary plane (setting fb to NULL), the global state pointer
updates to the second commit. When the first commit's asynchronous worker
executes this function, it could read the new global state where fb is NULL,
causing a null pointer dereference when accessing fb->format->format.
> struct drm_connector *connector = priv->connector;
> struct drm_bridge *bridge = priv->bridge;
> bool grayscale = false;
> u32 cntl;
> u32 ppl, hsw, hfp, hbp;
> u32 lpp, vsw, vfp, vbp;
> u32 cpl, tim2;
> int ret;
>
> ret = clk_set_rate(priv->clk, mode->clock * 1000);
> if (ret) {
> drm_err(drm,
> "Failed to set pixel clock rate to %d: %d\n",
> mode->clock * 1000, ret);
> }
>
> clk_prepare_enable(priv->clk);
[Severity: High]
This is a pre-existing issue, but are blocking functions being called from an
atomic context here?
The DRM subsystem explicitly defines CRTC atomic_enable and atomic_disable
callbacks as executing in an atomic context during non-blocking commits,
where sleeping is strictly forbidden. However,
pl111_crtc_helper_atomic_enable() calls blocking functions such as
clk_prepare_enable() and msleep(). This might lead to system instability,
kernel warnings, or deadlocks.
[ ... ]
> /* The PL110 in Integrator/Versatile does the BGR routing externally */
> if (priv->variant->external_bgr)
> cntl &= ~CNTL_BGR;
>
> /* Power sequence: first enable and chill */
> writel(cntl, priv->regs + priv->ctrl);
>
> /*
> * We expect this delay to stabilize the contrast
> * voltage Vee as stipulated by the manual
> */
> msleep(20);
[Severity: High]
This is a pre-existing issue, but similar to the clk_prepare_enable() call,
does calling msleep() in pl111_crtc_helper_atomic_enable() violate the DRM
subsystem guidelines for atomic context execution?
Sleeping from this path could cause warnings or deadlocks during non-blocking
commits.
[ ... ]
> @@ -355,9 +373,9 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> drm_crtc_vblank_on(crtc);
> }
>
> -static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> +static void pl111_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
[Severity: High]
This is a pre-existing issue, but does pl111_crtc_helper_atomic_disable() leak
pending vblank events during CRTC disable?
The DRM subsystem requires that pending vblank events requested by userspace
(like during a pageflip) are consumed and sent when the CRTC is disabled. In
this code, event consumption happens in pl111_crtc_helper_atomic_flush().
However, the DRM core skips atomic_flush() for disabled CRTCs, meaning the
event must be consumed in the atomic_disable hook.
Since pl111_crtc_helper_atomic_disable() fails to check for or consume
cstate->event, the event might be leaked. This could trigger a kernel warning
in drm_atomic_helper_commit_hw_done() and cause the userspace compositor to
hang indefinitely.
> - struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *drm = crtc->dev;
> struct pl111_drm_dev_private *priv = drm->dev_private;
> u32 cntl;
[ ... ]
> /* Power Down */
> cntl = readl(priv->regs + priv->ctrl);
> if (cntl & CNTL_LCDPWR) {
> cntl &= ~CNTL_LCDPWR;
> writel(cntl, priv->regs + priv->ctrl);
> }
>
> /*
> * We expect this delay to stabilize the contrast voltage Vee as
> * stipulated by the manual
> */
> msleep(20);
>
> if (priv->variant_display_disable)
> priv->variant_display_disable(drm);
>
> /* Disable */
> writel(0, priv->regs + priv->ctrl);
>
> clk_disable_unprepare(priv->clk);
> }
[Severity: High]
This is a pre-existing issue, but does calling msleep() and
clk_disable_unprepare() in pl111_crtc_helper_atomic_disable() block execution
in an atomic context?
Since this callback runs in an atomic context during non-blocking commits,
these blocking calls might lead to system instability or deadlocks.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
` (3 preceding siblings ...)
2026-07-16 9:01 ` [PATCH v2 4/8] drm/pl111: " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:14 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 6/8] drm/repaper: " Ze Huang
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Convert gm12u320 to direct primary plane, CRTC and encoder setup.
Keep shadow-plane helper state, framebuffer access helpers and
no-scaling plane-state check from simple-KMS path.
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/tiny/gm12u320.c | 132 ++++++++++++++++++++++++++++++++--------
1 file changed, 107 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index d73dfebb4353..df60968856e5 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -8,6 +8,7 @@
#include <linux/usb.h>
#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_atomic_state_helper.h>
#include <drm/drm_connector.h>
@@ -27,7 +28,6 @@
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
static bool eco_mode;
module_param(eco_mode, bool, 0644);
@@ -87,7 +87,9 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)");
struct gm12u320_device {
struct drm_device dev;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector conn;
unsigned char *cmd_buf;
unsigned char *data_buf[GM12U320_BLOCK_COUNT];
@@ -102,7 +104,10 @@ struct gm12u320_device {
} fb_update;
};
-#define to_gm12u320(__dev) container_of(__dev, struct gm12u320_device, dev)
+static struct gm12u320_device *to_gm12u320(struct drm_device *__dev)
+{
+ return container_of(__dev, struct gm12u320_device, dev);
+}
static const char cmd_data[CMD_SIZE] = {
0x55, 0x53, 0x42, 0x43, 0x00, 0x00, 0x00, 0x00,
@@ -554,43 +559,104 @@ static int gm12u320_conn_init(struct gm12u320_device *gm12u320)
}
/* ------------------------------------------------------------------ */
-/* gm12u320 (simple) display pipe */
+/* gm12u320 display pipe */
-static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
- struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
+ struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
+ struct drm_plane_state *plane_state = gm12u320->plane.state;
struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], &rect);
}
-static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void gm12u320_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
+ struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
gm12u320_stop_fb_update(gm12u320);
}
-static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_state)
+static void gm12u320_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct drm_plane_state *state = pipe->plane.state;
+ struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_plane_state *state = drm_atomic_get_new_plane_state(commit, plane);
struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
struct drm_rect rect;
+ if (!state->fb)
+ return;
+
if (drm_atomic_helper_damage_merged(old_state, state, &rect))
gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], &rect);
}
-static const struct drm_simple_display_pipe_funcs gm12u320_pipe_funcs = {
- .enable = gm12u320_pipe_enable,
- .disable = gm12u320_pipe_disable,
- .update = gm12u320_pipe_update,
- DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+static const struct drm_plane_funcs gm12u320_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static int gm12u320_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state = NULL;
+
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+}
+
+static const struct drm_plane_helper_funcs gm12u320_plane_helper_funcs = {
+ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ .atomic_check = gm12u320_plane_helper_atomic_check,
+ .atomic_update = gm12u320_plane_helper_atomic_update,
+};
+
+static int gm12u320_crtc_helper_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static const struct drm_crtc_helper_funcs gm12u320_crtc_helper_funcs = {
+ .atomic_check = gm12u320_crtc_helper_atomic_check,
+ .atomic_enable = gm12u320_crtc_helper_atomic_enable,
+ .atomic_disable = gm12u320_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs gm12u320_crtc_funcs = {
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs gm12u320_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
static const uint32_t gm12u320_pipe_formats[] = {
@@ -677,13 +743,29 @@ static int gm12u320_usb_probe(struct usb_interface *interface,
if (ret)
return ret;
- ret = drm_simple_display_pipe_init(&gm12u320->dev,
- &gm12u320->pipe,
- &gm12u320_pipe_funcs,
- gm12u320_pipe_formats,
- ARRAY_SIZE(gm12u320_pipe_formats),
- gm12u320_pipe_modifiers,
- &gm12u320->conn);
+ ret = drm_universal_plane_init(dev, &gm12u320->plane, 0,
+ &gm12u320_plane_funcs,
+ gm12u320_pipe_formats,
+ ARRAY_SIZE(gm12u320_pipe_formats),
+ gm12u320_pipe_modifiers,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+ drm_plane_helper_add(&gm12u320->plane, &gm12u320_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(dev, &gm12u320->crtc, &gm12u320->plane, NULL,
+ &gm12u320_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+ drm_crtc_helper_add(&gm12u320->crtc, &gm12u320_crtc_helper_funcs);
+
+ ret = drm_encoder_init(dev, &gm12u320->encoder, &gm12u320_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+ gm12u320->encoder.possible_crtcs = drm_crtc_mask(&gm12u320->crtc);
+
+ ret = drm_connector_attach_encoder(&gm12u320->conn, &gm12u320->encoder);
if (ret)
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 5/8] drm/gm12u320: " Ze Huang
@ 2026-07-16 9:14 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:14 UTC (permalink / raw)
To: Ze Huang; +Cc: imx, dri-devel, Frank.Li
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition causing a NULL pointer dereference when accessing the global `plane.state`.
--
--- Patch [5]: [PATCH v2 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit cb5be3218f14cd65e9bd1d82ce85f0bd23c4cc07
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers
Convert gm12u320 to direct primary plane, CRTC and encoder setup, while
keeping shadow-plane helper state and framebuffer access helpers.
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 4ad074337af07..0fc251f07e247 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
[ ... ]
> @@ -555,43 +560,104 @@ static int gm12u320_conn_init(struct gm12u320_device *gm12u320)
> }
>
> /* ------------------------------------------------------------------ */
> -/* gm12u320 (simple) display pipe */
> +/* gm12u320 display pipe */
>
> -static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
> - struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> + struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
> + struct drm_plane_state *plane_state = gm12u320->plane.state;
[Severity: High]
Could this lead to a NULL pointer dereference if a concurrent atomic commit
modifies the state?
When gm12u320_crtc_helper_atomic_enable() reads the global
gm12u320->plane.state directly instead of extracting the stable plane state
from the atomic commit context (such as by using
drm_atomic_get_new_plane_state(commit, &gm12u320->plane)), it appears to
expose a race condition.
If a non-blocking commit is immediately followed by a blocking disable
commit, the plane's fb pointer might become NULL before this worker
has a chance to execute.
Passing this NULL fb into gm12u320_fb_mark_dirty() would then cause a panic
when the function unconditionally dereferences fb->dev.
> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>
> gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
> gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], &rect);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
` (4 preceding siblings ...)
2026-07-16 9:01 ` [PATCH v2 5/8] drm/gm12u320: " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:21 ` sashiko-bot
2026-07-16 9:01 ` [PATCH v2 7/8] drm/tve200: " Ze Huang
2026-07-16 9:02 ` [PATCH v2 8/8] drm/xen: " Ze Huang
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Convert repaper to explicit primary plane, CRTC and encoder objects.
Keep shadow-plane helpers, framebuffer access handling and no-scaling
plane-state validation from simple-KMS path.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/tiny/repaper.c | 132 ++++++++++++++++++++++++++++++++---------
1 file changed, 104 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index c8270591afc7..7057075161c4 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -22,6 +22,7 @@
#include <linux/thermal.h>
#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_connector.h>
#include <drm/drm_damage_helper.h>
@@ -38,7 +39,6 @@
#include <drm/drm_rect.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#define REPAPER_RID_G2_COG_ID 0x12
@@ -65,7 +65,9 @@ enum repaper_epd_border_byte {
struct repaper_epd {
struct drm_device drm;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
const struct drm_display_mode *mode;
struct drm_connector connector;
struct spi_device *spi;
@@ -622,26 +624,24 @@ static void power_off(struct repaper_epd *epd)
gpiod_set_value_cansleep(epd->discharge, 0);
}
-static enum drm_mode_status repaper_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+static enum drm_mode_status repaper_crtc_helper_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct repaper_epd *epd = drm_to_epd(crtc->dev);
return drm_crtc_helper_mode_valid_fixed(crtc, mode, epd->mode);
}
-static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void repaper_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct repaper_epd *epd = drm_to_epd(pipe->crtc.dev);
+ struct repaper_epd *epd = drm_to_epd(crtc->dev);
struct spi_device *spi = epd->spi;
struct device *dev = &spi->dev;
bool dc_ok = false;
int i, ret, idx;
- if (!drm_dev_enter(pipe->crtc.dev, &idx))
+ if (!drm_dev_enter(crtc->dev, &idx))
return;
DRM_DEBUG_DRIVER("\n");
@@ -771,9 +771,10 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
drm_dev_exit(idx);
}
-static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void repaper_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct repaper_epd *epd = drm_to_epd(pipe->crtc.dev);
+ struct repaper_epd *epd = drm_to_epd(crtc->dev);
struct spi_device *spi = epd->spi;
unsigned int line;
@@ -827,27 +828,84 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
power_off(epd);
}
-static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_state)
+static void repaper_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct drm_plane_state *state = pipe->plane.state;
- struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
+ struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(pstate);
struct drm_rect rect;
- if (!pipe->crtc.state->active)
+ if (!pstate->fb)
return;
- if (drm_atomic_helper_damage_merged(old_state, state, &rect))
- repaper_fb_dirty(state->fb, shadow_plane_state->data,
+ if (drm_atomic_helper_damage_merged(old_pstate, pstate, &rect))
+ repaper_fb_dirty(pstate->fb, shadow_plane_state->data,
&shadow_plane_state->fmtcnv_state);
}
-static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
- .mode_valid = repaper_pipe_mode_valid,
- .enable = repaper_pipe_enable,
- .disable = repaper_pipe_disable,
- .update = repaper_pipe_update,
- DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+static const struct drm_plane_funcs repaper_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static int repaper_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state = NULL;
+
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+}
+
+static const struct drm_plane_helper_funcs repaper_plane_helper_funcs = {
+ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ .atomic_check = repaper_plane_helper_atomic_check,
+ .atomic_update = repaper_plane_helper_atomic_update,
+};
+
+static int repaper_crtc_helper_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(state, crtc);
+}
+
+static const struct drm_crtc_helper_funcs repaper_crtc_helper_funcs = {
+ .mode_valid = repaper_crtc_helper_mode_valid,
+ .atomic_check = repaper_crtc_helper_atomic_check,
+ .atomic_enable = repaper_crtc_helper_atomic_enable,
+ .atomic_disable = repaper_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs repaper_crtc_funcs = {
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs repaper_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
static int repaper_connector_get_modes(struct drm_connector *connector)
@@ -1102,9 +1160,27 @@ static int repaper_probe(struct spi_device *spi)
if (ret)
return ret;
- ret = drm_simple_display_pipe_init(drm, &epd->pipe, &repaper_pipe_funcs,
- repaper_formats, ARRAY_SIZE(repaper_formats),
- NULL, &epd->connector);
+ ret = drm_universal_plane_init(drm, &epd->plane, 0,
+ &repaper_plane_funcs,
+ repaper_formats, ARRAY_SIZE(repaper_formats),
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+ drm_plane_helper_add(&epd->plane, &repaper_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &epd->crtc, &epd->plane, NULL,
+ &repaper_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+ drm_crtc_helper_add(&epd->crtc, &repaper_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &epd->encoder, &repaper_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+ epd->encoder.possible_crtcs = drm_crtc_mask(&epd->crtc);
+
+ ret = drm_connector_attach_encoder(&epd->connector, &epd->encoder);
if (ret)
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 6/8] drm/repaper: " Ze Huang
@ 2026-07-16 9:21 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:21 UTC (permalink / raw)
To: Ze Huang; +Cc: Frank.Li, dri-devel, imx
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Missing CRTC active check in the plane's atomic_update callback causes the driver to send SPI framebuffer updates to a powered-off display panel.
Pre-existing issues:
- [High] The CRTC atomic enable/disable and plane atomic update callbacks execute sleeping functions and allocate memory with GFP_KERNEL, which is forbidden in atomic context.
--
--- Patch [6]: [PATCH v2 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 3d9748839d37276a557c720301a5a848296e54b7
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers
Convert repaper to explicit primary plane, CRTC and encoder objects.
Keep shadow-plane helpers, framebuffer access handling and no-scaling
plane-state validation from simple-KMS path.
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index c8270591afc7f..7057075161c4d 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
[ ... ]
> -static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void repaper_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct repaper_epd *epd = drm_to_epd(pipe->crtc.dev);
> + struct repaper_epd *epd = drm_to_epd(crtc->dev);
[Severity: High]
This is a pre-existing issue, but do the CRTC atomic enable/disable and plane
atomic update callbacks execute sleeping functions in atomic context?
The DRM subsystem explicitly forbids sleeping in CRTC enable/disable and plane
update callbacks. However:
- repaper_crtc_helper_atomic_enable() calls usleep_range()
- repaper_crtc_helper_atomic_disable() calls msleep()
- repaper_plane_helper_atomic_update() calls repaper_fb_dirty(), which
allocates memory using GFP_KERNEL
Since these operations are now wired directly to the standard atomic helpers
which run in atomic context (for example, during non-blocking commits), can
this cause system instability or deadlocks?
[ ... ]
> -static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_state)
> +static void repaper_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> {
> - struct drm_plane_state *state = pipe->plane.state;
> - struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
> + struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
> + struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(pstate);
> struct drm_rect rect;
>
> - if (!pipe->crtc.state->active)
> + if (!pstate->fb)
> return;
>
> - if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> - repaper_fb_dirty(state->fb, shadow_plane_state->data,
> + if (drm_atomic_helper_damage_merged(old_pstate, pstate, &rect))
> + repaper_fb_dirty(pstate->fb, shadow_plane_state->data,
> &shadow_plane_state->fmtcnv_state);
[Severity: High]
Can this cause the driver to send SPI framebuffer updates to a powered-off
display panel?
The previous code checked !pipe->crtc.state->active here to ensure we
do not update inactive displays. During a DPMS OFF request,
drm_atomic_helper_commit_modeset_disables() runs first and disables the
panel by calling repaper_crtc_helper_atomic_disable().
Next, drm_atomic_helper_commit_planes() will still call this update
function because active_only is not set by default and pstate->visible
can remain true.
Without the crtc_state->active check, doesn't this result in writing data
to dead hardware via repaper_fb_dirty(), potentially causing SPI timeouts
or driver hangs?
> }
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
` (5 preceding siblings ...)
2026-07-16 9:01 ` [PATCH v2 6/8] drm/repaper: " Ze Huang
@ 2026-07-16 9:01 ` Ze Huang
2026-07-16 9:25 ` sashiko-bot
2026-07-16 9:02 ` [PATCH v2 8/8] drm/xen: " Ze Huang
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:01 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Convert TVE200 to explicit plane, CRTC and encoder objects.
Keep generic plane-state validation before TVE200-specific mode,
alignment, pitch and format-change checks, and install GEM framebuffer
prepare helper explicitly.
Move page-flip event handling to the CRTC commit path and wire the CRTC
vblank callbacks explicitly so events are not consumed from the plane
update path.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/tve200/tve200_display.c | 217 ++++++++++++++++++++++----------
drivers/gpu/drm/tve200/tve200_drm.h | 6 +-
drivers/gpu/drm/tve200/tve200_drv.c | 12 +-
3 files changed, 161 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
index 26b6c65ef6fd..6021f90d8531 100644
--- a/drivers/gpu/drm/tve200/tve200_display.c
+++ b/drivers/gpu/drm/tve200/tve200_display.c
@@ -15,6 +15,8 @@
#include <linux/of_graph.h>
#include <linux/delay.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_fb_dma_helper.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_framebuffer.h>
@@ -52,7 +54,7 @@ irqreturn_t tve200_irq(int irq, void *data)
val = readl(priv->regs + TVE200_CTRL);
/* We have an actual start of vsync */
if (!(val & TVE200_VSTSTYPE_BITS)) {
- drm_crtc_handle_vblank(&priv->pipe.crtc);
+ drm_crtc_handle_vblank(&priv->crtc);
/* Toggle trigger to start of active image */
val |= TVE200_VSTSTYPE_VAI;
} else {
@@ -69,13 +71,32 @@ irqreturn_t tve200_irq(int irq, void *data)
return IRQ_HANDLED;
}
-static int tve200_display_check(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *pstate,
- struct drm_crtc_state *cstate)
+static int tve200_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *old_fb = pipe->plane.state->fb;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_crtc *crtc = pstate->crtc;
+ struct drm_crtc_state *cstate = NULL;
+ const struct drm_display_mode *mode;
+ struct drm_framebuffer *old_fb = old_pstate->fb;
struct drm_framebuffer *fb = pstate->fb;
+ int ret;
+
+ if (crtc)
+ cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ ret = drm_atomic_helper_check_plane_state(pstate, cstate,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+ if (ret)
+ return ret;
+
+ if (!pstate->visible || !cstate)
+ return 0;
+
+ mode = &cstate->mode;
/*
* We support these specific resolutions and nothing else.
@@ -119,16 +140,72 @@ static int tve200_display_check(struct drm_simple_display_pipe *pipe,
return 0;
}
-static void tve200_display_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *cstate,
- struct drm_plane_state *plane_state)
+static void tve200_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
+{
+ struct tve200_drm_dev_private *priv = plane->dev->dev_private;
+ struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_framebuffer *fb = pstate->fb;
+
+ if (!fb)
+ return;
+
+ /* For RGB, the Y component is used as base address */
+ writel(drm_fb_dma_get_gem_addr(fb, pstate, 0),
+ priv->regs + TVE200_Y_FRAME_BASE_ADDR);
+
+ /* For three plane YUV we need two more addresses */
+ if (fb->format->format == DRM_FORMAT_YUV420) {
+ writel(drm_fb_dma_get_gem_addr(fb, pstate, 1),
+ priv->regs + TVE200_U_FRAME_BASE_ADDR);
+ writel(drm_fb_dma_get_gem_addr(fb, pstate, 2),
+ priv->regs + TVE200_V_FRAME_BASE_ADDR);
+ }
+}
+
+static void tve200_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+ struct drm_pending_vblank_event *event = cstate->event;
+
+ if (!event)
+ return;
+
+ cstate->event = NULL;
+
+ spin_lock_irq(&crtc->dev->event_lock);
+ if (cstate->active && drm_crtc_vblank_get(crtc) == 0)
+ drm_crtc_arm_vblank_event(crtc, event);
+ else
+ drm_crtc_send_vblank_event(crtc, event);
+ spin_unlock_irq(&crtc->dev->event_lock);
+}
+
+static const struct drm_plane_helper_funcs tve200_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = tve200_plane_helper_atomic_check,
+ .atomic_update = tve200_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs tve200_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static void tve200_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_plane *plane = &pipe->plane;
struct drm_device *drm = crtc->dev;
struct tve200_drm_dev_private *priv = drm->dev_private;
+ struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
const struct drm_display_mode *mode = &cstate->mode;
- struct drm_framebuffer *fb = plane->state->fb;
+ struct drm_plane_state *plane_state = priv->plane.state;
+ struct drm_framebuffer *fb = plane_state->fb;
struct drm_connector *connector = priv->connector;
u32 format = fb->format->format;
u32 ctrl1 = 0;
@@ -240,9 +317,9 @@ static void tve200_display_enable(struct drm_simple_display_pipe *pipe,
drm_crtc_vblank_on(crtc);
}
-static void tve200_display_disable(struct drm_simple_display_pipe *pipe)
+static void tve200_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct tve200_drm_dev_private *priv = drm->dev_private;
@@ -255,46 +332,8 @@ static void tve200_display_disable(struct drm_simple_display_pipe *pipe)
clk_disable_unprepare(priv->clk);
}
-static void tve200_display_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_pstate)
-{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_device *drm = crtc->dev;
- struct tve200_drm_dev_private *priv = drm->dev_private;
- struct drm_pending_vblank_event *event = crtc->state->event;
- struct drm_plane *plane = &pipe->plane;
- struct drm_plane_state *pstate = plane->state;
- struct drm_framebuffer *fb = pstate->fb;
-
- if (fb) {
- /* For RGB, the Y component is used as base address */
- writel(drm_fb_dma_get_gem_addr(fb, pstate, 0),
- priv->regs + TVE200_Y_FRAME_BASE_ADDR);
-
- /* For three plane YUV we need two more addresses */
- if (fb->format->format == DRM_FORMAT_YUV420) {
- writel(drm_fb_dma_get_gem_addr(fb, pstate, 1),
- priv->regs + TVE200_U_FRAME_BASE_ADDR);
- writel(drm_fb_dma_get_gem_addr(fb, pstate, 2),
- priv->regs + TVE200_V_FRAME_BASE_ADDR);
- }
- }
-
- if (event) {
- crtc->state->event = NULL;
-
- spin_lock_irq(&crtc->dev->event_lock);
- if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
- drm_crtc_arm_vblank_event(crtc, event);
- else
- drm_crtc_send_vblank_event(crtc, event);
- spin_unlock_irq(&crtc->dev->event_lock);
- }
-}
-
-static int tve200_display_enable_vblank(struct drm_simple_display_pipe *pipe)
+static int tve200_crtc_enable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct tve200_drm_dev_private *priv = drm->dev_private;
@@ -304,22 +343,48 @@ static int tve200_display_enable_vblank(struct drm_simple_display_pipe *pipe)
return 0;
}
-static void tve200_display_disable_vblank(struct drm_simple_display_pipe *pipe)
+static void tve200_crtc_disable_vblank(struct drm_crtc *crtc)
{
- struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *drm = crtc->dev;
struct tve200_drm_dev_private *priv = drm->dev_private;
writel(0, priv->regs + TVE200_INT_EN);
}
-static const struct drm_simple_display_pipe_funcs tve200_display_funcs = {
- .check = tve200_display_check,
- .enable = tve200_display_enable,
- .disable = tve200_display_disable,
- .update = tve200_display_update,
- .enable_vblank = tve200_display_enable_vblank,
- .disable_vblank = tve200_display_disable_vblank,
+static int tve200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static const struct drm_crtc_helper_funcs tve200_crtc_helper_funcs = {
+ .atomic_check = tve200_crtc_helper_atomic_check,
+ .atomic_enable = tve200_crtc_helper_atomic_enable,
+ .atomic_disable = tve200_crtc_helper_atomic_disable,
+ .atomic_flush = tve200_crtc_helper_atomic_flush,
+};
+
+static const struct drm_crtc_funcs tve200_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+ .enable_vblank = tve200_crtc_enable_vblank,
+ .disable_vblank = tve200_crtc_disable_vblank,
+};
+
+static const struct drm_encoder_funcs tve200_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
int tve200_display_init(struct drm_device *drm)
@@ -346,13 +411,31 @@ int tve200_display_init(struct drm_device *drm)
DRM_FORMAT_YUV420,
};
- ret = drm_simple_display_pipe_init(drm, &priv->pipe,
- &tve200_display_funcs,
- formats, ARRAY_SIZE(formats),
- NULL,
- priv->connector);
+ ret = drm_universal_plane_init(drm, &priv->plane, 0,
+ &tve200_plane_funcs,
+ formats, ARRAY_SIZE(formats),
+ NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret)
return ret;
+ drm_plane_helper_add(&priv->plane, &tve200_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &priv->crtc,
+ &priv->plane, NULL,
+ &tve200_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&priv->crtc, &tve200_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &priv->encoder,
+ &tve200_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+
+ priv->encoder.possible_crtcs = drm_crtc_mask(&priv->crtc);
+
return 0;
}
diff --git a/drivers/gpu/drm/tve200/tve200_drm.h b/drivers/gpu/drm/tve200/tve200_drm.h
index 5420b52ea16b..631a9f5d9aa6 100644
--- a/drivers/gpu/drm/tve200/tve200_drm.h
+++ b/drivers/gpu/drm/tve200/tve200_drm.h
@@ -15,8 +15,6 @@
#include <linux/irqreturn.h>
-#include <drm/drm_simple_kms_helper.h>
-
struct clk;
struct drm_bridge;
struct drm_connector;
@@ -107,7 +105,9 @@ struct tve200_drm_dev_private {
struct drm_connector *connector;
struct drm_panel *panel;
struct drm_bridge *bridge;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
void *regs;
struct clk *pclk;
diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
index 562f3f11812a..0b8096b1e600 100644
--- a/drivers/gpu/drm/tve200/tve200_drv.c
+++ b/drivers/gpu/drm/tve200/tve200_drv.c
@@ -105,16 +105,20 @@ static int tve200_modeset_init(struct drm_device *dev)
goto out_bridge;
}
- ret = drm_simple_display_pipe_attach_bridge(&priv->pipe,
- bridge);
+ priv->panel = panel;
+ priv->bridge = bridge;
+
+ ret = drm_bridge_attach(&priv->encoder, bridge, NULL, 0);
if (ret) {
dev_err(dev->dev, "failed to attach bridge\n");
goto out_bridge;
}
- priv->panel = panel;
priv->connector = drm_panel_bridge_connector(bridge);
- priv->bridge = bridge;
+ if (!priv->connector) {
+ ret = -ENODEV;
+ goto out_bridge;
+ }
dev_info(dev->dev, "attached to panel %s\n",
dev_name(panel->dev));
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 ` [PATCH v2 7/8] drm/tve200: " Ze Huang
@ 2026-07-16 9:25 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:25 UTC (permalink / raw)
To: Ze Huang; +Cc: imx, dri-devel, Frank.Li
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Directly accessing `priv->plane.state` in the CRTC atomic enable callback introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition with subsequent non-blocking commits.
- [High] Missing `.cleanup_fb` callback in `tve200_plane_helper_funcs` leads to memory and resource leaks of pinned GEM objects and DMA-buf mappings.
Pre-existing issues:
- [High] The CRTC atomic enable and disable callbacks call functions that can sleep (`clk_prepare_enable`, `usleep_range`), violating DRM atomic context guidelines.
--
--- Patch [7]: [PATCH v2 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 30e007b0736d27ee72ba4a5f39e203c3808e470e
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers
Convert TVE200 to explicit plane, CRTC and encoder objects. Keep generic
plane-state validation before TVE200-specific mode, alignment, pitch and
format-change checks, and install GEM framebuffer prepare helper explicitly.
> diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
> --- a/drivers/gpu/drm/tve200/tve200_display.c
> +++ b/drivers/gpu/drm/tve200/tve200_display.c
[ ... ]
> @@ -119,16 +140,72 @@ static int tve200_display_check(struct drm_simple_display_pipe *pipe,
[ ... ]
> +static const struct drm_plane_helper_funcs tve200_plane_helper_funcs = {
> + .prepare_fb = drm_gem_plane_helper_prepare_fb,
> + .atomic_check = tve200_plane_helper_atomic_check,
> + .atomic_update = tve200_plane_helper_atomic_update,
> +};
[Severity: High]
Since .prepare_fb is assigned to drm_gem_plane_helper_prepare_fb, will the
lack of a corresponding .cleanup_fb callback lead to memory and resource
leaks of pinned GEM objects and DMA-buf mappings when the framebuffer is
swapped out?
[ ... ]
> -static void tve200_display_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *cstate,
> - struct drm_plane_state *plane_state)
> +static void tve200_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_plane *plane = &pipe->plane;
> struct drm_device *drm = crtc->dev;
> struct tve200_drm_dev_private *priv = drm->dev_private;
> + struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
> const struct drm_display_mode *mode = &cstate->mode;
> - struct drm_framebuffer *fb = plane->state->fb;
> + struct drm_plane_state *plane_state = priv->plane.state;
> + struct drm_framebuffer *fb = plane_state->fb;
[Severity: High]
Could this direct access to priv->plane.state introduce a race condition
with subsequent non-blocking commits? A fast sequence of commits might swap
the global state before a previous commit's worker executes this function.
Would it be safer to extract the current commit's plane state using
drm_atomic_get_new_plane_state(commit, &priv->plane) instead to guarantee
it reads the correct snapshot?
[Severity: High]
This is a pre-existing issue, but does tve200_crtc_helper_atomic_enable()
call sleeping functions in an atomic context?
Looking further down in this function, it calls clk_prepare_enable() and
usleep_range(). Similarly, tve200_crtc_helper_atomic_disable() calls
clk_disable_unprepare(). Since atomic commit callbacks can run in an
atomic context where sleeping is forbidden, could this cause system
instability or deadlocks?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 8/8] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:01 [PATCH v2 0/8] drm: replace simple display pipe users with atomic helpers Ze Huang
` (6 preceding siblings ...)
2026-07-16 9:01 ` [PATCH v2 7/8] drm/tve200: " Ze Huang
@ 2026-07-16 9:02 ` Ze Huang
2026-07-16 9:28 ` sashiko-bot
7 siblings, 1 reply; 17+ messages in thread
From: Ze Huang @ 2026-07-16 9:02 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Joel Stanley,
Andrew Jeffery, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Linus Walleij, Hans de Goede, Alex Lanzano,
Oleksandr Andrushchenko
Cc: dri-devel, linux-kernel, linux-aspeed, linux-arm-kernel, imx,
xen-devel, Ze Huang
Replace Xen frontend simple display pipe with explicit plane, CRTC and
encoder objects for each pipeline.
Keep generic plane-state validation before Xen-specific checks, and
install GEM framebuffer prepare helper explicitly.
Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
drivers/gpu/drm/xen/xen_drm_front.h | 6 +-
drivers/gpu/drm/xen/xen_drm_front_kms.c | 175 ++++++++++++++++++++++++--------
2 files changed, 134 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h b/drivers/gpu/drm/xen/xen_drm_front.h
index a987c78abe41..02138b5abef5 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -14,7 +14,7 @@
#include <linux/scatterlist.h>
#include <drm/drm_connector.h>
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_encoder.h>
#include "xen_drm_front_cfg.h"
@@ -100,7 +100,9 @@ struct xen_drm_front_drm_pipeline {
int index;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector conn;
/* These are only for connector mode checking */
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 48772b5fe71c..f869da3f3058 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -31,9 +31,15 @@
#define FRAME_DONE_TO_MS (XEN_DRM_FRONT_WAIT_BACK_MS + 100)
static struct xen_drm_front_drm_pipeline *
-to_xen_drm_pipeline(struct drm_simple_display_pipe *pipe)
+xen_drm_pipeline_from_crtc(struct drm_crtc *crtc)
{
- return container_of(pipe, struct xen_drm_front_drm_pipeline, pipe);
+ return container_of(crtc, struct xen_drm_front_drm_pipeline, crtc);
+}
+
+static struct xen_drm_front_drm_pipeline *
+xen_drm_pipeline_from_plane(struct drm_plane *plane)
+{
+ return container_of(plane, struct xen_drm_front_drm_pipeline, plane);
}
static void fb_destroy(struct drm_framebuffer *fb)
@@ -94,7 +100,7 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
static void send_pending_event(struct xen_drm_front_drm_pipeline *pipeline)
{
- struct drm_crtc *crtc = &pipeline->pipe.crtc;
+ struct drm_crtc *crtc = &pipeline->crtc;
struct drm_device *dev = crtc->dev;
unsigned long flags;
@@ -105,24 +111,21 @@ static void send_pending_event(struct xen_drm_front_drm_pipeline *pipeline)
spin_unlock_irqrestore(&dev->event_lock, flags);
}
-static void display_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void xen_drm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct xen_drm_front_drm_pipeline *pipeline =
- to_xen_drm_pipeline(pipe);
- struct drm_crtc *crtc = &pipe->crtc;
+ struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
+ struct drm_plane_state *plane_state = pipeline->plane.state;
struct drm_framebuffer *fb = plane_state->fb;
int ret, idx;
- if (!drm_dev_enter(pipe->crtc.dev, &idx))
+ if (!drm_dev_enter(crtc->dev, &idx))
return;
ret = xen_drm_front_mode_set(pipeline, crtc->x, crtc->y,
fb->width, fb->height,
fb->format->cpp[0] * 8,
xen_drm_front_fb_to_cookie(fb));
-
if (ret) {
DRM_ERROR("Failed to enable display: %d\n", ret);
pipeline->conn_connected = false;
@@ -131,13 +134,13 @@ static void display_enable(struct drm_simple_display_pipe *pipe,
drm_dev_exit(idx);
}
-static void display_disable(struct drm_simple_display_pipe *pipe)
+static void xen_drm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
- struct xen_drm_front_drm_pipeline *pipeline =
- to_xen_drm_pipeline(pipe);
+ struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
int ret = 0, idx;
- if (drm_dev_enter(pipe->crtc.dev, &idx)) {
+ if (drm_dev_enter(crtc->dev, &idx)) {
ret = xen_drm_front_mode_set(pipeline, 0, 0, 0, 0, 0,
xen_drm_front_fb_to_cookie(NULL));
drm_dev_exit(idx);
@@ -177,12 +180,13 @@ static void pflip_to_worker(struct work_struct *work)
send_pending_event(pipeline);
}
-static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
+static bool display_send_page_flip(struct xen_drm_front_drm_pipeline *pipeline,
+ struct drm_atomic_commit *commit,
struct drm_plane_state *old_plane_state)
{
struct drm_plane_state *plane_state =
- drm_atomic_get_new_plane_state(old_plane_state->state,
- &pipe->plane);
+ drm_atomic_get_new_plane_state(commit,
+ &pipeline->plane);
/*
* If old_plane_state->fb is NULL and plane_state->fb is not,
@@ -193,8 +197,6 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
* sent to the backend as a part of display_set_config call.
*/
if (old_plane_state->fb && plane_state->fb) {
- struct xen_drm_front_drm_pipeline *pipeline =
- to_xen_drm_pipeline(pipe);
struct xen_drm_front_drm_info *drm_info = pipeline->drm_info;
int ret;
@@ -224,10 +226,27 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
return false;
}
-static int display_check(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *plane_state,
- struct drm_crtc_state *crtc_state)
+static int xen_drm_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state = NULL;
+ int ret;
+
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+
+ ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+ if (ret)
+ return ret;
+
+ if (!plane_state->visible || !crtc_state)
+ return 0;
+
/*
* Xen doesn't initialize vblanking via drm_vblank_init(), so
* DRM helpers assume that it doesn't handle vblanking and start
@@ -242,12 +261,12 @@ static int display_check(struct drm_simple_display_pipe *pipe,
return 0;
}
-static void display_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_plane_state)
+static void xen_drm_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *commit)
{
- struct xen_drm_front_drm_pipeline *pipeline =
- to_xen_drm_pipeline(pipe);
- struct drm_crtc *crtc = &pipe->crtc;
+ struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(commit, plane);
+ struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_plane(plane);
+ struct drm_crtc *crtc = &pipeline->crtc;
struct drm_pending_vblank_event *event;
int idx;
@@ -265,7 +284,7 @@ static void display_update(struct drm_simple_display_pipe *pipe,
spin_unlock_irqrestore(&dev->event_lock, flags);
}
- if (!drm_dev_enter(pipe->crtc.dev, &idx)) {
+ if (!drm_dev_enter(crtc->dev, &idx)) {
send_pending_event(pipeline);
return;
}
@@ -278,19 +297,17 @@ static void display_update(struct drm_simple_display_pipe *pipe,
* If this is not a page flip, e.g. no flip done event from the backend
* is expected, then send now.
*/
- if (!display_send_page_flip(pipe, old_plane_state))
+ if (!display_send_page_flip(pipeline, commit, old_plane_state))
send_pending_event(pipeline);
drm_dev_exit(idx);
}
static enum drm_mode_status
-display_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+xen_drm_crtc_helper_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct xen_drm_front_drm_pipeline *pipeline =
- container_of(pipe, struct xen_drm_front_drm_pipeline,
- pipe);
+ struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
if (mode->hdisplay != pipeline->width)
return MODE_ERROR;
@@ -301,12 +318,53 @@ display_mode_valid(struct drm_simple_display_pipe *pipe,
return MODE_OK;
}
-static const struct drm_simple_display_pipe_funcs display_funcs = {
- .mode_valid = display_mode_valid,
- .enable = display_enable,
- .disable = display_disable,
- .check = display_check,
- .update = display_update,
+static int xen_drm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *commit)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+ int ret;
+
+ if (crtc_state->enable) {
+ ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+ if (ret)
+ return ret;
+ }
+
+ return drm_atomic_add_affected_planes(commit, crtc);
+}
+
+static const struct drm_plane_helper_funcs display_plane_helper_funcs = {
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = xen_drm_plane_helper_atomic_check,
+ .atomic_update = xen_drm_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs display_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_crtc_helper_funcs display_crtc_helper_funcs = {
+ .mode_valid = xen_drm_crtc_helper_mode_valid,
+ .atomic_check = xen_drm_crtc_helper_atomic_check,
+ .atomic_enable = xen_drm_crtc_helper_atomic_enable,
+ .atomic_disable = xen_drm_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs display_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs display_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
};
static int display_pipe_init(struct xen_drm_front_drm_info *drm_info,
@@ -331,10 +389,37 @@ static int display_pipe_init(struct xen_drm_front_drm_info *drm_info,
formats = xen_drm_front_conn_get_formats(&format_count);
- return drm_simple_display_pipe_init(dev, &pipeline->pipe,
- &display_funcs, formats,
- format_count, NULL,
- &pipeline->conn);
+ ret = drm_universal_plane_init(dev, &pipeline->plane, 0,
+ &display_plane_funcs,
+ formats, format_count,
+ NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+
+ drm_plane_helper_add(&pipeline->plane, &display_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(dev, &pipeline->crtc,
+ &pipeline->plane, NULL,
+ &display_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&pipeline->crtc, &display_crtc_helper_funcs);
+
+ ret = drm_encoder_init(dev, &pipeline->encoder,
+ &display_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret)
+ return ret;
+
+ pipeline->encoder.possible_crtcs = drm_crtc_mask(&pipeline->crtc);
+
+ ret = drm_connector_attach_encoder(&pipeline->conn, &pipeline->encoder);
+ if (ret)
+ return ret;
+
+ return 0;
}
int xen_drm_front_kms_init(struct xen_drm_front_drm_info *drm_info)
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 8/8] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers
2026-07-16 9:02 ` [PATCH v2 8/8] drm/xen: " Ze Huang
@ 2026-07-16 9:28 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-16 9:28 UTC (permalink / raw)
To: Ze Huang; +Cc: dri-devel, Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Critical] Global state data race in `atomic_enable` leading to a NULL pointer dereference.
- [High] Pending vblank events are leaked when the CRTC is disabled, causing userspace to hang.
Pre-existing issues:
- [High] Global state data race in `atomic_update` causing stolen events and userspace hangs.
--
--- Patch [8]: [PATCH v2 8/8] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 545da3a98a8e32db9ed9bc44680620afafb3b601
Author: Ze Huang <ze.huang@oss.qualcomm.com>
drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers
Replace Xen frontend simple display pipe with explicit plane, CRTC and
encoder objects for each pipeline.
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -105,24 +111,21 @@ static void send_pending_event(struct xen_drm_front_drm_pipeline *pipeline)
> spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> -static void display_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void xen_drm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct xen_drm_front_drm_pipeline *pipeline =
> - to_xen_drm_pipeline(pipe);
> - struct drm_crtc *crtc = &pipe->crtc;
> + struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
> + struct drm_plane_state *plane_state = pipeline->plane.state;
> + struct drm_framebuffer *fb = plane_state->fb;
> int ret, idx;
>
> - if (!drm_dev_enter(pipe->crtc.dev, &idx))
> + if (!drm_dev_enter(crtc->dev, &idx))
> return;
>
> ret = xen_drm_front_mode_set(pipeline, crtc->x, crtc->y,
> fb->width, fb->height,
[Severity: Critical]
Can this sequence cause a NULL pointer dereference?
In xen_drm_crtc_helper_atomic_enable(), the global plane state is
read asynchronously and fb is dereferenced directly.
If userspace submits two non-blocking atomic commits rapidly (the
first enabling the CRTC and plane, and the second disabling the plane),
the global state pointer (pipeline->plane.state) is swapped
synchronously during the ioctl.
When the first commit's worker executes, it reads the newer state where
fb is NULL, and unconditionally dereferences fb->width.
Would it be safer to extract the state from the commit object using
drm_atomic_get_new_plane_state(commit, &pipeline->plane)?
> fb->format->cpp[0] * 8,
> xen_drm_front_fb_to_cookie(fb));
> -
> if (ret) {
> DRM_ERROR("Failed to enable display: %d\n", ret);
[ ... ]
> @@ -131,13 +134,13 @@ static void display_enable(struct drm_simple_display_pipe *pipe,
> drm_dev_exit(idx);
> }
>
> -static void display_disable(struct drm_simple_display_pipe *pipe)
> +static void xen_drm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct xen_drm_front_drm_pipeline *pipeline =
> - to_xen_drm_pipeline(pipe);
> + struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
> int ret = 0, idx;
>
> - if (drm_dev_enter(pipe->crtc.dev, &idx)) {
> + if (drm_dev_enter(crtc->dev, &idx)) {
> ret = xen_drm_front_mode_set(pipeline, 0, 0, 0, 0, 0,
> xen_drm_front_fb_to_cookie(NULL));
> drm_dev_exit(idx);
> @@ -147,6 +150,11 @@ static void display_disable(struct drm_simple_display_pipe *pipe)
>
> /* Make sure we can restart with enabled connector next time */
> pipeline->conn_connected = true;
> +
> + /* release stalled event if any */
> + send_pending_event(pipeline);
> +}
[Severity: High]
Does this function leak the vblank event when the CRTC is disabled?
When moving away from drm_simple_kms_helper, the driver loses the core
helper's automatic consumption of crtc_state->event during CRTC
disable.
If userspace submits an atomic commit that disables the CRTC and
requests a vblank event (e.g., modeset to 0 or DPMS off),
xen_drm_crtc_helper_atomic_disable() completely omits the consumption and
signaling of the event.
This leaves the event dangling, which causes a kernel warning in the
commit tail and hangs the userspace compositor indefinitely as it waits
for the pageflip event.
Should this function pull and signal the event from
drm_atomic_get_new_crtc_state(commit, crtc)->event?
[ ... ]
> @@ -242,12 +261,12 @@ static int display_check(struct drm_simple_display_pipe *pipe,
> return 0;
> }
>
> -static void display_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_plane_state)
> +static void xen_drm_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> {
> - struct xen_drm_front_drm_pipeline *pipeline =
> - to_xen_drm_pipeline(pipe);
> - struct drm_crtc *crtc = &pipe->crtc;
> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(commit, plane);
> + struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_plane(plane);
> + struct drm_crtc *crtc = &pipeline->crtc;
> struct drm_pending_vblank_event *event;
> int idx;
[Severity: High]
This is a pre-existing issue, but can this code steal events from
future commits?
In xen_drm_plane_helper_atomic_update(), the global CRTC state's event
is accessed directly:
event = crtc->state->event;
Since crtc->state is swapped synchronously, it may reflect a future
commit's state by the time this worker runs. The worker would then steal
the future commit's event and clear it.
When the future commit's worker runs, it finds no event, while the
current commit's original event is stranded on its old state object and
never signaled, resulting in userspace hangs.
Should the event be fetched reliably from the specific commit object
using drm_atomic_get_new_crtc_state(commit, crtc)->event?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=8
^ permalink raw reply [flat|nested] 17+ messages in thread