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