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

On Mon Jul 27, 2026 at 3:57 AM CST, sashiko-bot wrote:
> 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?

False positive

>
>>  {
>> -	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?

False positive

>
>>  {
>> -	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?
>

False positive

>>  }