Re: [PATCH v3 4/8] 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:

Pre-existing issues:
- [High] CRTC atomic enable and disable callbacks call sleeping functions (`msleep`, `clk_prepare_enable`, `clk_disable_unprepare`), which violates the subsystem rules regarding atomic contexts.
--

--- Patch [4]: [PATCH v3 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit e2dfe2bacf45ede4c23cfee6fb0f779ed622850c
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 also moves timing and format validation to explicit
atomic check paths, and page-flip event handling to the CRTC commit path.

> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 5d10bc5fdf1f6..deac1dee7838a 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
[ ... ]
> -static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> -				 struct drm_crtc_state *cstate,
> -				 struct drm_plane_state *plane_state)
> +static void pl111_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					    struct drm_atomic_commit *commit)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does this function safely run
in an atomic context?

CRTC atomic enable callbacks execute in an atomic context where sleeping is
forbidden. Looking at the wider implementation of
pl111_crtc_helper_atomic_enable(), it appears to still call sleeping functions
like clk_prepare_enable() and msleep().

Could this lead to scheduling-while-atomic kernel warnings or potential
deadlocks when userspace sets a display mode and triggers the enable path?

[ ... ]
> -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 *commit)
>  {
[ ... ]
>  	/* Disable */
>  	writel(0, priv->regs + priv->ctrl);
>  
>  	clk_disable_unprepare(priv->clk);
>  }

[Severity: High]
This is also a pre-existing issue, but is it safe to use sleeping functions
like clk_disable_unprepare() and msleep() during the disable sequence?

Similar to the enable path, pl111_crtc_helper_atomic_disable() operates as
an atomic disable callback, which also runs in an atomic context. Could these
sleep calls cause system instability or scheduling warnings when the CRTC is
powered off?

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