[PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Ze Huang]

Convert i.MX LCDC to explicit primary plane, CRTC and encoder objects.
Keep no-scaling plane check and GEM framebuffer prepare callback from
simple-KMS path.

Wire the vblank lifecycle explicitly with CRTC vblank callbacks and
drm_crtc_vblank_on()/drm_crtc_vblank_off(). Use the old CRTC state in the
disable path for clock unwinding so the clock reference count remains
paired with the previous active state.

Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
---
struct drm_simple_display_pipe was meant to simplify simple DRM
drivers, but instead adds an extra wrapper around normal DRM atomic
helper setup. As noted in Documentation/gpu/todo.rst, remaining users
should be converted to regular atomic helpers and stop depending on the
simple-KMS interfaces.

Each patch replaces drm_simple_display_pipe_init() with explicit
primary plane, CRTC and encoder setup, and moves the old simple-pipe
callbacks into regular plane and CRTC helper callbacks named according
to local driver conventions.

The conversions preserve helper behavior that used to be implicit in
drm_simple_kms_helper.c, including plane-state validation, CRTC
primary-plane checks, affected-plane propagation, framebuffer prepare
handling, and existing event/vblank flow where applicable.

Result is less helper indirection and more explicit driver-side atomic
wiring, with no remaining simple-KMS dependency in these drivers.

Changes are build-tested only. No hardware testing has been performed.

This patch is based on drm-next-2026-06-27.

AI usage disclosure:
- AI tools were also used to review the code and suggest code changes for
  the DRM atomic conversion.

Thanks,
Ze Huang
---
Changes in v2:
- use 'commit' as name of struct drm_atomic_commit in atomic helpers
- improve control flow in *_crtc_helper_atomic_check() and
  *_plane_helper_atomic_check()
- Moved page-flip/vblank event handling out of plane update paths and into
  CRTC atomic_flush(), using atomic_flush and disable paths
- decide clock disable by old crtc state
- add calls to drm_crtc_vblank_on() and drm_crtc_vblank_off() in crtc
  atomic_enable() and atomic_disbale()
- test crtc_state->enable before mode size check
- Link to v1: https://patch.msgid.link/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com
---
 drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 217 ++++++++++++++++++++++++++----------
 1 file changed, 159 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
index f52832b43aca..78eac2c4a54c 100644
--- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
+++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
@@ -14,9 +14,9 @@
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_of.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 #include <drm/drm_vblank.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
@@ -102,7 +102,9 @@
 
 struct imx_lcdc {
 	struct drm_device drm;
-	struct drm_simple_display_pipe pipe;
+	struct drm_plane plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
 	struct drm_connector *connector;
 	void __iomem *base;
 
@@ -135,14 +137,13 @@ static unsigned int imx_lcdc_get_format(unsigned int drm_format)
 	}
 }
 
-static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
+static void imx_lcdc_update_hw_registers(struct drm_crtc *crtc,
 					 struct drm_plane_state *old_state,
 					 bool mode_set)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct drm_plane_state *new_state = pipe->plane.state;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
+	struct drm_plane_state *new_state = lcdc->plane.state;
 	struct drm_framebuffer *fb = new_state->fb;
-	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
 	u32 lpcr, lvcr, lhcr;
 	u32 framesize;
 	dma_addr_t addr;
@@ -188,15 +189,16 @@ static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
 		clk_prepare_enable(lcdc->clk_per);
 }
 
-static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
-				 struct drm_crtc_state *crtc_state,
-				 struct drm_plane_state *plane_state)
+static void imx_lcdc_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+					       struct drm_atomic_commit *commit)
 {
 	int ret;
 	int clk_div;
 	int bpp;
-	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
-	struct drm_display_mode *mode = &pipe->crtc.mode;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
+	struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
+	struct drm_plane_state *plane_state = lcdc->plane.state;
+	struct drm_display_mode *mode = &cstate->mode;
 	struct drm_display_info *disp_info = &lcdc->connector->display_info;
 	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
 	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
@@ -231,40 +233,46 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	ret = clk_prepare_enable(lcdc->clk_ipg);
 	if (ret) {
-		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
+		dev_err(crtc->dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
 		return;
 	}
 	ret = clk_prepare_enable(lcdc->clk_ahb);
 	if (ret) {
-		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
+		dev_err(crtc->dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
 
 		clk_disable_unprepare(lcdc->clk_ipg);
 
 		return;
 	}
 
-	imx_lcdc_update_hw_registers(pipe, NULL, true);
+	imx_lcdc_update_hw_registers(crtc, NULL, true);
 
 	/* Enable VBLANK Interrupt */
 	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
+
+	drm_crtc_vblank_on(crtc);
 }
 
-static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void imx_lcdc_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+						struct drm_atomic_commit *commit)
 {
-	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
-	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
 	struct drm_pending_vblank_event *event;
 
+	drm_crtc_vblank_off(crtc);
+
 	clk_disable_unprepare(lcdc->clk_ahb);
 	clk_disable_unprepare(lcdc->clk_ipg);
 
-	if (pipe->crtc.enabled)
+	if (old_crtc_state->enable)
 		clk_disable_unprepare(lcdc->clk_per);
 
 	spin_lock_irq(&lcdc->drm.event_lock);
-	event = crtc->state->event;
+	event = new_crtc_state->event;
 	if (event) {
-		crtc->state->event = NULL;
+		new_crtc_state->event = NULL;
 		drm_crtc_send_vblank_event(crtc, event);
 	}
 	spin_unlock_irq(&lcdc->drm.event_lock);
@@ -273,17 +281,20 @@ static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
 	writel(0, lcdc->base + IMX21LCDC_LIER);
 }
 
-static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
-			       struct drm_plane_state *plane_state,
-			       struct drm_crtc_state *crtc_state)
+static int imx_lcdc_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);
+	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
 	const struct drm_display_mode *mode = &crtc_state->mode;
-	const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
+	const struct drm_display_mode *old_mode = &old_crtc_state->mode;
+	int ret;
 
-	if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
-	    mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
-	    mode->hdisplay % 0x10) { /* must be multiple of 16 */
-		drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
+	if (crtc_state->enable &&
+	    (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
+	     mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
+	     mode->hdisplay % 0x10)) { /* must be multiple of 16 */
+		drm_err(crtc->dev, "unsupported display mode (%u x %u)\n",
 			mode->hdisplay, mode->vdisplay);
 		return -EINVAL;
 	}
@@ -292,46 +303,116 @@ static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
 		old_mode->hdisplay != mode->hdisplay ||
 		old_mode->vdisplay != mode->vdisplay;
 
-	return 0;
+	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 imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
-				 struct drm_plane_state *old_state)
+static void imx_lcdc_plane_helper_atomic_update(struct drm_plane *plane,
+						struct drm_atomic_commit *commit)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct drm_pending_vblank_event *event = crtc->state->event;
-	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
+	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(commit, plane);
+	struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_framebuffer *old_fb = old_state->fb;
 	struct drm_crtc *old_crtc = old_state->crtc;
 	bool mode_changed = false;
 
+	if (!fb)
+		return;
+
 	if (old_fb && old_fb->format != fb->format)
 		mode_changed = true;
 	else if (old_crtc != crtc)
 		mode_changed = true;
 
-	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
+	imx_lcdc_update_hw_registers(crtc, old_state, mode_changed);
+}
 
