Re: [PATCH v3 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers

On Mon Jul 27, 2026 at 3:59 AM CST, sashiko-bot wrote:
> 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 <[email protected]>
> 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?

False positive

>
> [ ... ]
>> -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?

False positive