Re: [PATCH 3/9] 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] CRTC disable operations fail unconditionally due to early mode validation.
- [High] Plane atomic check rejects offline plane updates, breaking display disabling.
- [High] Missing drm_crtc_vblank_on() and drm_crtc_vblank_off() calls break vblank synchronization.
- [High] Missing .enable_vblank hook in drm_crtc_funcs causes kernel warnings and breaks vblank.

Pre-existing issues:
- [High] Racy direct state access in atomic callbacks leading to potential use-after-free.
- [High] Unbounded clock reference count leak on CRTC disable cycles.
--

commit 404ed5ff8b68b90a5ba44e0c6a32b76744948691
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

This commit converts the i.MX LCDC driver to use explicit primary plane,
CRTC, and encoder objects instead of the simple display pipe helpers. It
retains the no-scaling plane checks and GEM framebuffer prepare callbacks,
adjusting hardware state only when a framebuffer is present.

> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> index c67fe80b8115e..f0c4c19f91166 100644
> --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
[ ... ]
> -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 drm_plane_state *new_state = crtc->primary->state;

[Severity: High]
This is a pre-existing issue, but does this direct state access create a race
condition?

In non-blocking atomic commits, the DRM core synchronously swaps these object
pointers before the asynchronous worker executes. If multiple commits are
queued, we might retrieve the state for a future commit instead of the
current one, which could lead to programming hardware with an unpinned or
incorrect framebuffer.

[ ... ]
> @@ -230,34 +231,34 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
[ ... ]
>  	imx_lcdc_update_hw_registers(crtc, NULL, true);
>  
>  	/* Enable VBLANK Interrupt */
>  	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
>  }

[Severity: High]
Should drm_crtc_vblank_on() be called here?

Without notifying the DRM core that the hardware vblank interrupt is enabled,
the core might consider the vblank state as off. This causes vblank waits to
fail unconditionally, forcing the plane atomic update to immediately send page
flip events to userspace rather than waiting for vsync.

[ ... ]
> -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 *state)
>  {
> -	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> -	struct drm_crtc *crtc = &lcdc->pipe.crtc;
> +	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
>  	struct drm_pending_vblank_event *event;
>  
>  	clk_disable_unprepare(lcdc->clk_ahb);
>  	clk_disable_unprepare(lcdc->clk_ipg);
>  
> -	if (pipe->crtc.enabled)
> +	if (crtc->enabled)
>  		clk_disable_unprepare(lcdc->clk_per);

[Severity: High]
This is a pre-existing issue, but does this leak a clock reference?

The atomic_disable callback runs after the DRM core updates legacy state
variables, so crtc->enabled is guaranteed to be false here. This means the
clock might never be disabled during a DPMS cycle.

[ ... ]
>  	spin_unlock_irq(&lcdc->drm.event_lock);
>  
>  	/* Disable VBLANK Interrupt */
>  	writel(0, lcdc->base + IMX21LCDC_LIER);
>  }

[Severity: High]
Is a call to drm_crtc_vblank_off() needed here to synchronously disable vblank
processing in the DRM core when shutting down the hardware interrupt?

[ ... ]
> -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 *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 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 = &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 */

[Severity: High]
Will this cause CRTC disable operations to fail?

When userspace disables the CRTC (like via DPMS OFF), crtc_state->enable is
false and the display mode might be zeroed out. Validating the mode dimensions
before checking crtc_state->enable could cause this check to return -EINVAL
unconditionally.

[ ... ]
> -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 *state)
>  {
> -	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(state, plane);
> +	struct drm_plane_state *new_state = plane->state;

[Severity: High]
This is a pre-existing issue, but does assigning new_state directly from
plane->state risk a race condition?

For non-blocking commits, fetching the new plane state from the
drm_atomic_commit object avoids reading state from a potentially concurrent
future commit.

[ ... ]
> +static int imx_lcdc_plane_helper_atomic_check(struct drm_plane *plane,
> +					      struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct drm_crtc_state *crtc_state = NULL;
> +	int ret;
> +
> +	if (crtc)
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);

[Severity: High]
Does passing false for the can_update_disabled parameter reject offline plane
updates?

If userspace tries to disable the CRTC without detaching the framebuffer, the
helper will see a plane on a disabled CRTC and return -EINVAL, which breaks
display disabling.

[ ... ]
> +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,
> +};

[Severity: High]
Should this struct include implementations for .enable_vblank and
.disable_vblank?

When drm_vblank_get() is called to wait for vsync, the DRM core requires the
enable_vblank hook. Omitting these causes a kernel warning and makes the wait
return -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=3