-	if (event) {
-		crtc->state->event = NULL;
+static int imx_lcdc_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;
 
-		spin_lock_irq(&crtc->dev->event_lock);
+	if (crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
 
-		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);
+	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
 
-		spin_unlock_irq(&crtc->dev->event_lock);
-	}
+static const struct drm_plane_helper_funcs imx_lcdc_plane_helper_funcs = {
+	.prepare_fb	= drm_gem_plane_helper_prepare_fb,
+	.atomic_check	= imx_lcdc_plane_helper_atomic_check,
+	.atomic_update	= imx_lcdc_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs imx_lcdc_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 imx_lcdc_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 (new_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 const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
-	.enable = imx_lcdc_pipe_enable,
-	.disable = imx_lcdc_pipe_disable,
-	.check = imx_lcdc_pipe_check,
-	.update = imx_lcdc_pipe_update,
+static const struct drm_crtc_helper_funcs imx_lcdc_crtc_helper_funcs = {
+	.atomic_check	= imx_lcdc_crtc_helper_atomic_check,
+	.atomic_enable	= imx_lcdc_crtc_helper_atomic_enable,
+	.atomic_disable	= imx_lcdc_crtc_helper_atomic_disable,
+	.atomic_flush	= imx_lcdc_crtc_helper_atomic_flush,
+};
+
+static int imx_lcdc_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+	return 0;
+}
+
+static void imx_lcdc_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+}
+
+static const struct drm_crtc_funcs imx_lcdc_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		= imx_lcdc_crtc_enable_vblank,
+	.disable_vblank		= imx_lcdc_crtc_disable_vblank,
+};
+
+static const struct drm_encoder_funcs imx_lcdc_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
 };
 
 static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
@@ -369,7 +450,7 @@ MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
 static irqreturn_t imx_lcdc_irq_handler(int irq, void *arg)
 {
 	struct imx_lcdc *lcdc = arg;
-	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	struct drm_crtc *crtc = &lcdc->crtc;
 	unsigned int status;
 
 	status = readl(lcdc->base + IMX21LCDC_LISR);
@@ -387,6 +468,9 @@ static int imx_lcdc_probe(struct platform_device *pdev)
 	struct imx_lcdc *lcdc;
 	struct drm_device *drm;
 	struct drm_bridge *bridge;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
 	int irq;
 	int ret;
 	struct device *dev = &pdev->dev;
@@ -428,23 +512,40 @@ static int imx_lcdc_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Cannot initialize mode configuration structure\n");
 
-	/* CRTC, Plane, Encoder */
-	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
-					   &imx_lcdc_pipe_funcs,
-					   imx_lcdc_formats,
-					   ARRAY_SIZE(imx_lcdc_formats), NULL, NULL);
+	plane = &lcdc->plane;
+	ret = drm_universal_plane_init(drm, plane, 0,
+				       &imx_lcdc_plane_funcs,
+				       imx_lcdc_formats,
+				       ARRAY_SIZE(imx_lcdc_formats),
+				       NULL,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Cannot initialize primary plane\n");
+	drm_plane_helper_add(plane, &imx_lcdc_plane_helper_funcs);
+
+	crtc = &lcdc->crtc;
+	ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
+					&imx_lcdc_crtc_funcs, NULL);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Cannot initialize CRTC\n");
+	drm_crtc_helper_add(crtc, &imx_lcdc_crtc_helper_funcs);
+
+	encoder = &lcdc->encoder;
+	ret = drm_encoder_init(drm, encoder, &imx_lcdc_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret < 0)
-		return dev_err_probe(drm->dev, ret, "Cannot setup simple display pipe\n");
+		return dev_err_probe(drm->dev, ret, "Cannot initialize encoder\n");
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
 	if (ret < 0)
 		return dev_err_probe(drm->dev, ret, "Failed to initialize vblank\n");
 
-	ret = drm_bridge_attach(&lcdc->pipe.encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret)
 		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
 
-	lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
+	lcdc->connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(lcdc->connector))
 		return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
 

---
base-commit: 3696d07837d1df13a5603d77f667685e7dfb3c53
change-id: 20260704-drm-simple-kms-removal-01a031c6a129

Best regards,
-- 
Ze Huang <ze.huang@oss.qualcomm.com>

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Missing framebuffer validation in atomic paths leads to an unconditional NULL pointer dereference when a plane is configured without a framebuffer.
- [High] `imx_lcdc_plane_helper_atomic_check` fetches a potentially NULL CRTC state, causing a kernel panic in core helpers.
- [High] Redundant clock enable calls cause a persistent reference count leak for `clk_per` on every display modeset.

Pre-existing issues:
- [Critical] The driver accesses peripheral MMIO registers and leaves interrupts unmasked while the AHB and IPG bus clocks are disabled.
- [High] The CRTC atomic check unconditionally overwrites the `mode_changed` flag, potentially discarding modeset requests initiated by the DRM core.
- [High] VBLANK completion events are signaled prematurely during display disable/modeset sequences.
--

--- Patch [1]: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 78bda5593353310f7405860b2b4d7ea3060e0afa
Author: Ze Huang <ze.huang@oss.qualcomm.com>
Subject: drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

This commit converts the i.MX LCDC driver to explicit primary plane, CRTC, 
and encoder objects, removing the dependency on the simple-KMS framework. 
It preserves most of the previous callbacks by rewiring them into the 
standard atomic helper paths.

Link: https://patch.msgid.link/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com

> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> index c67fe80b8115e..5c858fc81a78c 100644
> --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c

[ ... ]

