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

[sashiko-bot@kernel.org]

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