Re: [PATCH 5/9] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconsumed CRTC VBLANK event when disabling the CRTC causes a memory leak and userspace hang.
--

commit f35a0721a36bbf78ed41d1be8bb186efb88c9092
Author: Ze Huang <ze.huang@oss.qualcomm.com>
Subject: drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers

This commit replaces the PL111 simple display pipe with explicit plane, CRTC, 
and encoder objects. It preserves existing hardware programming and vblank 
behavior while using the GEM framebuffer prepare helper.

> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 5d10bc5fdf1f6..b1bdd4c9dbe68 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
[ ... ]
> -static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> +static void pl111_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					     struct drm_atomic_commit *state)
>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
>  	struct drm_device *drm = crtc->dev;
>  	struct pl111_drm_dev_private *priv = drm->dev_private;
>  	u32 cntl;
[ ... ]
> -static void pl111_display_update(struct drm_simple_display_pipe *pipe,
> -				 struct drm_plane_state *old_pstate)
> +static void pl111_plane_helper_atomic_update(struct drm_plane *plane,
> +					     struct drm_atomic_commit *state)
>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_device *drm = crtc->dev;
> -	struct pl111_drm_dev_private *priv = drm->dev_private;
> -	struct drm_pending_vblank_event *event = crtc->state->event;
> -	struct drm_plane *plane = &pipe->plane;
> +	struct drm_crtc *crtc = plane->state->crtc;
> +	struct drm_device *drm;
> +	struct pl111_drm_dev_private *priv;
> +	struct drm_pending_vblank_event *event;
>  	struct drm_plane_state *pstate = plane->state;
>  	struct drm_framebuffer *fb = pstate->fb;
>  
> +	if (!crtc)
> +		return;
> +

[Severity: High]
When disabling the CRTC, does this code leak the vblank event?

If an atomic commit disables the CRTC and requests a completion event, this
early return in pl111_plane_helper_atomic_update() triggers because 
plane->state->crtc is NULL when the plane is disabled.

Since the event consumption logic previously provided by the 
drm_simple_display_pipe framework has been removed, and 
pl111_crtc_helper_atomic_disable() also lacks logic to consume 
crtc->state->event, won't the event remain attached and unconsumed?

This can trigger a kernel warning in drm_atomic_helper_commit_hw_done() 
(which warns if the event is still attached) and cause userspace 
compositors to hang indefinitely waiting for the page flip event.

> +	drm = crtc->dev;
> +	priv = drm->dev_private;
> +	event = crtc->state->event;
> +
>  	if (fb) {
>  		u32 addr = drm_fb_dma_get_gem_addr(fb, pstate, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=5