> -static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
> -				 struct drm_crtc_state *crtc_state,
> -				 struct drm_plane_state *plane_state)
> +static void imx_lcdc_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					       struct drm_atomic_commit *commit)
>  {
>  	int ret;
>  	int clk_div;
>  	int bpp;
> -	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> -	struct drm_display_mode *mode = &pipe->crtc.mode;
> +	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
> +	struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
> +	struct drm_plane_state *plane_state = lcdc->plane.state;

[Severity: High]
Does this code result in a NULL pointer dereference when a primary plane is
configured without a framebuffer?

If userspace submits an atomic commit that enables the CRTC but sets the
primary plane's FB_ID to 0, plane_state->fb will be NULL.

Since drm_atomic_helper_check_crtc_primary_plane only verifies that the
primary plane is present in the plane mask, it does not prevent a NULL
framebuffer. This causes an unconditional dereference shortly after this line
when extracting the format via imx_lcdc_get_format(), as well as inside 
imx_lcdc_update_hw_registers() when calling drm_fb_dma_get_gem_addr().

> +	struct drm_display_mode *mode = &cstate->mode;
>  	struct drm_display_info *disp_info = &lcdc->connector->display_info;
>  	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
>  	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;

[ ... ]

> @@ -230,40 +232,46 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	ret = clk_prepare_enable(lcdc->clk_ipg);
>  	if (ret) {
> -		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
> +		dev_err(crtc->dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
>  		return;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this perform memory-mapped writes to
peripheral registers before the IPG and AHB bus clocks are enabled?

Just prior to enabling the IPG clock here, the driver performs multiple
writel() configuration writes to the LPCR and LCPR registers. Accessing
registers of an unclocked IP block on i.MX platforms can hang the bus and
trigger a synchronous external abort. Could we ensure the clocks are
prepared and enabled before doing configuration writes?

>  	ret = clk_prepare_enable(lcdc->clk_ahb);
>  	if (ret) {
> -		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
> +		dev_err(crtc->dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
>  
>  		clk_disable_unprepare(lcdc->clk_ipg);
>  
>  		return;
>  	}
>  
> -	imx_lcdc_update_hw_registers(pipe, NULL, true);
> +	imx_lcdc_update_hw_registers(crtc, NULL, true);
>  
>  	/* Enable VBLANK Interrupt */
>  	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
> +
> +	drm_crtc_vblank_on(crtc);
>  }
>  
> -static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void imx_lcdc_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +						struct drm_atomic_commit *commit)
>  {
> -	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> -	struct drm_crtc *crtc = &lcdc->pipe.crtc;
> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
> +	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> +	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
>  	struct drm_pending_vblank_event *event;
>  
> +	drm_crtc_vblank_off(crtc);
> +
>  	clk_disable_unprepare(lcdc->clk_ahb);
>  	clk_disable_unprepare(lcdc->clk_ipg);
>  
> -	if (pipe->crtc.enabled)
> +	if (old_crtc_state->enable)
>  		clk_disable_unprepare(lcdc->clk_per);
>  
>  	spin_lock_irq(&lcdc->drm.event_lock);
> -	event = crtc->state->event;
> +	event = new_crtc_state->event;
>  	if (event) {
> -		crtc->state->event = NULL;
> +		new_crtc_state->event = NULL;
>  		drm_crtc_send_vblank_event(crtc, event);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does consuming the event
during the disable phase signal completion to userspace prematurely?

Sending the event here signals to the compositor that the modeset is complete
before the new mode is programmed and flushed, which could lead to frame
submission races or compositor stalls.

>  	spin_unlock_irq(&lcdc->drm.event_lock);
> @@ -272,17 +280,20 @@ static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
>  	writel(0, lcdc->base + IMX21LCDC_LIER);

[Severity: Critical]
This is a pre-existing issue, but are the bus clocks disabled before clearing
the VBLANK interrupt?

Similar to the enable path, writing to the LIER register while the IPG and
AHB clocks are disabled can trigger a bus fault. Furthermore, since the
interrupt is unmasked before the clocks are disabled, an incoming IRQ could
cause the handler to read the LISR register while clocks are off.

>  }
>  
> -static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
> -			       struct drm_plane_state *plane_state,
> -			       struct drm_crtc_state *crtc_state)
> +static int imx_lcdc_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);
> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
>  	const struct drm_display_mode *mode = &crtc_state->mode;
> -	const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
> +	const struct drm_display_mode *old_mode = &old_crtc_state->mode;
> +	int ret;
>  
> -	if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
> -	    mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
> -	    mode->hdisplay % 0x10) { /* must be multiple of 16 */
> -		drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
> +	if (crtc_state->enable &&
> +	    (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
> +	     mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
> +	     mode->hdisplay % 0x10)) { /* must be multiple of 16 */
> +		drm_err(crtc->dev, "unsupported display mode (%u x %u)\n",
>  			mode->hdisplay, mode->vdisplay);
>  		return -EINVAL;
>  	}
> @@ -291,46 +302,116 @@ static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
>  		old_mode->hdisplay != mode->hdisplay ||
>  		old_mode->vdisplay != mode->vdisplay;

[Severity: High]
This is a pre-existing issue, but does this direct assignment to
mode_changed discard modeset requests initiated by the DRM core?

The core drm_atomic_helper_check_modeset() function may have already set
mode_changed to true due to connector routing changes or userspace forcing a
modeset. Using a direct assignment instead of a bitwise OR (|=) silently
clears that flag if the spatial dimensions haven't changed.

> -	return 0;
> +	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 imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
> -				 struct drm_plane_state *old_state)
> +static void imx_lcdc_plane_helper_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_commit *commit)
>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_pending_vblank_event *event = crtc->state->event;
> -	struct drm_plane_state *new_state = pipe->plane.state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(commit, plane);
> +	struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_framebuffer *old_fb = old_state->fb;
>  	struct drm_crtc *old_crtc = old_state->crtc;
>  	bool mode_changed = false;
>  
> +	if (!fb)
> +		return;
> +
>  	if (old_fb && old_fb->format != fb->format)
>  		mode_changed = true;
>  	else if (old_crtc != crtc)
>  		mode_changed = true;
>  
> -	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
> +	imx_lcdc_update_hw_registers(crtc, old_state, mode_changed);
> +}

[Severity: High]
Does calling imx_lcdc_update_hw_registers() here cause a persistent reference
count leak for clk_per?

During a modeset, imx_lcdc_crtc_helper_atomic_enable() already calls
imx_lcdc_update_hw_registers() which increments the clock reference count
when the CRTC is enabled.

When the plane helper subsequently calls it again here, the clock prepare
enable block is hit a second time. However, during display disable, the
plane update returns early due to a NULL fb, meaning the clock is only
disabled once in the CRTC disable path.

>  
> -	if (event) {
> -		crtc->state->event = NULL;
> +static int imx_lcdc_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;
>  
> -		spin_lock_irq(&crtc->dev->event_lock);
> +	if (crtc)
> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>  
> -		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);
> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,

[Severity: High]
Can fetching the CRTC state this way cause a kernel panic in the core atomic
helpers?

If userspace submits a commit that modifies only plane properties without
affecting the CRTC, the CRTC is not implicitly added to the atomic commit
state.

In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
when it dereferences crtc_state->enable.

Could this use drm_atomic_get_crtc_state() instead to ensure the state is
brought into the commit if it is missing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-1-7e4a6b8528e6@oss.qualcomm.com?part=1

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Ze Huang]

