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

On Mon Jul 27, 2026 at 4:00 AM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Calling sleeping functions from CRTC atomic enable and disable callbacks violates atomic context constraints.
> - [High] Setting `cstate->mode_changed = true` inside the plane's `atomic_check` callback bypasses downstream modeset routing, leaving bridges active during CRTC resets.
> --
>
> --- Patch [7]: [PATCH v3 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers ---
> commit c9e2ab55e3b69f124e64f349a1ced762885b2aea
> Author: Ze Huang <[email protected]>
> Subject: drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers
>
> Convert TVE200 to explicit plane, CRTC and encoder objects. Keep generic
> plane-state validation before TVE200-specific mode, alignment, pitch and
> format-change checks.
>
>> diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
>> index 26b6c65ef6fd0..beb3a5a63b5d5 100644
>> --- a/drivers/gpu/drm/tve200/tve200_display.c
>> +++ b/drivers/gpu/drm/tve200/tve200_display.c
> [ ... ]
>> @@ -69,13 +71,34 @@ irqreturn_t tve200_irq(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> -static int tve200_display_check(struct drm_simple_display_pipe *pipe,
>> -			       struct drm_plane_state *pstate,
>> -			       struct drm_crtc_state *cstate)
>> +static int tve200_plane_helper_atomic_check(struct drm_plane *plane,
>> +					    struct drm_atomic_commit *commit)
>>  {
>
> [Severity: High]
> This is a pre-existing issue, but does setting cstate->mode_changed = true
> inside the plane check hook bypass downstream modeset routing?
>
> In tve200_plane_helper_atomic_check(), the code raises a mode change if the
> framebuffer format changes:
>
>     if (old_fb && old_fb->format != fb->format)
>             cstate->mode_changed = true;
>
> In the default DRM state check sequence, plane checks are executed after the
> modeset checks. Because mode_changed is raised late and the driver does not
> explicitly loop back to recalculate modeset routing, the downstream
> encoders and bridges may miss their disable/enable hooks. Could this leave
> bridges active during a CRTC reset?

Yes, the DRM helper path does use mode_changed to drive modeset
routing and the bridge/encoder/CRTC modeset hooks.

As mentioned for patch 2, I would prefer not to mix too much fixes into
this simple-KMS conversion series. If we want to make this stricter, the
driver can grow a custom .atomic_check wrapper and rerun
drm_atomic_helper_check_modeset() after plane checks, but that should be
a separate follow-up/preparatory patch.

>
> [ ... ]
>> @@ -119,16 +142,72 @@ static int tve200_display_check(struct drm_simple_display_pipe *pipe,
>>  	return 0;
>>  }
>>  
> [ ... ]
>> +static void tve200_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>> +					     struct drm_atomic_commit *commit)
>>  {
>> -	struct drm_crtc *crtc = &pipe->crtc;
>> -	struct drm_plane *plane = &pipe->plane;
>>  	struct drm_device *drm = crtc->dev;
>>  	struct tve200_drm_dev_private *priv = drm->dev_private;
>> +	struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
>>  	const struct drm_display_mode *mode = &cstate->mode;
>
> [Severity: High]
> This is a pre-existing issue, but does this code sleep in atomic context?
>
> The DRM core invokes CRTC atomic enable callbacks during display commits,
> which run in atomic context. However, tve200_crtc_helper_atomic_enable()
> calls sleeping functions:
>
>     clk_prepare_enable(priv->clk);
>     ...
>     usleep_range(30000, 50000);
>
> I see this logic was carried over directly from the old
> drm_simple_display_pipe hooks. Since sleeping in atomic context can cause
> kernel warnings and potential deadlocks, is there a safe way to handle this
> enable sequence?

False positive

>
> [ ... ]
>> @@ -240,9 +319,9 @@ static void tve200_display_enable(struct drm_simple_display_pipe *pipe,
>>  	drm_crtc_vblank_on(crtc);
>>  }
>>  
>> -static void tve200_display_disable(struct drm_simple_display_pipe *pipe)
>> +static void tve200_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +					      struct drm_atomic_commit *commit)
>>  {
>> -	struct drm_crtc *crtc = &pipe->crtc;
>>  	struct drm_device *drm = crtc->dev;
>>  	struct tve200_drm_dev_private *priv = drm->dev_private;
>
> [Severity: High]
> This is a pre-existing issue, but similar to the enable path above, does the
> disable callback also sleep in atomic context?
>
> tve200_crtc_helper_atomic_disable() eventually calls clk_disable_unprepare(),
> which can sleep:
>
>     clk_disable_unprepare(priv->clk);
>
> Since this runs in the atomic disable path, could this lead to a deadlock?

False positive