Re: [PATCH v3 2/2] 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 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 <ze.huang@oss.qualcomm.com>

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?

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-drm-simple-kms-removal-v3-0-de36e534f7a1@oss.qualcomm.com?part=2