On Thu Jul 16, 2026 at 5:14 PM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
>
> New issues:
> - [High] Missing framebuffer validation in atomic paths leads to an unconditional NULL pointer dereference when a plane is configured without a framebuffer.
> - [High] `imx_lcdc_plane_helper_atomic_check` fetches a potentially NULL CRTC state, causing a kernel panic in core helpers.
> - [High] Redundant clock enable calls cause a persistent reference count leak for `clk_per` on every display modeset.
>
> Pre-existing issues:
> - [Critical] The driver accesses peripheral MMIO registers and leaves interrupts unmasked while the AHB and IPG bus clocks are disabled.
> - [High] The CRTC atomic check unconditionally overwrites the `mode_changed` flag, potentially discarding modeset requests initiated by the DRM core.
> - [High] VBLANK completion events are signaled prematurely during display disable/modeset sequences.
> --
>
> --- Patch [1]: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers ---
> commit 78bda5593353310f7405860b2b4d7ea3060e0afa
> Author: Ze Huang <ze.huang@oss.qualcomm.com>
> Subject: drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers
>
> This commit converts the i.MX LCDC driver to explicit primary plane, CRTC, 
> and encoder objects, removing the dependency on the simple-KMS framework. 
> It preserves most of the previous callbacks by rewiring them into the 
> standard atomic helper paths.
>
> Link: https://patch.msgid.link/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com
>
>> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
>> index c67fe80b8115e..5c858fc81a78c 100644
>> --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
>> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
>
> [ ... ]
>
>> -static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>> -				 struct drm_crtc_state *crtc_state,
>> -				 struct drm_plane_state *plane_state)
>> +static void imx_lcdc_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>> +					       struct drm_atomic_commit *commit)
>>  {
>>  	int ret;
>>  	int clk_div;
>>  	int bpp;
>> -	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
>> -	struct drm_display_mode *mode = &pipe->crtc.mode;
>> +	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
>> +	struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
>> +	struct drm_plane_state *plane_state = lcdc->plane.state;
>
> [Severity: High]
> Does this code result in a NULL pointer dereference when a primary plane is
> configured without a framebuffer?
>
> If userspace submits an atomic commit that enables the CRTC but sets the
> primary plane's FB_ID to 0, plane_state->fb will be NULL.
>

Will access plane_state with drm_atomic_get_new_plane_state()

>
> Since drm_atomic_helper_check_crtc_primary_plane only verifies that the
> primary plane is present in the plane mask, it does not prevent a NULL
> framebuffer. This causes an unconditional dereference shortly after this line
> when extracting the format via imx_lcdc_get_format(), as well as inside 
> imx_lcdc_update_hw_registers() when calling drm_fb_dma_get_gem_addr().
>
>> +	struct drm_display_mode *mode = &cstate->mode;
>>  	struct drm_display_info *disp_info = &lcdc->connector->display_info;
>>  	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
>>  	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
>
> [ ... ]
>
>> @@ -230,40 +232,46 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>>  
>>  	ret = clk_prepare_enable(lcdc->clk_ipg);
>>  	if (ret) {
>> -		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
>> +		dev_err(crtc->dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
>>  		return;
>>  	}
>
> [Severity: Critical]
> This is a pre-existing issue, but does this perform memory-mapped writes to
> peripheral registers before the IPG and AHB bus clocks are enabled?
>
> Just prior to enabling the IPG clock here, the driver performs multiple
> writel() configuration writes to the LPCR and LCPR registers. Accessing
> registers of an unclocked IP block on i.MX platforms can hang the bus and
> trigger a synchronous external abort. Could we ensure the clocks are
> prepared and enabled before doing configuration writes?
>
>>  	ret = clk_prepare_enable(lcdc->clk_ahb);
>>  	if (ret) {
>> -		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
>> +		dev_err(crtc->dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
>>  
>>  		clk_disable_unprepare(lcdc->clk_ipg);
>>  
>>  		return;
>>  	}
>>  
>> -	imx_lcdc_update_hw_registers(pipe, NULL, true);
>> +	imx_lcdc_update_hw_registers(crtc, NULL, true);
>>  
>>  	/* Enable VBLANK Interrupt */
>>  	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
>> +
>> +	drm_crtc_vblank_on(crtc);
>>  }
>>  
>> -static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
>> +static void imx_lcdc_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +						struct drm_atomic_commit *commit)
>>  {
>> -	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
>> -	struct drm_crtc *crtc = &lcdc->pipe.crtc;
>> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
>> +	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>> +	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
>>  	struct drm_pending_vblank_event *event;
>>  
>> +	drm_crtc_vblank_off(crtc);
>> +
>>  	clk_disable_unprepare(lcdc->clk_ahb);
>>  	clk_disable_unprepare(lcdc->clk_ipg);
>>  
>> -	if (pipe->crtc.enabled)
>> +	if (old_crtc_state->enable)
>>  		clk_disable_unprepare(lcdc->clk_per);
>>  
>>  	spin_lock_irq(&lcdc->drm.event_lock);
>> -	event = crtc->state->event;
>> +	event = new_crtc_state->event;
>>  	if (event) {
>> -		crtc->state->event = NULL;
>> +		new_crtc_state->event = NULL;
>>  		drm_crtc_send_vblank_event(crtc, event);
>>  	}
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does consuming the event
> during the disable phase signal completion to userspace prematurely?
>
> Sending the event here signals to the compositor that the modeset is complete
> before the new mode is programmed and flushed, which could lead to frame
> submission races or compositor stalls.
>
>>  	spin_unlock_irq(&lcdc->drm.event_lock);
>> @@ -272,17 +280,20 @@ static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
>>  	writel(0, lcdc->base + IMX21LCDC_LIER);
>
> [Severity: Critical]
> This is a pre-existing issue, but are the bus clocks disabled before clearing
> the VBLANK interrupt?
>
> Similar to the enable path, writing to the LIER register while the IPG and
> AHB clocks are disabled can trigger a bus fault. Furthermore, since the
> interrupt is unmasked before the clocks are disabled, an incoming IRQ could
> cause the handler to read the LISR register while clocks are off.
>
>>  }
>>  
>> -static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
>> -			       struct drm_plane_state *plane_state,
>> -			       struct drm_crtc_state *crtc_state)
>> +static int imx_lcdc_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);
>> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
>>  	const struct drm_display_mode *mode = &crtc_state->mode;
>> -	const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
>> +	const struct drm_display_mode *old_mode = &old_crtc_state->mode;
>> +	int ret;
>>  
>> -	if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
>> -	    mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
>> -	    mode->hdisplay % 0x10) { /* must be multiple of 16 */
>> -		drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
>> +	if (crtc_state->enable &&
>> +	    (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
>> +	     mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
>> +	     mode->hdisplay % 0x10)) { /* must be multiple of 16 */
>> +		drm_err(crtc->dev, "unsupported display mode (%u x %u)\n",
>>  			mode->hdisplay, mode->vdisplay);
>>  		return -EINVAL;
>>  	}
>> @@ -291,46 +302,116 @@ static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
>>  		old_mode->hdisplay != mode->hdisplay ||
>>  		old_mode->vdisplay != mode->vdisplay;
>
> [Severity: High]
> This is a pre-existing issue, but does this direct assignment to
> mode_changed discard modeset requests initiated by the DRM core?
>
> The core drm_atomic_helper_check_modeset() function may have already set
> mode_changed to true due to connector routing changes or userspace forcing a
> modeset. Using a direct assignment instead of a bitwise OR (|=) silently
> clears that flag if the spatial dimensions haven't changed.
>
>> -	return 0;
>> +	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 imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
>> -				 struct drm_plane_state *old_state)
>> +static void imx_lcdc_plane_helper_atomic_update(struct drm_plane *plane,
>> +						struct drm_atomic_commit *commit)
>>  {
>> -	struct drm_crtc *crtc = &pipe->crtc;
>> -	struct drm_pending_vblank_event *event = crtc->state->event;
>> -	struct drm_plane_state *new_state = pipe->plane.state;
>> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
>> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(commit, plane);
>> +	struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
>>  	struct drm_framebuffer *fb = new_state->fb;
>>  	struct drm_framebuffer *old_fb = old_state->fb;
>>  	struct drm_crtc *old_crtc = old_state->crtc;
>>  	bool mode_changed = false;
>>  
>> +	if (!fb)
>> +		return;
>> +
>>  	if (old_fb && old_fb->format != fb->format)
>>  		mode_changed = true;
>>  	else if (old_crtc != crtc)
>>  		mode_changed = true;
>>  
>> -	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
>> +	imx_lcdc_update_hw_registers(crtc, old_state, mode_changed);
>> +}
>
> [Severity: High]
> Does calling imx_lcdc_update_hw_registers() here cause a persistent reference
> count leak for clk_per?
>
> During a modeset, imx_lcdc_crtc_helper_atomic_enable() already calls
> imx_lcdc_update_hw_registers() which increments the clock reference count
> when the CRTC is enabled.
>
> When the plane helper subsequently calls it again here, the clock prepare
> enable block is hit a second time. However, during display disable, the
> plane update returns early due to a NULL fb, meaning the clock is only
> disabled once in the CRTC disable path.
>
>>  
>> -	if (event) {
>> -		crtc->state->event = NULL;
>> +static int imx_lcdc_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;
>>  
>> -		spin_lock_irq(&crtc->dev->event_lock);
>> +	if (crtc)
>> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>>  
>> -		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);
>> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>
> [Severity: High]
> Can fetching the CRTC state this way cause a kernel panic in the core atomic
> helpers?
>
> If userspace submits a commit that modifies only plane properties without
> affecting the CRTC, the CRTC is not implicitly added to the atomic commit
> state.
>
> In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
> NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
> when it dereferences crtc_state->enable.
>
> Could this use drm_atomic_get_crtc_state() instead to ensure the state is
> brought into the commit if it is missing?

