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 <[email protected]>
>
> 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
