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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Sleeping functions called from atomic context in CRTC callbacks
--

--- Patch [3]: [PATCH v3 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit ba616a75cb8eb47f91848c4e4530a9f09433e557
Author: Ze Huang <[email protected]>

drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers
    
Convert MCDE to explicit plane, CRTC and encoder objects.
    
Handle pending page-flip events from the CRTC atomic flush path using the
commit-local CRTC state, and avoid manual connector/encoder attachment
before the connector has been created by the bridge path.

> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 257a6e84dd583..50c90cf84a9a3 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
[ ... ]
> @@ -1149,16 +1168,15 @@ static void mcde_setup_dsi(struct mcde *mcde, const struct drm_display_mode *mod
>  	*dsi_formatter_frame = formatter_frame;
>  }
>  
> -static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
> -				struct drm_crtc_state *cstate,
> -				struct drm_plane_state *plane_state)
> +static void mcde_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					   struct drm_atomic_commit *commit)

[Severity: High]
This is a pre-existing issue and not a regression introduced by this patch,
but does this function call sleeping APIs from an atomic context?

During non-blocking commits, CRTC atomic callbacks execute in an atomic
context where sleeping is not allowed. Looking at the implementation of
mcde_crtc_helper_atomic_enable() in the wider context, it eventually calls
regulator_enable() and clk_prepare_enable(), which can sleep.

Can this lead to kernel warnings or deadlocks when this path is invoked?

>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_plane *plane = &pipe->plane;
>  	struct drm_device *drm = crtc->dev;
>  	struct mcde *mcde = to_mcde(drm);
[ ... ]
> @@ -1298,9 +1316,10 @@ static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
>  	dev_info(drm->dev, "MCDE display is enabled\n");
>  }
>  
> -static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
> +static void mcde_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					    struct drm_atomic_commit *commit)

[Severity: High]
This isn't a bug introduced by this patch, but does the disable callback also
sleep in an atomic context?

Similar to the enable path, mcde_crtc_helper_atomic_disable() runs in an
atomic context during non-blocking commits. However, it calls
regulator_disable(), clk_disable_unprepare(), and directly invokes
usleep_range(). 

Could these sleeping functions cause system instability when called from this
atomic callback?

>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
>  	struct drm_device *drm = crtc->dev;
>  	struct mcde *mcde = to_mcde(drm);
[ ... ]
> +static void mcde_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> +					  struct drm_atomic_commit *commit)
> +{
> +	struct mcde *mcde = to_mcde(crtc->dev);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, &mcde->plane);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> +	struct drm_pending_vblank_event *event = crtc_state->event;
[ ... ]
> -	/*
> -	 * We do not start sending framebuffer updates before the
> -	 * display is enabled. Update events will however be dispatched
> -	 * from the DRM core before the display is enabled.
> -	 */
> -	if (fb) {
> -		mcde_set_extsrc(mcde, drm_fb_dma_get_gem_addr(fb, pstate, 0));
> -		dev_info_once(mcde->dev, "first update of display contents\n");
> -		/*
> -		 * Usually the flow is already active, unless we are in
> -		 * oneshot mode, then we need to kick the flow right here.
> -		 */
> -		if (mcde->flow_active == 0)
> -			mcde_start_flow(mcde);
> -	} else {
> -		/*
> -		 * If an update is receieved before the MCDE is enabled
> -		 * (before mcde_display_enable() is called) we can't really
> -		 * do much with that buffer.
> -		 */
> -		dev_info(mcde->dev, "ignored a display update\n");
> -	}
> +	if (crtc_state->active && plane_state && plane_state->fb && mcde->flow_active == 0)
> +		mcde_start_flow(mcde);

[Severity: High]
This is also a pre-existing issue, but does mcde_start_flow() ultimately
sleep?

Inside mcde_crtc_helper_atomic_flush(), the code calls mcde_start_flow().
If mcde_start_flow() disables the FIFO via mcde_disable_fifo(), it appears
to use usleep_range() to wait for the FIFO to drain. Since atomic_flush()
runs in an atomic context during non-blocking commits, can this sleeping
function cause issues here as well?

>  }

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