I think it is fine here; I'll just copy the pattern from [1].

[1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann]

Hi

Am 18.07.26 um 21:12 schrieb Ze Huang:
[...]
>>>   
>>> -	if (event) {
>>> -		crtc->state->event = NULL;
>>> +static int imx_lcdc_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;
>>>   
>>> -		spin_lock_irq(&crtc->dev->event_lock);
>>> +	if (crtc)
>>> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>>>   
>>> -		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);
>>> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> [Severity: High]
>> Can fetching the CRTC state this way cause a kernel panic in the core atomic
>> helpers?
>>
>> If userspace submits a commit that modifies only plane properties without
>> affecting the CRTC, the CRTC is not implicitly added to the atomic commit
>> state.
>>
>> In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
>> NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
>> when it dereferences crtc_state->enable.
>>
>> Could this use drm_atomic_get_crtc_state() instead to ensure the state is
>> brought into the commit if it is missing?
> I think it is fine here; I'll just copy the pattern from [1].
>
> [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487

It could be that there's a long standing problem in the overall logic. 
Not having a CRTC (and hence crtc_state) should also mean !fb, so we'd 
return at [1]. If we have a CRTC on the plane but pass a crtc_state of 
NULL, we could get a panic at [2], where it does crtc_state->crtc.  I'm 
not aware of any bug reports about this problem, but it's still an issue.

A number of drivers get this wrong by using 
drm_atomic_helper_get_new_crtc_state(). The bot suggests to use 
drm_atomic_helper_get_crtc_state() instead.  This helper also returns 
the new state. But  if there's no new state, it duplicates the CRTC's 
existing state. That's a bit of an overhead, but probably not an issue.  
Several drivers use this helper, but also get it wrong. They tend to 
return early in the case of !crtc or !fb without calling 
_check_plane_state(). See [3] and [4] for examples.

I think, going with the bot's suggestion to use 
drm_atomic_helper_get_crtc_state() might be the best resolution for now. 
It still needs a crtc pointer, so the pattern is

crtc_state = NULL
if (plane_state->crtc)
     crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)

_check_plane_state(plane_state, crtc_state);

And in this case, _check_plane_state() should work correctly. But you 
can only use _get_crtc_state() in the atomic_check helpers! In the 
atomic_update, atomic_enable, etc helpers, it's too late for the helper 
to copy the CRTC state.

I think some other DRM dev should look over this as well. It's one of 
the trickier things in DRM to get right.

Best regards
Thomas


[1] 
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L912
[2] 
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L907

[3] 
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/mediatek/mtk_plane.c#L230
[4] 
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/tidss/tidss_plane.c#L31



-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3642 bytes --]

