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

On Mon Jul 27, 2026 at 3:55 AM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Critical] NULL pointer dereference in `imx_lcdc_crtc_helper_atomic_enable` and `imx_lcdc_update_hw_registers` when the primary plane has no framebuffer.
>
> Pre-existing issues:
> - [High] Sleeping in atomic context by calling clock manipulation functions from the plane `atomic_update` callback.
> --
>
> --- Patch [2]: [PATCH v3 2/2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers ---
> commit e5bb94188c05e612166e809c2f562025fecbb70c
> Author: Ze Huang <[email protected]>
>
> drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers
>
> Convert i.MX LCDC to explicit primary plane, CRTC and encoder objects. Keep
> no-scaling plane check and GEM framebuffer prepare callback from simple-KMS
> path.
>
>> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
>> index e00e52c1c46ec..e0784ce0125b7 100644
>> --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
>> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> [ ... ]
>> @@ -206,7 +211,7 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>>  
>>  	clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
>>  					mode->clock * 1000);
>> -	bpp = imx_lcdc_get_format(plane_state->fb->format->format);
>> +	bpp = imx_lcdc_get_format(pstate->fb->format->format);
>
> [Severity: Critical]
> Can this dereference a NULL pointer if the primary plane has no framebuffer?

It could not be possible.

If the plane was attached to crtc, the fb would be checked [1] in the
atomic check phase (before crtc atomic enable):

    /* either *both* CRTC and FB must be set, or neither */
    if (crtc && !fb) {
        drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
                       plane->base.id, plane->name);
        return -EINVAL;

[1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L715

>
> If a userspace process issues an atomic modeset commit that enables the CRTC
> but leaves the primary plane with no framebuffer, pstate->fb would be NULL.
> The drm_atomic_helper_check_crtc_primary_plane() helper checks if the
> primary plane is attached, but doesn't guarantee a visible framebuffer.
>
> Could this lead to a crash when querying the format here, or when passing
> the fb to drm_fb_dma_get_gem_addr() inside imx_lcdc_update_hw_registers()
> just below?
>
> [ ... ]
>> @@ -272,66 +283,151 @@ static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
> [ ... ]
>> +	if (old_fb && old_fb->format != fb->format)
>> +		mode_changed = true;
>> +	else if (old_crtc != crtc)
>> +		mode_changed = true;
>> +
>> -	if (!drm_atomic_crtc_needs_modeset(crtc->state))
>> -		imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
>> +	if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
>> +		imx_lcdc_update_hw_registers(crtc, old_crtc_state, new_crtc_state,
>> +					     new_state, mode_changed);
>
> [Severity: High]
> This is a pre-existing issue, but does this sleep in an atomic context?
>
> When userspace requests a plane update that changes the pixel format but
> does not alter the display resolution, drm_atomic_crtc_needs_modeset()
> evaluates to false, but mode_changed is set to true. 
>
> This causes imx_lcdc_update_hw_registers() to be called with mode_set=true:
>
> imx_lcdc_plane_helper_atomic_update()
>   imx_lcdc_update_hw_registers(..., mode_changed);
>     clk_disable_unprepare(lcdc->clk_per);
>
> Since clk_disable_unprepare() acquires a mutex and sleeps, is it safe to
> call this from the plane's atomic_update callback, which executes in
> atomic context where sleeping is forbidden?

False positive