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

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

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 <ze.huang@oss.qualcomm.com>
> 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-b4e1ca053623@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