On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 18.07.26 um 21:12 schrieb Ze Huang:
> [...]
> > > > -	if (event) {
> > > > -		crtc->state->event = NULL;
> > > > +static int imx_lcdc_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;
> > > > -		spin_lock_irq(&crtc->dev->event_lock);
> > > > +	if (crtc)
> > > > +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> > > > -		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);
> > > > +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > > [Severity: High]
> > > Can fetching the CRTC state this way cause a kernel panic in the core atomic
> > > helpers?
> > > 
> > > If userspace submits a commit that modifies only plane properties without
> > > affecting the CRTC, the CRTC is not implicitly added to the atomic commit
> > > state.
> > > 
> > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
> > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
> > > when it dereferences crtc_state->enable.
> > > 
> > > Could this use drm_atomic_get_crtc_state() instead to ensure the state is
> > > brought into the commit if it is missing?
> > I think it is fine here; I'll just copy the pattern from [1].
> > 
> > [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> 
> It could be that there's a long standing problem in the overall logic. Not
> having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
> [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
> get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
> bug reports about this problem, but it's still an issue.
> 
> A number of drivers get this wrong by using
> drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
> drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
> new state. But  if there's no new state, it duplicates the CRTC's existing
> state. That's a bit of an overhead, but probably not an issue.  Several
> drivers use this helper, but also get it wrong. They tend to return early in
> the case of !crtc or !fb without calling _check_plane_state(). See [3] and
> [4] for examples.
> 
> I think, going with the bot's suggestion to use
> drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
> still needs a crtc pointer, so the pattern is
> 
> crtc_state = NULL
> if (plane_state->crtc)
>     crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
> 
> _check_plane_state(plane_state, crtc_state);
> 
> And in this case, _check_plane_state() should work correctly. But you can
> only use _get_crtc_state() in the atomic_check helpers! In the
> atomic_update, atomic_enable, etc helpers, it's too late for the helper to
> copy the CRTC state.
> 
> I think some other DRM dev should look over this as well. It's one of the
> trickier things in DRM to get right.

drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
everything after that must use either get_new_crtc_state or
get_old_crtc_state, as the global state cannot be modified anymore.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann]

Hi

Am 20.07.26 um 16:08 schrieb Maxime Ripard:
> On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 18.07.26 um 21:12 schrieb Ze Huang:
>> [...]
>>>>> -	if (event) {
>>>>> -		crtc->state->event = NULL;
>>>>> +static int imx_lcdc_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;
>>>>> -		spin_lock_irq(&crtc->dev->event_lock);
>>>>> +	if (crtc)
>>>>> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>>>>> -		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);
>>>>> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>>>> [Severity: High]
>>>> Can fetching the CRTC state this way cause a kernel panic in the core atomic
>>>> helpers?
>>>>
>>>> If userspace submits a commit that modifies only plane properties without
>>>> affecting the CRTC, the CRTC is not implicitly added to the atomic commit
>>>> state.
>>>>
>>>> In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
>>>> NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
>>>> when it dereferences crtc_state->enable.
>>>>
>>>> Could this use drm_atomic_get_crtc_state() instead to ensure the state is
>>>> brought into the commit if it is missing?
>>> I think it is fine here; I'll just copy the pattern from [1].
>>>
>>> [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
>> It could be that there's a long standing problem in the overall logic. Not
>> having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
>> [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
>> get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
>> bug reports about this problem, but it's still an issue.
>>
>> A number of drivers get this wrong by using
>> drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
>> drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
>> new state. But  if there's no new state, it duplicates the CRTC's existing
>> state. That's a bit of an overhead, but probably not an issue.  Several
>> drivers use this helper, but also get it wrong. They tend to return early in
>> the case of !crtc or !fb without calling _check_plane_state(). See [3] and
>> [4] for examples.
>>
>> I think, going with the bot's suggestion to use
>> drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
>> still needs a crtc pointer, so the pattern is
>>
>> crtc_state = NULL
>> if (plane_state->crtc)
>>      crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
>>
>> _check_plane_state(plane_state, crtc_state);
>>
>> And in this case, _check_plane_state() should work correctly. But you can
>> only use _get_crtc_state() in the atomic_check helpers! In the
>> atomic_update, atomic_enable, etc helpers, it's too late for the helper to
>> copy the CRTC state.
>>
>> I think some other DRM dev should look over this as well. It's one of the
>> trickier things in DRM to get right.
> drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> everything after that must use either get_new_crtc_state or
> get_old_crtc_state, as the global state cannot be modified anymore.

Thanks a lot for confirming.

Wrt. the logic in plane atomic_check, we might have to fix a number of 
drivers. As I outlined above, some use _get_new_crtc_state(), some use 
_get_crtc_state() incorrectly. But that's for another series.

Best regards
Thomas

>
> Maxime

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4425 bytes --]

On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
> Am 20.07.26 um 16:08 schrieb Maxime Ripard:
> > On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 18.07.26 um 21:12 schrieb Ze Huang:
> > > [...]
> > > > > > -	if (event) {
> > > > > > -		crtc->state->event = NULL;
> > > > > > +static int imx_lcdc_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;
> > > > > > -		spin_lock_irq(&crtc->dev->event_lock);
> > > > > > +	if (crtc)
> > > > > > +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> > > > > > -		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);
> > > > > > +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > > > > [Severity: High]
> > > > > Can fetching the CRTC state this way cause a kernel panic in the core atomic
> > > > > helpers?
> > > > > 
> > > > > If userspace submits a commit that modifies only plane properties without
> > > > > affecting the CRTC, the CRTC is not implicitly added to the atomic commit
> > > > > state.
> > > > > 
> > > > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
> > > > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
> > > > > when it dereferences crtc_state->enable.
> > > > > 
> > > > > Could this use drm_atomic_get_crtc_state() instead to ensure the state is
> > > > > brought into the commit if it is missing?
> > > > I think it is fine here; I'll just copy the pattern from [1].
> > > > 
> > > > [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> > > It could be that there's a long standing problem in the overall logic. Not
> > > having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
> > > [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
> > > get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
> > > bug reports about this problem, but it's still an issue.
> > > 
> > > A number of drivers get this wrong by using
> > > drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
> > > drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
> > > new state. But  if there's no new state, it duplicates the CRTC's existing
> > > state. That's a bit of an overhead, but probably not an issue.  Several
> > > drivers use this helper, but also get it wrong. They tend to return early in
> > > the case of !crtc or !fb without calling _check_plane_state(). See [3] and
> > > [4] for examples.
> > > 
> > > I think, going with the bot's suggestion to use
> > > drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
> > > still needs a crtc pointer, so the pattern is
> > > 
> > > crtc_state = NULL
> > > if (plane_state->crtc)
> > >      crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
> > > 
> > > _check_plane_state(plane_state, crtc_state);
> > > 
> > > And in this case, _check_plane_state() should work correctly. But you can
> > > only use _get_crtc_state() in the atomic_check helpers! In the
> > > atomic_update, atomic_enable, etc helpers, it's too late for the helper to
> > > copy the CRTC state.
> > > 
> > > I think some other DRM dev should look over this as well. It's one of the
> > > trickier things in DRM to get right.
> > drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> > everything after that must use either get_new_crtc_state or
> > get_old_crtc_state, as the global state cannot be modified anymore.
> 
> Thanks a lot for confirming.
> 
> Wrt. the logic in plane atomic_check, we might have to fix a number of
> drivers. As I outlined above, some use _get_new_crtc_state(), some use
> _get_crtc_state() incorrectly. But that's for another series.

Sigh... I removed all of them a couple of years ago, I guess some crept
back in. Maybe we should warn loudly if it happens?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann]

Hi

Am 20.07.26 um 17:06 schrieb Maxime Ripard:
> On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
>> Am 20.07.26 um 16:08 schrieb Maxime Ripard:
>>> On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 18.07.26 um 21:12 schrieb Ze Huang:
>>>> [...]
>>>>>>> -	if (event) {
>>>>>>> -		crtc->state->event = NULL;
>>>>>>> +static int imx_lcdc_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;
>>>>>>> -		spin_lock_irq(&crtc->dev->event_lock);
>>>>>>> +	if (crtc)
>>>>>>> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>>>>>>> -		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);
>>>>>>> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>>>>>> [Severity: High]
>>>>>> Can fetching the CRTC state this way cause a kernel panic in the core atomic
>>>>>> helpers?
>>>>>>
>>>>>> If userspace submits a commit that modifies only plane properties without
>>>>>> affecting the CRTC, the CRTC is not implicitly added to the atomic commit
>>>>>> state.
>>>>>>
>>>>>> In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
>>>>>> NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
>>>>>> when it dereferences crtc_state->enable.
>>>>>>
>>>>>> Could this use drm_atomic_get_crtc_state() instead to ensure the state is
>>>>>> brought into the commit if it is missing?
>>>>> I think it is fine here; I'll just copy the pattern from [1].
>>>>>
>>>>> [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
>>>> It could be that there's a long standing problem in the overall logic. Not
>>>> having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
>>>> [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
>>>> get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
>>>> bug reports about this problem, but it's still an issue.
>>>>
>>>> A number of drivers get this wrong by using
>>>> drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
>>>> drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
>>>> new state. But  if there's no new state, it duplicates the CRTC's existing
>>>> state. That's a bit of an overhead, but probably not an issue.  Several
>>>> drivers use this helper, but also get it wrong. They tend to return early in
>>>> the case of !crtc or !fb without calling _check_plane_state(). See [3] and
>>>> [4] for examples.
>>>>
>>>> I think, going with the bot's suggestion to use
>>>> drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
>>>> still needs a crtc pointer, so the pattern is
>>>>
>>>> crtc_state = NULL
>>>> if (plane_state->crtc)
>>>>       crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
>>>>
>>>> _check_plane_state(plane_state, crtc_state);
>>>>
>>>> And in this case, _check_plane_state() should work correctly. But you can
>>>> only use _get_crtc_state() in the atomic_check helpers! In the
>>>> atomic_update, atomic_enable, etc helpers, it's too late for the helper to
>>>> copy the CRTC state.
>>>>
>>>> I think some other DRM dev should look over this as well. It's one of the
>>>> trickier things in DRM to get right.
>>> drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
>>> everything after that must use either get_new_crtc_state or
>>> get_old_crtc_state, as the global state cannot be modified anymore.
>> Thanks a lot for confirming.
>>
>> Wrt. the logic in plane atomic_check, we might have to fix a number of
>> drivers. As I outlined above, some use _get_new_crtc_state(), some use
>> _get_crtc_state() incorrectly. But that's for another series.
> Sigh... I removed all of them a couple of years ago, I guess some crept
> back in. Maybe we should warn loudly if it happens?

I think the handling of atomic state is non-trivial and not easily 
discoverable. So yeah. Once sorted out, there should probably be a 
WARN_ONCE of some kind.

Best regards
Thomas


>
> Maxime

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4822 bytes --]

On Mon, Jul 20, 2026 at 05:06:41PM +0200, Maxime Ripard wrote:
> On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
> > Am 20.07.26 um 16:08 schrieb Maxime Ripard:
> > > On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
> > > > Hi
> > > > 
> > > > Am 18.07.26 um 21:12 schrieb Ze Huang:
> > > > [...]
> > > > > > > -	if (event) {
> > > > > > > -		crtc->state->event = NULL;
> > > > > > > +static int imx_lcdc_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;
> > > > > > > -		spin_lock_irq(&crtc->dev->event_lock);
> > > > > > > +	if (crtc)
> > > > > > > +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> > > > > > > -		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);
> > > > > > > +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > > > > > [Severity: High]
> > > > > > Can fetching the CRTC state this way cause a kernel panic in the core atomic
> > > > > > helpers?
> > > > > > 
> > > > > > If userspace submits a commit that modifies only plane properties without
> > > > > > affecting the CRTC, the CRTC is not implicitly added to the atomic commit
> > > > > > state.
> > > > > > 
> > > > > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
> > > > > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
> > > > > > when it dereferences crtc_state->enable.
> > > > > > 
> > > > > > Could this use drm_atomic_get_crtc_state() instead to ensure the state is
> > > > > > brought into the commit if it is missing?
> > > > > I think it is fine here; I'll just copy the pattern from [1].
> > > > > 
> > > > > [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> > > > It could be that there's a long standing problem in the overall logic. Not
> > > > having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
> > > > [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
> > > > get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
> > > > bug reports about this problem, but it's still an issue.
> > > > 
> > > > A number of drivers get this wrong by using
> > > > drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
> > > > drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
> > > > new state. But  if there's no new state, it duplicates the CRTC's existing
> > > > state. That's a bit of an overhead, but probably not an issue.  Several
> > > > drivers use this helper, but also get it wrong. They tend to return early in
> > > > the case of !crtc or !fb without calling _check_plane_state(). See [3] and
> > > > [4] for examples.
> > > > 
> > > > I think, going with the bot's suggestion to use
> > > > drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
> > > > still needs a crtc pointer, so the pattern is
> > > > 
> > > > crtc_state = NULL
> > > > if (plane_state->crtc)
> > > >      crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
> > > > 
> > > > _check_plane_state(plane_state, crtc_state);
> > > > 
> > > > And in this case, _check_plane_state() should work correctly. But you can
> > > > only use _get_crtc_state() in the atomic_check helpers! In the
> > > > atomic_update, atomic_enable, etc helpers, it's too late for the helper to
> > > > copy the CRTC state.
> > > > 
> > > > I think some other DRM dev should look over this as well. It's one of the
> > > > trickier things in DRM to get right.
> > > drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> > > everything after that must use either get_new_crtc_state or
> > > get_old_crtc_state, as the global state cannot be modified anymore.
> > 
> > Thanks a lot for confirming.
> > 
> > Wrt. the logic in plane atomic_check, we might have to fix a number of
> > drivers. As I outlined above, some use _get_new_crtc_state(), some use
> > _get_crtc_state() incorrectly. But that's for another series.
> 
> Sigh... I removed all of them a couple of years ago, I guess some crept
> back in. Maybe we should warn loudly if it happens?

I started to look into it for drm_atomic_helper_get_crtc_state and I
cannot find an occurence of this in drm-misc-next. Do you have a
specific example in mind?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann]

Hi

Am 21.07.26 um 11:28 schrieb Maxime Ripard:
> On Mon, Jul 20, 2026 at 05:06:41PM +0200, Maxime Ripard wrote:
>> On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
>>> Am 20.07.26 um 16:08 schrieb Maxime Ripard:
>>>> On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 18.07.26 um 21:12 schrieb Ze Huang:
>>>>> [...]
>>>>>>>> -	if (event) {
>>>>>>>> -		crtc->state->event = NULL;
>>>>>>>> +static int imx_lcdc_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;
>>>>>>>> -		spin_lock_irq(&crtc->dev->event_lock);
>>>>>>>> +	if (crtc)
>>>>>>>> +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>>>>>>>> -		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);
>>>>>>>> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>>>>>>> [Severity: High]
>>>>>>> Can fetching the CRTC state this way cause a kernel panic in the core atomic
>>>>>>> helpers?
>>>>>>>
>>>>>>> If userspace submits a commit that modifies only plane properties without
>>>>>>> affecting the CRTC, the CRTC is not implicitly added to the atomic commit
>>>>>>> state.
>>>>>>>
>>>>>>> In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
>>>>>>> NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
>>>>>>> when it dereferences crtc_state->enable.
>>>>>>>
>>>>>>> Could this use drm_atomic_get_crtc_state() instead to ensure the state is
>>>>>>> brought into the commit if it is missing?
>>>>>> I think it is fine here; I'll just copy the pattern from [1].
>>>>>>
>>>>>> [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
>>>>> It could be that there's a long standing problem in the overall logic. Not
>>>>> having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
>>>>> [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
>>>>> get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
>>>>> bug reports about this problem, but it's still an issue.
>>>>>
>>>>> A number of drivers get this wrong by using
>>>>> drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
>>>>> drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
>>>>> new state. But  if there's no new state, it duplicates the CRTC's existing
>>>>> state. That's a bit of an overhead, but probably not an issue.  Several
>>>>> drivers use this helper, but also get it wrong. They tend to return early in
>>>>> the case of !crtc or !fb without calling _check_plane_state(). See [3] and
>>>>> [4] for examples.
>>>>>
>>>>> I think, going with the bot's suggestion to use
>>>>> drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
>>>>> still needs a crtc pointer, so the pattern is
>>>>>
>>>>> crtc_state = NULL
>>>>> if (plane_state->crtc)
>>>>>       crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
>>>>>
>>>>> _check_plane_state(plane_state, crtc_state);
>>>>>
>>>>> And in this case, _check_plane_state() should work correctly. But you can
>>>>> only use _get_crtc_state() in the atomic_check helpers! In the
>>>>> atomic_update, atomic_enable, etc helpers, it's too late for the helper to
>>>>> copy the CRTC state.
>>>>>
>>>>> I think some other DRM dev should look over this as well. It's one of the
>>>>> trickier things in DRM to get right.
>>>> drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
>>>> everything after that must use either get_new_crtc_state or
>>>> get_old_crtc_state, as the global state cannot be modified anymore.
>>> Thanks a lot for confirming.
>>>
>>> Wrt. the logic in plane atomic_check, we might have to fix a number of
>>> drivers. As I outlined above, some use _get_new_crtc_state(), some use
>>> _get_crtc_state() incorrectly. But that's for another series.
>> Sigh... I removed all of them a couple of years ago, I guess some crept
>> back in. Maybe we should warn loudly if it happens?
> I started to look into it for drm_atomic_helper_get_crtc_state and I
> cannot find an occurence of this in drm-misc-next. Do you have a
> specific example in mind?

It's called drm_atomic_get_crtc_state(). [1] :)    I've not evaluated 
all calls, but there are cases like [2], which returns early from !fb 
before running the whole drm_atomic_helper_check_plane_state() 
machinery. It should have at least set plane_state->visible to false.  
Better would be to call drm_atomic_helper_check_plane_state() in any case.
[1] 
https://elixir.bootlin.com/linux/v7.1.4/A/ident/drm_atomic_get_crtc_state
[2] 
https://elixir.bootlin.com/linux/v7.1.4/source/drivers/gpu/drm/mediatek/mtk_plane.c#L239

Best regards
Thomas


>
> Maxime

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5852 bytes --]

On Tue, Jul 21, 2026 at 11:37:32AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 21.07.26 um 11:28 schrieb Maxime Ripard:
> > On Mon, Jul 20, 2026 at 05:06:41PM +0200, Maxime Ripard wrote:
> > > On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
> > > > Am 20.07.26 um 16:08 schrieb Maxime Ripard:
> > > > > On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
> > > > > > Hi
> > > > > > 
> > > > > > Am 18.07.26 um 21:12 schrieb Ze Huang:
> > > > > > [...]
> > > > > > > > > -	if (event) {
> > > > > > > > > -		crtc->state->event = NULL;
> > > > > > > > > +static int imx_lcdc_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;
> > > > > > > > > -		spin_lock_irq(&crtc->dev->event_lock);
> > > > > > > > > +	if (crtc)
> > > > > > > > > +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> > > > > > > > > -		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);
> > > > > > > > > +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > > > > > > > [Severity: High]
> > > > > > > > Can fetching the CRTC state this way cause a kernel panic in the core atomic
> > > > > > > > helpers?
> > > > > > > > 
> > > > > > > > If userspace submits a commit that modifies only plane properties without
> > > > > > > > affecting the CRTC, the CRTC is not implicitly added to the atomic commit
> > > > > > > > state.
> > > > > > > > 
> > > > > > > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
> > > > > > > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
> > > > > > > > when it dereferences crtc_state->enable.
> > > > > > > > 
> > > > > > > > Could this use drm_atomic_get_crtc_state() instead to ensure the state is
> > > > > > > > brought into the commit if it is missing?
> > > > > > > I think it is fine here; I'll just copy the pattern from [1].
> > > > > > > 
> > > > > > > [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> > > > > > It could be that there's a long standing problem in the overall logic. Not
> > > > > > having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
> > > > > > [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
> > > > > > get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
> > > > > > bug reports about this problem, but it's still an issue.
> > > > > > 
> > > > > > A number of drivers get this wrong by using
> > > > > > drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
> > > > > > drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
> > > > > > new state. But  if there's no new state, it duplicates the CRTC's existing
> > > > > > state. That's a bit of an overhead, but probably not an issue.  Several
> > > > > > drivers use this helper, but also get it wrong. They tend to return early in
> > > > > > the case of !crtc or !fb without calling _check_plane_state(). See [3] and
> > > > > > [4] for examples.
> > > > > > 
> > > > > > I think, going with the bot's suggestion to use
> > > > > > drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
> > > > > > still needs a crtc pointer, so the pattern is
> > > > > > 
> > > > > > crtc_state = NULL
> > > > > > if (plane_state->crtc)
> > > > > >       crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
> > > > > > 
> > > > > > _check_plane_state(plane_state, crtc_state);
> > > > > > 
> > > > > > And in this case, _check_plane_state() should work correctly. But you can
> > > > > > only use _get_crtc_state() in the atomic_check helpers! In the
> > > > > > atomic_update, atomic_enable, etc helpers, it's too late for the helper to
> > > > > > copy the CRTC state.
> > > > > > 
> > > > > > I think some other DRM dev should look over this as well. It's one of the
> > > > > > trickier things in DRM to get right.
> > > > > drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> > > > > everything after that must use either get_new_crtc_state or
> > > > > get_old_crtc_state, as the global state cannot be modified anymore.
> > > > Thanks a lot for confirming.
> > > > 
> > > > Wrt. the logic in plane atomic_check, we might have to fix a number of
> > > > drivers. As I outlined above, some use _get_new_crtc_state(), some use
> > > > _get_crtc_state() incorrectly. But that's for another series.
> > > Sigh... I removed all of them a couple of years ago, I guess some crept
> > > back in. Maybe we should warn loudly if it happens?
> > I started to look into it for drm_atomic_helper_get_crtc_state and I
> > cannot find an occurence of this in drm-misc-next. Do you have a
> > specific example in mind?
> 
> It's called drm_atomic_get_crtc_state(). [1] :)

Yeah, sorry :)

> I've not evaluated all calls, but there are cases like [2], which
> returns early from !fb before running the whole
> drm_atomic_helper_check_plane_state() machinery. It should have at
> least set plane_state->visible to false.  Better would be to call
> drm_atomic_helper_check_plane_state() in any case.

Oh right, I misunderstood what you were saying then. I thought you were
saying that there's drm_atomic_get_crtc_state() calls in !atomic_check,
which doesn't seem to be the case.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Ze Huang]

On Mon Jul 20, 2026 at 10:08 PM CST, Maxime Ripard wrote:
> On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
>> Hi
>> 
>> Am 18.07.26 um 21:12 schrieb Ze Huang:
>> [...]
>> > > > -	if (event) {
>> > > > -		crtc->state->event = NULL;
>> > > > +static int imx_lcdc_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;
>> > > > -		spin_lock_irq(&crtc->dev->event_lock);
>> > > > +	if (crtc)
>> > > > +		crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>> > > > -		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);
>> > > > +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> > > [Severity: High]
>> > > Can fetching the CRTC state this way cause a kernel panic in the core atomic
>> > > helpers?
>> > > 
>> > > If userspace submits a commit that modifies only plane properties without
>> > > affecting the CRTC, the CRTC is not implicitly added to the atomic commit
>> > > state.
>> > > 
>> > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL. Passing this
>> > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes a crash
>> > > when it dereferences crtc_state->enable.
>> > > 
>> > > Could this use drm_atomic_get_crtc_state() instead to ensure the state is
>> > > brought into the commit if it is missing?
>> > I think it is fine here; I'll just copy the pattern from [1].
>> > 
>> > [1] https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
>> 
>> It could be that there's a long standing problem in the overall logic. Not
>> having a CRTC (and hence crtc_state) should also mean !fb, so we'd return at
>> [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we could
>> get a panic at [2], where it does crtc_state->crtc.  I'm not aware of any
>> bug reports about this problem, but it's still an issue.
>> 
>> A number of drivers get this wrong by using
>> drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
>> drm_atomic_helper_get_crtc_state() instead.  This helper also returns the
>> new state. But  if there's no new state, it duplicates the CRTC's existing
>> state. That's a bit of an overhead, but probably not an issue.  Several
>> drivers use this helper, but also get it wrong. They tend to return early in
>> the case of !crtc or !fb without calling _check_plane_state(). See [3] and
>> [4] for examples.
>> 
>> I think, going with the bot's suggestion to use
>> drm_atomic_helper_get_crtc_state() might be the best resolution for now. It
>> still needs a crtc pointer, so the pattern is
>> 

Thank you Thomas and Maxime for the detailed explanations.

I will follow the suggested pattern here:

>> crtc_state = NULL
>> if (plane_state->crtc)
>>     crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
>> 
>> _check_plane_state(plane_state, crtc_state);

IIUC, atomic_check is the last stage where valid to extend commit state.
After that, the state is fixed and helpers should only use
get_new_crtc_state() or get_old_crtc_state().

My only remaining question is about the doc warning [1]:

  * WARNING: Drivers may only add new CRTC states to a @state if
  * drm_atomic_state.allow_modeset is set, or if it's a driver-internal
  * commit not created by userspace through an IOCTL call.

if we end up adding the CRTC state for a userspace non-modeset commit,
it could hit the requested/affected CRTC warning in
drm_atomic_check_only() [2]. That would still be more acceptable than
crashing, though.

[1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L355
[2] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L1746

Thanks,
Ze

>> 
>> And in this case, _check_plane_state() should work correctly. But you can
>> only use _get_crtc_state() in the atomic_check helpers! In the
>> atomic_update, atomic_enable, etc helpers, it's too late for the helper to
>> copy the CRTC state.
>> 
>> I think some other DRM dev should look over this as well. It's one of the
>> trickier things in DRM to get right.
>
> drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> everything after that must use either get_new_crtc_state or
> get_old_crtc_state, as the global state cannot be modified anymore.
>
> Maxime