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

["Ze Huang" <ze.huang@oss.qualcomm.com>]

On Sun Jul 5, 2026 at 2:46 AM CST, sashiko-bot wrote:
> 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.
>

Will switch to drm_atomic_get_new_plane_state()

> [ ... ]
>> @@ -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?
>

Yes, will do

>
> 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.

You're right, crtc->enabled is no longer valid here, I'll use old state
instead.

    old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);

    clk_disable_unprepare(lcdc->clk_ahb);
    clk_disable_unprepare(lcdc->clk_ipg);

    if (old_crtc_state->enable)
        clk_disable_unprepare(lcdc->clk_per);

>
> [ ... ]
>>  	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?
>

Yes, indeed, will do.

> [ ... ]
>> -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.
>

OK, will check crtc_state->enable before checking mode dimensions

>
> [ ... ]
>> -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.
>

Will do

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

It is inherited from drm_simple_kms_plane_atomic_check(). I'd like to
keep it the same way.

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

Right, we need enable_vblank and disable_vblank here. I'll implement two
empty hooks.

Thanks,
Ze